| Message ID | 20200320161911.27494-6-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 Nested Stage Setup (VFIO part) | expand |
Hi Eric, On 2020/3/21 0:19, Eric Auger wrote: > Register an IOMMU fault handler which records faults in > the DMA FAULT region ring buffer. In a subsequent patch, we > will add the signaling of a specific eventfd to allow the > userspace to be notified whenever a new fault as shown up. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> [...] > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 586b89debed5..69595c240baf 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -27,6 +27,7 @@ > #include <linux/vfio.h> > #include <linux/vgaarb.h> > #include <linux/nospec.h> > +#include <linux/circ_buf.h> > > #include "vfio_pci_private.h" > > @@ -283,6 +284,38 @@ static const struct vfio_pci_regops vfio_pci_dma_fault_regops = { > .add_capability = vfio_pci_dma_fault_add_capability, > }; > > +int vfio_pci_iommu_dev_fault_handler(struct iommu_fault *fault, void *data) > +{ > + struct vfio_pci_device *vdev = (struct vfio_pci_device *)data; > + struct vfio_region_dma_fault *reg = > + (struct vfio_region_dma_fault *)vdev->fault_pages; > + struct iommu_fault *new = > + (struct iommu_fault *)(vdev->fault_pages + reg->offset + > + reg->head * reg->entry_size); Shouldn't 'reg->head' be protected under the fault_queue_lock? Otherwise things may change behind our backs... We shouldn't take any assumption about how IOMMU driver would report the fault (serially or in parallel), I think. > + int head, tail, size; > + int ret = 0; > + > + if (fault->type != IOMMU_FAULT_DMA_UNRECOV) > + return -ENOENT; > + > + mutex_lock(&vdev->fault_queue_lock); > + > + head = reg->head; > + tail = reg->tail; > + size = reg->nb_entries; > + > + if (CIRC_SPACE(head, tail, size) < 1) { > + ret = -ENOSPC; > + goto unlock; > + } > + > + *new = *fault; > + reg->head = (head + 1) % size; > +unlock: > + mutex_unlock(&vdev->fault_queue_lock); > + return ret; > +} > + > #define DMA_FAULT_RING_LENGTH 512 > > static int vfio_pci_init_dma_fault_region(struct vfio_pci_device *vdev) > @@ -317,6 +350,13 @@ static int vfio_pci_init_dma_fault_region(struct vfio_pci_device *vdev) > header->entry_size = sizeof(struct iommu_fault); > header->nb_entries = DMA_FAULT_RING_LENGTH; > header->offset = sizeof(struct vfio_region_dma_fault); > + > + ret = iommu_register_device_fault_handler(&vdev->pdev->dev, > + vfio_pci_iommu_dev_fault_handler, > + vdev); > + if (ret) > + goto out; > + > return 0; > out: > kfree(vdev->fault_pages); Thanks, Zenghui
Hi Zenghui, On 9/24/20 10:49 AM, Zenghui Yu wrote: > Hi Eric, > > On 2020/3/21 0:19, Eric Auger wrote: >> Register an IOMMU fault handler which records faults in >> the DMA FAULT region ring buffer. In a subsequent patch, we >> will add the signaling of a specific eventfd to allow the >> userspace to be notified whenever a new fault as shown up. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > > [...] > >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index 586b89debed5..69595c240baf 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -27,6 +27,7 @@ >> #include <linux/vfio.h> >> #include <linux/vgaarb.h> >> #include <linux/nospec.h> >> +#include <linux/circ_buf.h> >> #include "vfio_pci_private.h" >> @@ -283,6 +284,38 @@ static const struct vfio_pci_regops >> vfio_pci_dma_fault_regops = { >> .add_capability = vfio_pci_dma_fault_add_capability, >> }; >> +int vfio_pci_iommu_dev_fault_handler(struct iommu_fault *fault, >> void *data) >> +{ >> + struct vfio_pci_device *vdev = (struct vfio_pci_device *)data; >> + struct vfio_region_dma_fault *reg = >> + (struct vfio_region_dma_fault *)vdev->fault_pages; >> + struct iommu_fault *new = >> + (struct iommu_fault *)(vdev->fault_pages + reg->offset + >> + reg->head * reg->entry_size); > > Shouldn't 'reg->head' be protected under the fault_queue_lock? Otherwise > things may change behind our backs...> > We shouldn't take any assumption about how IOMMU driver would report the > fault (serially or in parallel), I think. Yes I modified the locking Thanks Eric > >> + int head, tail, size; >> + int ret = 0; >> + >> + if (fault->type != IOMMU_FAULT_DMA_UNRECOV) >> + return -ENOENT; >> + >> + mutex_lock(&vdev->fault_queue_lock); >> + >> + head = reg->head; >> + tail = reg->tail; >> + size = reg->nb_entries; >> + >> + if (CIRC_SPACE(head, tail, size) < 1) { >> + ret = -ENOSPC; >> + goto unlock; >> + } >> + >> + *new = *fault; >> + reg->head = (head + 1) % size; >> +unlock: >> + mutex_unlock(&vdev->fault_queue_lock); >> + return ret; >> +} >> + >> #define DMA_FAULT_RING_LENGTH 512 >> static int vfio_pci_init_dma_fault_region(struct vfio_pci_device >> *vdev) >> @@ -317,6 +350,13 @@ static int vfio_pci_init_dma_fault_region(struct >> vfio_pci_device *vdev) >> header->entry_size = sizeof(struct iommu_fault); >> header->nb_entries = DMA_FAULT_RING_LENGTH; >> header->offset = sizeof(struct vfio_region_dma_fault); >> + >> + ret = iommu_register_device_fault_handler(&vdev->pdev->dev, >> + vfio_pci_iommu_dev_fault_handler, >> + vdev); >> + if (ret) >> + goto out; >> + >> return 0; >> out: >> kfree(vdev->fault_pages); > > > Thanks, > Zenghui >
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 586b89debed5..69595c240baf 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -27,6 +27,7 @@ #include <linux/vfio.h> #include <linux/vgaarb.h> #include <linux/nospec.h> +#include <linux/circ_buf.h> #include "vfio_pci_private.h" @@ -283,6 +284,38 @@ static const struct vfio_pci_regops vfio_pci_dma_fault_regops = { .add_capability = vfio_pci_dma_fault_add_capability, }; +int vfio_pci_iommu_dev_fault_handler(struct iommu_fault *fault, void *data) +{ + struct vfio_pci_device *vdev = (struct vfio_pci_device *)data; + struct vfio_region_dma_fault *reg = + (struct vfio_region_dma_fault *)vdev->fault_pages; + struct iommu_fault *new = + (struct iommu_fault *)(vdev->fault_pages + reg->offset + + reg->head * reg->entry_size); + int head, tail, size; + int ret = 0; + + if (fault->type != IOMMU_FAULT_DMA_UNRECOV) + return -ENOENT; + + mutex_lock(&vdev->fault_queue_lock); + + head = reg->head; + tail = reg->tail; + size = reg->nb_entries; + + if (CIRC_SPACE(head, tail, size) < 1) { + ret = -ENOSPC; + goto unlock; + } + + *new = *fault; + reg->head = (head + 1) % size; +unlock: + mutex_unlock(&vdev->fault_queue_lock); + return ret; +} + #define DMA_FAULT_RING_LENGTH 512 static int vfio_pci_init_dma_fault_region(struct vfio_pci_device *vdev) @@ -317,6 +350,13 @@ static int vfio_pci_init_dma_fault_region(struct vfio_pci_device *vdev) header->entry_size = sizeof(struct iommu_fault); header->nb_entries = DMA_FAULT_RING_LENGTH; header->offset = sizeof(struct vfio_region_dma_fault); + + ret = iommu_register_device_fault_handler(&vdev->pdev->dev, + vfio_pci_iommu_dev_fault_handler, + vdev); + if (ret) + goto out; + return 0; out: kfree(vdev->fault_pages); @@ -542,6 +582,8 @@ static void vfio_pci_release(void *device_data) if (!(--vdev->refcnt)) { vfio_spapr_pci_eeh_release(vdev->pdev); vfio_pci_disable(vdev); + /* TODO: Failure problematics */ + iommu_unregister_device_fault_handler(&vdev->pdev->dev); } mutex_unlock(&vdev->reflck->lock);
Register an IOMMU fault handler which records faults in the DMA FAULT region ring buffer. In a subsequent patch, we will add the signaling of a specific eventfd to allow the userspace to be notified whenever a new fault as shown up. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v8 -> v9: - handler now takes an iommu_fault handle - eventfd signaling moved to a subsequent patch - check the fault type and return an error if != UNRECOV - still the fault handler registration can fail. We need to reach an agreement about how to deal with the situation v3 -> v4: - move iommu_unregister_device_fault_handler to vfio_pci_release --- drivers/vfio/pci/vfio_pci.c | 42 +++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)