Re: [PATCH v10 05/11] vfio/pci: Register an iommu fault handler

[Zenghui Yu <yuzenghui@huawei.com>]

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