Re: [PATCH v11 12/13] vfio/pci: Register a DMA fault response region

[Auger Eric]

Hi Shameer,

On 2/18/21 11:36 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
> 
>>> -----Original Message-----
>>> From: Eric Auger [mailto:eric.auger@xxxxxxxxxx]
>>> Sent: 16 November 2020 11:00
>>> To: eric.auger.pro@xxxxxxxxx; eric.auger@xxxxxxxxxx;
>>> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>> kvm@xxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; will@xxxxxxxxxx;
>>> joro@xxxxxxxxxx; maz@xxxxxxxxxx; robin.murphy@xxxxxxx;
>>> alex.williamson@xxxxxxxxxx
>>> Cc: jean-philippe@xxxxxxxxxx; zhangfei.gao@xxxxxxxxxx;
>>> zhangfei.gao@xxxxxxxxx; vivek.gautam@xxxxxxx; Shameerali Kolothum
>>> Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
>>> jacob.jun.pan@xxxxxxxxxxxxxxx; yi.l.liu@xxxxxxxxx; tn@xxxxxxxxxxxx;
>>> nicoleotsuka@xxxxxxxxx; yuzenghui <yuzenghui@xxxxxxxxxx>
>>> Subject: [PATCH v11 12/13] vfio/pci: Register a DMA fault response
>>> region
>>>
>>> In preparation for vSVA, let's register a DMA fault response region,
>>> where the userspace will push the page responses and increment the
>>> head of the buffer. The kernel will pop those responses and inject
>>> them on iommu side.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>> ---
>>>  drivers/vfio/pci/vfio_pci.c         | 114 +++++++++++++++++++++++++---
>>>  drivers/vfio/pci/vfio_pci_private.h |   5 ++
>>>  drivers/vfio/pci/vfio_pci_rdwr.c    |  39 ++++++++++
>>>  include/uapi/linux/vfio.h           |  32 ++++++++
>>>  4 files changed, 181 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>> index 65a83fd0e8c0..e9a904ce3f0d 100644
>>> --- a/drivers/vfio/pci/vfio_pci.c
>>> +++ b/drivers/vfio/pci/vfio_pci.c
>>> @@ -318,9 +318,20 @@ static void vfio_pci_dma_fault_release(struct
>>> vfio_pci_device *vdev,
>>>  	kfree(vdev->fault_pages);
>>>  }
>>>
>>> -static int vfio_pci_dma_fault_mmap(struct vfio_pci_device *vdev,
>>> -				   struct vfio_pci_region *region,
>>> -				   struct vm_area_struct *vma)
>>> +static void
>>> +vfio_pci_dma_fault_response_release(struct vfio_pci_device *vdev,
>>> +				    struct vfio_pci_region *region) {
>>> +	if (vdev->dma_fault_response_wq)
>>> +		destroy_workqueue(vdev->dma_fault_response_wq);
>>> +	kfree(vdev->fault_response_pages);
>>> +	vdev->fault_response_pages = NULL;
>>> +}
>>> +
>>> +static int __vfio_pci_dma_fault_mmap(struct vfio_pci_device *vdev,
>>> +				     struct vfio_pci_region *region,
>>> +				     struct vm_area_struct *vma,
>>> +				     u8 *pages)
>>>  {
>>>  	u64 phys_len, req_len, pgoff, req_start;
>>>  	unsigned long long addr;
>>> @@ -333,14 +344,14 @@ static int vfio_pci_dma_fault_mmap(struct
>>> vfio_pci_device *vdev,
>>>  		((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
>>>  	req_start = pgoff << PAGE_SHIFT;
>>>
>>> -	/* only the second page of the producer fault region is mmappable */
>>> +	/* only the second page of the fault region is mmappable */
>>>  	if (req_start < PAGE_SIZE)
>>>  		return -EINVAL;
>>>
>>>  	if (req_start + req_len > phys_len)
>>>  		return -EINVAL;
>>>
>>> -	addr = virt_to_phys(vdev->fault_pages);
>>> +	addr = virt_to_phys(pages);
>>>  	vma->vm_private_data = vdev;
>>>  	vma->vm_pgoff = (addr >> PAGE_SHIFT) + pgoff;
>>>
>>> @@ -349,13 +360,29 @@ static int vfio_pci_dma_fault_mmap(struct
>>> vfio_pci_device *vdev,
>>>  	return ret;
>>>  }
>>>
>>> -static int vfio_pci_dma_fault_add_capability(struct vfio_pci_device *vdev,
>>> -					     struct vfio_pci_region *region,
>>> -					     struct vfio_info_cap *caps)
>>> +static int vfio_pci_dma_fault_mmap(struct vfio_pci_device *vdev,
>>> +				   struct vfio_pci_region *region,
>>> +				   struct vm_area_struct *vma)
>>> +{
>>> +	return __vfio_pci_dma_fault_mmap(vdev, region, vma,
>>> vdev->fault_pages);
>>> +}
>>> +
>>> +static int
>>> +vfio_pci_dma_fault_response_mmap(struct vfio_pci_device *vdev,
>>> +				struct vfio_pci_region *region,
>>> +				struct vm_area_struct *vma)
>>> +{
>>> +	return __vfio_pci_dma_fault_mmap(vdev, region, vma,
>>> vdev->fault_response_pages);
>>> +}
>>> +
>>> +static int __vfio_pci_dma_fault_add_capability(struct vfio_pci_device *vdev,
>>> +					       struct vfio_pci_region *region,
>>> +					       struct vfio_info_cap *caps,
>>> +					       u32 cap_id)
>>>  {
>>>  	struct vfio_region_info_cap_sparse_mmap *sparse = NULL;
>>>  	struct vfio_region_info_cap_fault cap = {
>>> -		.header.id = VFIO_REGION_INFO_CAP_DMA_FAULT,
>>> +		.header.id = cap_id,
>>>  		.header.version = 1,
>>>  		.version = 1,
>>>  	};
>>> @@ -383,6 +410,14 @@ static int
>>> vfio_pci_dma_fault_add_capability(struct
>>> vfio_pci_device *vdev,
>>>  	return ret;
>>>  }
>>>
>>> +static int vfio_pci_dma_fault_add_capability(struct vfio_pci_device *vdev,
>>> +					     struct vfio_pci_region *region,
>>> +					     struct vfio_info_cap *caps) {
>>> +	return __vfio_pci_dma_fault_add_capability(vdev, region, caps,
>>> +						   VFIO_REGION_INFO_CAP_DMA_FAULT); }
>>> +
>>>  static const struct vfio_pci_regops vfio_pci_dma_fault_regops = {
>>>  	.rw		= vfio_pci_dma_fault_rw,
>>>  	.release	= vfio_pci_dma_fault_release,
>>> @@ -390,6 +425,13 @@ static const struct vfio_pci_regops
>>> vfio_pci_dma_fault_regops = {
>>>  	.add_capability = vfio_pci_dma_fault_add_capability,
>>>  };
>>>
>>> +static const struct vfio_pci_regops vfio_pci_dma_fault_response_regops = {
>>> +	.rw		= vfio_pci_dma_fault_response_rw,
>>> +	.release	= vfio_pci_dma_fault_response_release,
>>> +	.mmap		= vfio_pci_dma_fault_response_mmap,
>>> +	.add_capability = vfio_pci_dma_fault_add_capability,
> 
> As I mentioned in the Qemu patch ([RFC v7 26/26] vfio/pci: Implement 
> return_page_response page response callback), it looks like we are using the
> VFIO_REGION_INFO_CAP_DMA_FAULT cap id for the dma_fault_response here
> as well. Is that intentional?
> (Was wondering how it worked in the first place and noted this).
yep, copy paste error :-(

Thanks

Eric
> 
> Please check.
> 
> Thanks,
> Shameer
>