Re: [RFC v7 26/26] vfio/pci: Implement return_page_response page response callback

[Auger Eric]

Hi Shameer,

On 2/18/21 11:19 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
> 
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: 16 November 2020 18:14
>> To: eric.auger.pro@gmail.com; eric.auger@redhat.com;
>> qemu-devel@nongnu.org; qemu-arm@nongnu.org;
>> alex.williamson@redhat.com
>> Cc: peter.maydell@linaro.org; jean-philippe@linaro.org; peterx@redhat.com;
>> jacob.jun.pan@linux.intel.com; yi.l.liu@intel.com; Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; tn@semihalf.com;
>> nicoleotsuka@gmail.com; yuzenghui <yuzenghui@huawei.com>;
>> zhangfei.gao@gmail.com; vivek.gautam@arm.com
>> Subject: [RFC v7 26/26] vfio/pci: Implement return_page_response page
>> response callback
>>
>> This patch implements the page response path. The
>> response s written into the page response ring buffer and then
>> update header's head index is updated. This path is not used
>> by this series. It is introduced here as a POC for vSVA/ARM
>> integration.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/pci.h |   2 +
>>  hw/vfio/pci.c | 121
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 123 insertions(+)
>>
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 350e9e9005..ce0472611e 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -147,6 +147,8 @@ struct VFIOPCIDevice {
>>      VFIOPCIExtIRQ *ext_irqs;
>>      VFIORegion dma_fault_region;
>>      uint32_t fault_tail_index;
>> +    VFIORegion dma_fault_response_region;
>> +    uint32_t fault_response_head_index;
>>      int (*resetfn)(struct VFIOPCIDevice *);
>>      uint32_t vendor_id;
>>      uint32_t device_id;
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4e3495bb60..797acd9c73 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2631,6 +2631,61 @@ out:
>>      g_free(fault_region_info);
>>  }
>>
>> +static void vfio_init_fault_response_regions(VFIOPCIDevice *vdev, Error
>> **errp)
>> +{
>> +    struct vfio_region_info *fault_region_info = NULL;
>> +    struct vfio_region_info_cap_fault *cap_fault;
>> +    VFIODevice *vbasedev = &vdev->vbasedev;
>> +    struct vfio_info_cap_header *hdr;
>> +    char *fault_region_name;
>> +    int ret;
>> +
>> +    ret = vfio_get_dev_region_info(&vdev->vbasedev,
>> +                                   VFIO_REGION_TYPE_NESTED,
>> +
>> VFIO_REGION_SUBTYPE_NESTED_DMA_FAULT_RESPONSE,
>> +                                   &fault_region_info);
>> +    if (ret) {
>> +        goto out;
>> +    }
>> +
>> +    hdr = vfio_get_region_info_cap(fault_region_info,
>> +
>> VFIO_REGION_INFO_CAP_DMA_FAULT);
> 
> VFIO_REGION_INFO_CAP_DMA_FAULT_RESPONSE ? 
yes!
> 
>> +    if (!hdr) {
>> +        error_setg(errp, "failed to retrieve DMA FAULT RESPONSE
>> capability");
>> +        goto out;
>> +    }
>> +    cap_fault = container_of(hdr, struct vfio_region_info_cap_fault,
>> +                             header);
>> +    if (cap_fault->version != 1) {
>> +        error_setg(errp, "Unsupported DMA FAULT RESPONSE API
>> version %d",
>> +                   cap_fault->version);
>> +        goto out;
>> +    }
>> +
>> +    fault_region_name = g_strdup_printf("%s DMA FAULT RESPONSE %d",
>> +                                        vbasedev->name,
>> +                                        fault_region_info->index);
>> +
>> +    ret = vfio_region_setup(OBJECT(vdev), vbasedev,
>> +                            &vdev->dma_fault_response_region,
>> +                            fault_region_info->index,
>> +                            fault_region_name);
>> +    g_free(fault_region_name);
>> +    if (ret) {
>> +        error_setg_errno(errp, -ret,
>> +                         "failed to set up the DMA FAULT RESPONSE
>> region %d",
>> +                         fault_region_info->index);
>> +        goto out;
>> +    }
>> +
>> +    ret = vfio_region_mmap(&vdev->dma_fault_response_region);
>> +    if (ret) {
>> +        error_setg_errno(errp, -ret, "Failed to mmap the DMA FAULT
>> RESPONSE queue");
>> +    }
>> +out:
>> +    g_free(fault_region_info);
>> +}
>> +
>>  static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>      VFIODevice *vbasedev = &vdev->vbasedev;
>> @@ -2706,6 +2761,12 @@ static void vfio_populate_device(VFIOPCIDevice
>> *vdev, Error **errp)
>>          return;
>>      }
>>
>> +    vfio_init_fault_response_regions(vdev, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>>      irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;
>>
>>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>> @@ -2884,8 +2945,68 @@ static int vfio_iommu_set_pasid_table(PCIBus
>> *bus, int32_t devfn,
>>      return ioctl(container->fd, VFIO_IOMMU_SET_PASID_TABLE, &info);
>>  }
>>
>> +static int vfio_iommu_return_page_response(PCIBus *bus, int32_t devfn,
>> +                                           IOMMUPageResponse
>> *resp)
>> +{
>> +    PCIDevice *pdev = bus->devices[devfn];
>> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +    struct iommu_page_response *response = &resp->resp;
>> +    struct vfio_region_dma_fault_response header;
>> +    struct iommu_page_response *queue;
>> +    char *queue_buffer = NULL;
>> +    ssize_t bytes;
>> +
>> +    if (!vdev->dma_fault_response_region.mem) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* read the header */
>> +    bytes = pread(vdev->vbasedev.fd, &header, sizeof(header),
>> +                  vdev->dma_fault_response_region.fd_offset);
>> +    if (bytes != sizeof(header)) {
>> +        error_report("%s unable to read the fault region header (0x%lx)",
>> +                     __func__, bytes);
>> +        return -1;
>> +    }
>> +
>> +    /* Normally the fault queue is mmapped */
>> +    queue = (struct iommu_page_response
>> *)vdev->dma_fault_response_region.mmaps[0].mmap;
>> +    if (!queue) {
>> +        size_t queue_size = header.nb_entries * header.entry_size;
>> +
>> +        error_report("%s: fault queue not mmapped: slower fault handling",
>> +                     vdev->vbasedev.name);
>> +
>> +        queue_buffer = g_malloc(queue_size);
>> +        bytes = pread(vdev->vbasedev.fd, queue_buffer, queue_size,
>> +                      vdev->dma_fault_response_region.fd_offset +
>> header.offset);
>> +        if (bytes != queue_size) {
>> +            error_report("%s unable to read the fault queue (0x%lx)",
>> +                         __func__, bytes);
>> +            return -1;
>> +        }
>> +
>> +        queue = (struct iommu_page_response *)queue_buffer;
>> +    }
>> +    /* deposit the new response in the queue and increment the head */
>> +    memcpy(queue + header.head, response, header.entry_size);
>> +
>> +    vdev->fault_response_head_index =
>> +        (vdev->fault_response_head_index + 1) % header.nb_entries;
>> +    bytes = pwrite(vdev->vbasedev.fd, &vdev->fault_response_head_index,
>> 4,
>> +                   vdev->dma_fault_response_region.fd_offset);
>> +    if (bytes != 4) {
>> +        error_report("%s unable to write the fault response region head
>> index (0x%lx)",
>> +                     __func__, bytes);
>> +    }
>> +    g_free(queue_buffer);
>> +
>> +    return 0;
>> +}
>> +
>>  static PCIPASIDOps vfio_pci_pasid_ops = {
>>      .set_pasid_table = vfio_iommu_set_pasid_table,
>> +    .return_page_response = vfio_iommu_return_page_response,
>>  };
>>
>>  static void vfio_dma_fault_notifier_handler(void *opaque)
> 
> Also, I just noted that this patch breaks the dev hot add/del functionality.
> device_add works fine but device_del is not removing the dev cleanly.Thank 
> you for reporting this!

The test matrix becomes bigger and bigger :-( I Need to write some
avocado-vt tests or alike.

I am currently working on the respin. At the moment I investigate the
DPDK issue that you reported and I was able to reproduce.

I intend to rebase on top of Jean-Philippe's
[PATCH v12 00/10] iommu: I/O page faults for SMMUv3

Is that good enough for your SVA integration or do you want I prepare a
rebase on some extended code?

Thanks

Eric
> 
> The below one fixes it. Please check.
> 
> Thanks,
> Shameer
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 797acd9c73..92c1d48316 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3470,6 +3470,7 @@ static void vfio_instance_finalize(Object *obj)
>      vfio_display_finalize(vdev);
>      vfio_bars_finalize(vdev);
>      vfio_region_finalize(&vdev->dma_fault_region);
> +    vfio_region_finalize(&vdev->dma_fault_response_region);
>      g_free(vdev->emulated_config_bits);
>      g_free(vdev->rom);
>      /*
> @@ -3491,6 +3492,7 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_unregister_err_notifier(vdev);
>      vfio_unregister_ext_irq_notifiers(vdev);
>      vfio_region_exit(&vdev->dma_fault_region);
> +    vfio_region_exit(&vdev->dma_fault_response_region);
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      if (vdev->irqchip_change_notifier.notify) {
>          kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_not
> 
> 
>