Messages in this thread | | | Date | Tue, 25 Jan 2022 14:29:57 +0100 | Subject | Re: [PATCH v2 20/30] KVM: s390: pci: provide routines for enabling/disabling IOAT assist | From | Pierre Morel <> |
| |
On 1/14/22 21:31, Matthew Rosato wrote: > These routines will be wired into the vfio_pci_zdev ioctl handlers to > respond to requests to enable / disable a device for PCI I/O Address > Translation assistance. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > arch/s390/include/asm/kvm_pci.h | 15 ++++ > arch/s390/include/asm/pci_dma.h | 2 + > arch/s390/kvm/pci.c | 139 ++++++++++++++++++++++++++++++++ > arch/s390/kvm/pci.h | 2 + > 4 files changed, 158 insertions(+) > > diff --git a/arch/s390/include/asm/kvm_pci.h b/arch/s390/include/asm/kvm_pci.h > index 01fe14fffd7a..770849f13a70 100644 > --- a/arch/s390/include/asm/kvm_pci.h > +++ b/arch/s390/include/asm/kvm_pci.h > @@ -16,11 +16,21 @@ > #include <linux/kvm_host.h> > #include <linux/kvm.h> > #include <linux/pci.h> > +#include <linux/mutex.h> > #include <asm/pci_insn.h> > +#include <asm/pci_dma.h> > + > +struct kvm_zdev_ioat { > + unsigned long *head[ZPCI_TABLE_PAGES]; > + unsigned long **seg; > + unsigned long ***pt; > + struct mutex lock;
Can we please rename the mutex ioat_lock to have a unique name easy to follow for maintenance. Can you please add a description about when the lock should be used?
> +}; > > struct kvm_zdev { > struct zpci_dev *zdev; > struct kvm *kvm; > + struct kvm_zdev_ioat ioat; > struct zpci_fib fib; > }; > > @@ -33,6 +43,11 @@ int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib, > bool assist); > int kvm_s390_pci_aif_disable(struct zpci_dev *zdev); > > +int kvm_s390_pci_ioat_probe(struct zpci_dev *zdev); > +int kvm_s390_pci_ioat_enable(struct zpci_dev *zdev, u64 iota); > +int kvm_s390_pci_ioat_disable(struct zpci_dev *zdev); > +u8 kvm_s390_pci_get_dtsm(struct zpci_dev *zdev); > + > int kvm_s390_pci_interp_probe(struct zpci_dev *zdev); > int kvm_s390_pci_interp_enable(struct zpci_dev *zdev); > int kvm_s390_pci_interp_disable(struct zpci_dev *zdev); > diff --git a/arch/s390/include/asm/pci_dma.h b/arch/s390/include/asm/pci_dma.h > index 91e63426bdc5..69e616d0712c 100644 > --- a/arch/s390/include/asm/pci_dma.h > +++ b/arch/s390/include/asm/pci_dma.h > @@ -50,6 +50,8 @@ enum zpci_ioat_dtype { > #define ZPCI_TABLE_ALIGN ZPCI_TABLE_SIZE > #define ZPCI_TABLE_ENTRY_SIZE (sizeof(unsigned long)) > #define ZPCI_TABLE_ENTRIES (ZPCI_TABLE_SIZE / ZPCI_TABLE_ENTRY_SIZE) > +#define ZPCI_TABLE_PAGES (ZPCI_TABLE_SIZE >> PAGE_SHIFT) > +#define ZPCI_TABLE_ENTRIES_PAGES (ZPCI_TABLE_ENTRIES * ZPCI_TABLE_PAGES) > > #define ZPCI_TABLE_BITS 11 > #define ZPCI_PT_BITS 8 > diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c > index 7ed9abc476b6..39c13c25a700 100644 > --- a/arch/s390/kvm/pci.c > +++ b/arch/s390/kvm/pci.c > @@ -13,12 +13,15 @@ > #include <asm/pci.h> > #include <asm/pci_insn.h> > #include <asm/pci_io.h> > +#include <asm/pci_dma.h> > #include <asm/sclp.h> > #include "pci.h" > #include "kvm-s390.h" > > struct zpci_aift *aift; > > +#define shadow_ioat_init zdev->kzdev->ioat.head[0] > + > static inline int __set_irq_noiib(u16 ctl, u8 isc) > { > union zpci_sic_iib iib = {{0}}; > @@ -344,6 +347,135 @@ int kvm_s390_pci_aif_disable(struct zpci_dev *zdev) > } > EXPORT_SYMBOL_GPL(kvm_s390_pci_aif_disable); > > +int kvm_s390_pci_ioat_probe(struct zpci_dev *zdev) > +{ > + /* Must have a KVM association registered */
may be add something like : "The ioat structure is embeded in kzdev"
> + if (!zdev->kzdev || !zdev->kzdev->kvm)
Why do we need to check for kvm ? Having kzdev is already tested by the unique caller.
> + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(kvm_s390_pci_ioat_probe); > + > +int kvm_s390_pci_ioat_enable(struct zpci_dev *zdev, u64 iota) > +{ > + gpa_t gpa = (gpa_t)(iota & ZPCI_RTE_ADDR_MASK); > + struct kvm_zdev_ioat *ioat; > + struct page *page; > + struct kvm *kvm; > + unsigned int idx; > + void *iaddr; > + int i, rc = 0;
no need to initialize rc
> + > + if (shadow_ioat_init) > + return -EINVAL; > + > + /* Ensure supported type specified */ > + if ((iota & ZPCI_IOTA_RTTO_FLAG) != ZPCI_IOTA_RTTO_FLAG) > + return -EINVAL; > + > + kvm = zdev->kzdev->kvm; > + ioat = &zdev->kzdev->ioat; > + mutex_lock(&ioat->lock); > + idx = srcu_read_lock(&kvm->srcu); > + for (i = 0; i < ZPCI_TABLE_PAGES; i++) { > + page = gfn_to_page(kvm, gpa_to_gfn(gpa)); > + if (is_error_page(page)) { > + srcu_read_unlock(&kvm->srcu, idx); > + rc = -EIO; > + goto out;
goto unpin ?
> + } > + iaddr = page_to_virt(page) + (gpa & ~PAGE_MASK); > + ioat->head[i] = (unsigned long *)iaddr; > + gpa += PAGE_SIZE; > + } > + srcu_read_unlock(&kvm->srcu, idx); > + > + zdev->kzdev->ioat.seg = kcalloc(ZPCI_TABLE_ENTRIES_PAGES, > + sizeof(unsigned long *), GFP_KERNEL);
What about:
ioat->seg = kcalloc(ZPCI_TABLE_ENTRIES_PAGES, sizeof(*ioat->seg), GFP_KERNEL); if (!ioat->seg) ... ioat->pt = ... ?
> + if (!zdev->kzdev->ioat.seg) > + goto unpin; > + zdev->kzdev->ioat.pt = kcalloc(ZPCI_TABLE_ENTRIES, > + sizeof(unsigned long **), GFP_KERNEL); > + if (!zdev->kzdev->ioat.pt) > + goto free_seg; > + > +out: > + mutex_unlock(&ioat->lock); > + return rc;
return 0 ?
> + > +free_seg: > + kfree(zdev->kzdev->ioat.seg);
kfree(ioat->seg) ? rc = -ENOMEM;
> +unpin: > + for (i = 0; i < ZPCI_TABLE_PAGES; i++) { > + kvm_release_pfn_dirty((u64)ioat->head[i] >> PAGE_SHIFT); > + ioat->head[i] = 0; > + } > + mutex_unlock(&ioat->lock); > + return -ENOMEM;
return rc;
> +} ...snip...
-- Pierre Morel IBM Lab Boeblingen
| |