[RFC,v1,05/18] vfio/pci: add pasid alloc/free implementation

Commit Message

Yi Liu July 5, 2019, 11:01 a.m. UTC

This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
These two functions are used to propagate guest pasid allocation and
free requests to host via vfio container ioctl.

Cc: Kevin Tian <[e-mail]>
Cc: Jacob Pan <[e-mail]>
Cc: Peter Xu <[e-mail]>
Cc: Eric Auger <[e-mail]>
Cc: Yi Sun <[e-mail]>
Cc: David Gibson <[e-mail]>
Signed-off-by: Liu Yi L <[e-mail]>
Signed-off-by: Yi Sun <[e-mail]>
---
 hw/vfio/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Peter Xu July 9, 2019, 2:23 a.m. UTC | #1

On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> These two functions are used to propagate guest pasid allocation and
> free requests to host via vfio container ioctl.
> 
> Cc: Kevin Tian <[e-mail]>
> Cc: Jacob Pan <[e-mail]>
> Cc: Peter Xu <[e-mail]>
> Cc: Eric Auger <[e-mail]>
> Cc: Yi Sun <[e-mail]>
> Cc: David Gibson <[e-mail]>
> Signed-off-by: Liu Yi L <[e-mail]>
> Signed-off-by: Yi Sun <[e-mail]>
> ---
>  hw/vfio/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ce3fe96..ab184ad 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2690,6 +2690,65 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>      vdev->req_enabled = false;
>  }
>  
> +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> +                                               int32_t devfn,
> +                                               uint32_t min_pasid,
> +                                               uint32_t max_pasid)
> +{
> +    PCIDevice *pdev = bus->devices[devfn];
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    VFIOContainer *container = vdev->vbasedev.group->container;
> +    struct vfio_iommu_type1_pasid_request req;
> +    unsigned long argsz;
> +    int pasid;
> +
> +    argsz = sizeof(req);
> +    req.argsz = argsz;
> +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> +    req.min_pasid = min_pasid;
> +    req.max_pasid = max_pasid;
> +
> +    rcu_read_lock();

Could I ask what's this RCU lock protecting?

> +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> +    if (pasid < 0) {
> +        error_report("vfio_pci_device_request_pasid_alloc:"
> +                     " request failed, contanier: %p", container);

Can use __func__, also since we're going to dump the error after all,
we can also include the errno (pasid) here which seems to be more
helpful than the container pointer at least to me. :)

> +    }
> +    rcu_read_unlock();
> +    return pasid;
> +}
> +
> +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> +                                              int32_t devfn,
> +                                              uint32_t pasid)
> +{
> +    PCIDevice *pdev = bus->devices[devfn];
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    VFIOContainer *container = vdev->vbasedev.group->container;
> +    struct vfio_iommu_type1_pasid_request req;
> +    unsigned long argsz;
> +    int ret = 0;
> +
> +    argsz = sizeof(req);
> +    req.argsz = argsz;
> +    req.flag = VFIO_IOMMU_PASID_FREE;
> +    req.pasid = pasid;
> +
> +    rcu_read_lock();
> +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> +    if (ret != 0) {
> +        error_report("vfio_pci_device_request_pasid_free:"
> +                     " request failed, contanier: %p", container);
> +    }
> +    rcu_read_unlock();
> +    return ret;
> +}
> +
> +static PCIPASIDOps vfio_pci_pasid_ops = {
> +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> +    .free_pasid = vfio_pci_device_request_pasid_free,
> +};
> +
>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      vfio_register_req_notifier(vdev);
>      vfio_setup_resetfn_quirk(vdev);
>  
> +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> +
>      return;
>  
>  out_teardown:
> -- 
> 2.7.4
> 

Regards,

Yi Liu July 10, 2019, 12:16 p.m. UTC | #2

> From: Peter Xu [mailto:[e-mail]]
> Sent: Tuesday, July 9, 2019 10:24 AM
> To: Liu, Yi L <[e-mail]>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> > These two functions are used to propagate guest pasid allocation and
> > free requests to host via vfio container ioctl.
> >
> > Cc: Kevin Tian <[e-mail]>
> > Cc: Jacob Pan <[e-mail]>
> > Cc: Peter Xu <[e-mail]>
> > Cc: Eric Auger <[e-mail]>
> > Cc: Yi Sun <[e-mail]>
> > Cc: David Gibson <[e-mail]>
> > Signed-off-by: Liu Yi L <[e-mail]>
> > Signed-off-by: Yi Sun <[e-mail]>
> > ---
> >  hw/vfio/pci.c | 61
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index ce3fe96..ab184ad 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2690,6 +2690,65 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice
> *vdev)
> >      vdev->req_enabled = false;
> >  }
> >
> > +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> > +                                               int32_t devfn,
> > +                                               uint32_t min_pasid,
> > +                                               uint32_t max_pasid)
> > +{
> > +    PCIDevice *pdev = bus->devices[devfn];
> > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > +    struct vfio_iommu_type1_pasid_request req;
> > +    unsigned long argsz;
> > +    int pasid;
> > +
> > +    argsz = sizeof(req);
> > +    req.argsz = argsz;
> > +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> > +    req.min_pasid = min_pasid;
> > +    req.max_pasid = max_pasid;
> > +
> > +    rcu_read_lock();
> 
> Could I ask what's this RCU lock protecting?

good catch, let me remove it.

> 
> > +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > +    if (pasid < 0) {
> > +        error_report("vfio_pci_device_request_pasid_alloc:"
> > +                     " request failed, contanier: %p", container);
> 
> Can use __func__, also since we're going to dump the error after all,
> we can also include the errno (pasid) here which seems to be more
> helpful than the container pointer at least to me. :)

accepted, thanks.

> > +    }
> > +    rcu_read_unlock();
> > +    return pasid;
> > +}
> > +
> > +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> > +                                              int32_t devfn,
> > +                                              uint32_t pasid)
> > +{
> > +    PCIDevice *pdev = bus->devices[devfn];
> > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > +    struct vfio_iommu_type1_pasid_request req;
> > +    unsigned long argsz;
> > +    int ret = 0;
> > +
> > +    argsz = sizeof(req);
> > +    req.argsz = argsz;
> > +    req.flag = VFIO_IOMMU_PASID_FREE;
> > +    req.pasid = pasid;
> > +
> > +    rcu_read_lock();
> > +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > +    if (ret != 0) {
> > +        error_report("vfio_pci_device_request_pasid_free:"
> > +                     " request failed, contanier: %p", container);
> > +    }
> > +    rcu_read_unlock();
> > +    return ret;
> > +}
> > +
> > +static PCIPASIDOps vfio_pci_pasid_ops = {
> > +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> > +    .free_pasid = vfio_pci_device_request_pasid_free,
> > +};
> > +
> >  static void vfio_realize(PCIDevice *pdev, Error **errp)
> >  {
> >      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >      vfio_register_req_notifier(vdev);
> >      vfio_setup_resetfn_quirk(vdev);
> >
> > +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> > +
> >      return;
> >
> >  out_teardown:
> > --
> > 2.7.4
> >
> 
> Regards,
> 
> --
> Peter Xu

Thanks,
Yi Liu

David Gibson July 15, 2019, 2:55 a.m. UTC | #3

On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> These two functions are used to propagate guest pasid allocation and
> free requests to host via vfio container ioctl.

As I said in an earlier comment, I think doing this on the device is
conceptually incorrect.  I think we need an explcit notion of an SVM
context (i.e. the namespace in which all the PASIDs live) - which will
IIUC usually be shared amongst multiple devices.  The create and free
PASID requests should be on that object.

> 
> Cc: Kevin Tian <[e-mail]>
> Cc: Jacob Pan <[e-mail]>
> Cc: Peter Xu <[e-mail]>
> Cc: Eric Auger <[e-mail]>
> Cc: Yi Sun <[e-mail]>
> Cc: David Gibson <[e-mail]>
> Signed-off-by: Liu Yi L <[e-mail]>
> Signed-off-by: Yi Sun <[e-mail]>
> ---
>  hw/vfio/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ce3fe96..ab184ad 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2690,6 +2690,65 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>      vdev->req_enabled = false;
>  }
>  
> +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> +                                               int32_t devfn,
> +                                               uint32_t min_pasid,
> +                                               uint32_t max_pasid)
> +{
> +    PCIDevice *pdev = bus->devices[devfn];
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    VFIOContainer *container = vdev->vbasedev.group->container;
> +    struct vfio_iommu_type1_pasid_request req;
> +    unsigned long argsz;
> +    int pasid;
> +
> +    argsz = sizeof(req);
> +    req.argsz = argsz;
> +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> +    req.min_pasid = min_pasid;
> +    req.max_pasid = max_pasid;
> +
> +    rcu_read_lock();
> +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> +    if (pasid < 0) {
> +        error_report("vfio_pci_device_request_pasid_alloc:"
> +                     " request failed, contanier: %p", container);
> +    }
> +    rcu_read_unlock();
> +    return pasid;
> +}
> +
> +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> +                                              int32_t devfn,
> +                                              uint32_t pasid)
> +{
> +    PCIDevice *pdev = bus->devices[devfn];
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    VFIOContainer *container = vdev->vbasedev.group->container;
> +    struct vfio_iommu_type1_pasid_request req;
> +    unsigned long argsz;
> +    int ret = 0;
> +
> +    argsz = sizeof(req);
> +    req.argsz = argsz;
> +    req.flag = VFIO_IOMMU_PASID_FREE;
> +    req.pasid = pasid;
> +
> +    rcu_read_lock();
> +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> +    if (ret != 0) {
> +        error_report("vfio_pci_device_request_pasid_free:"
> +                     " request failed, contanier: %p", container);
> +    }
> +    rcu_read_unlock();
> +    return ret;
> +}
> +
> +static PCIPASIDOps vfio_pci_pasid_ops = {
> +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> +    .free_pasid = vfio_pci_device_request_pasid_free,
> +};
> +
>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      vfio_register_req_notifier(vdev);
>      vfio_setup_resetfn_quirk(vdev);
>  
> +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> +
>      return;
>  
>  out_teardown:

Yi Liu July 16, 2019, 10:25 a.m. UTC | #4

> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of David Gibson
> Sent: Monday, July 15, 2019 10:55 AM
> To: Liu, Yi L <[e-mail]>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> > These two functions are used to propagate guest pasid allocation and
> > free requests to host via vfio container ioctl.
> 
> As I said in an earlier comment, I think doing this on the device is
> conceptually incorrect.  I think we need an explcit notion of an SVM
> context (i.e. the namespace in which all the PASIDs live) - which will
> IIUC usually be shared amongst multiple devices.  The create and free
> PASID requests should be on that object.

Actually, the allocation is not doing on this device. System wide, it is
done on a container. So not sure if it is the API interface gives you a
sense that this is done on device. Also, curious on the SVM context
concept, do you mean it a per-VM context or a per-SVM usage context?
May you elaborate a little more. :-)

Thanks,
Yi Liu

> >
> > Cc: Kevin Tian <[e-mail]>
> > Cc: Jacob Pan <[e-mail]>
> > Cc: Peter Xu <[e-mail]>
> > Cc: Eric Auger <[e-mail]>
> > Cc: Yi Sun <[e-mail]>
> > Cc: David Gibson <[e-mail]>
> > Signed-off-by: Liu Yi L <[e-mail]>
> > Signed-off-by: Yi Sun <[e-mail]>
> > ---
> >  hw/vfio/pci.c | 61
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index ce3fe96..ab184ad 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2690,6 +2690,65 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice
> *vdev)
> >      vdev->req_enabled = false;
> >  }
> >
> > +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> > +                                               int32_t devfn,
> > +                                               uint32_t min_pasid,
> > +                                               uint32_t max_pasid)
> > +{
> > +    PCIDevice *pdev = bus->devices[devfn];
> > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > +    struct vfio_iommu_type1_pasid_request req;
> > +    unsigned long argsz;
> > +    int pasid;
> > +
> > +    argsz = sizeof(req);
> > +    req.argsz = argsz;
> > +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> > +    req.min_pasid = min_pasid;
> > +    req.max_pasid = max_pasid;
> > +
> > +    rcu_read_lock();
> > +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > +    if (pasid < 0) {
> > +        error_report("vfio_pci_device_request_pasid_alloc:"
> > +                     " request failed, contanier: %p", container);
> > +    }
> > +    rcu_read_unlock();
> > +    return pasid;
> > +}
> > +
> > +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> > +                                              int32_t devfn,
> > +                                              uint32_t pasid)
> > +{
> > +    PCIDevice *pdev = bus->devices[devfn];
> > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > +    struct vfio_iommu_type1_pasid_request req;
> > +    unsigned long argsz;
> > +    int ret = 0;
> > +
> > +    argsz = sizeof(req);
> > +    req.argsz = argsz;
> > +    req.flag = VFIO_IOMMU_PASID_FREE;
> > +    req.pasid = pasid;
> > +
> > +    rcu_read_lock();
> > +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > +    if (ret != 0) {
> > +        error_report("vfio_pci_device_request_pasid_free:"
> > +                     " request failed, contanier: %p", container);
> > +    }
> > +    rcu_read_unlock();
> > +    return ret;
> > +}
> > +
> > +static PCIPASIDOps vfio_pci_pasid_ops = {
> > +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> > +    .free_pasid = vfio_pci_device_request_pasid_free,
> > +};
> > +
> >  static void vfio_realize(PCIDevice *pdev, Error **errp)
> >  {
> >      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >      vfio_register_req_notifier(vdev);
> >      vfio_setup_resetfn_quirk(vdev);
> >
> > +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> > +
> >      return;
> >
> >  out_teardown:
> 
> --
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

David Gibson July 17, 2019, 3:06 a.m. UTC | #5

On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> > From: [e-mail] [mailto:[e-mail]] On Behalf
> > Of David Gibson
> > Sent: Monday, July 15, 2019 10:55 AM
> > To: Liu, Yi L <[e-mail]>
> > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> > 
> > On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > > This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> > > These two functions are used to propagate guest pasid allocation and
> > > free requests to host via vfio container ioctl.
> > 
> > As I said in an earlier comment, I think doing this on the device is
> > conceptually incorrect.  I think we need an explcit notion of an SVM
> > context (i.e. the namespace in which all the PASIDs live) - which will
> > IIUC usually be shared amongst multiple devices.  The create and free
> > PASID requests should be on that object.
> 
> Actually, the allocation is not doing on this device. System wide, it is
> done on a container. So not sure if it is the API interface gives you a
> sense that this is done on device.

Sorry, I should have been clearer.  I can see that at the VFIO level
it is done on the container.  However the function here takes a bus
and devfn, so this qemu internal interface is per-device, which
doesn't really make sense.

> Also, curious on the SVM context
> concept, do you mean it a per-VM context or a per-SVM usage context?
> May you elaborate a little more. :-)

Sorry, I'm struggling to find a good term for this.  By "context" I
mean a namespace containing a bunch of PASID address spaces, those
PASIDs are then visible to some group of devices.

> 
> Thanks,
> Yi Liu
> 
> > >
> > > Cc: Kevin Tian <[e-mail]>
> > > Cc: Jacob Pan <[e-mail]>
> > > Cc: Peter Xu <[e-mail]>
> > > Cc: Eric Auger <[e-mail]>
> > > Cc: Yi Sun <[e-mail]>
> > > Cc: David Gibson <[e-mail]>
> > > Signed-off-by: Liu Yi L <[e-mail]>
> > > Signed-off-by: Yi Sun <[e-mail]>
> > > ---
> > >  hw/vfio/pci.c | 61
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > >
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index ce3fe96..ab184ad 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2690,6 +2690,65 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice
> > *vdev)
> > >      vdev->req_enabled = false;
> > >  }
> > >
> > > +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> > > +                                               int32_t devfn,
> > > +                                               uint32_t min_pasid,
> > > +                                               uint32_t max_pasid)
> > > +{
> > > +    PCIDevice *pdev = bus->devices[devfn];
> > > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > +    struct vfio_iommu_type1_pasid_request req;
> > > +    unsigned long argsz;
> > > +    int pasid;
> > > +
> > > +    argsz = sizeof(req);
> > > +    req.argsz = argsz;
> > > +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> > > +    req.min_pasid = min_pasid;
> > > +    req.max_pasid = max_pasid;
> > > +
> > > +    rcu_read_lock();
> > > +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > +    if (pasid < 0) {
> > > +        error_report("vfio_pci_device_request_pasid_alloc:"
> > > +                     " request failed, contanier: %p", container);
> > > +    }
> > > +    rcu_read_unlock();
> > > +    return pasid;
> > > +}
> > > +
> > > +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> > > +                                              int32_t devfn,
> > > +                                              uint32_t pasid)
> > > +{
> > > +    PCIDevice *pdev = bus->devices[devfn];
> > > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > +    struct vfio_iommu_type1_pasid_request req;
> > > +    unsigned long argsz;
> > > +    int ret = 0;
> > > +
> > > +    argsz = sizeof(req);
> > > +    req.argsz = argsz;
> > > +    req.flag = VFIO_IOMMU_PASID_FREE;
> > > +    req.pasid = pasid;
> > > +
> > > +    rcu_read_lock();
> > > +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > +    if (ret != 0) {
> > > +        error_report("vfio_pci_device_request_pasid_free:"
> > > +                     " request failed, contanier: %p", container);
> > > +    }
> > > +    rcu_read_unlock();
> > > +    return ret;
> > > +}
> > > +
> > > +static PCIPASIDOps vfio_pci_pasid_ops = {
> > > +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> > > +    .free_pasid = vfio_pci_device_request_pasid_free,
> > > +};
> > > +
> > >  static void vfio_realize(PCIDevice *pdev, Error **errp)
> > >  {
> > >      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > > @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> > >      vfio_register_req_notifier(vdev);
> > >      vfio_setup_resetfn_quirk(vdev);
> > >
> > > +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> > > +
> > >      return;
> > >
> > >  out_teardown:
> > 
>

Yi Liu July 22, 2019, 7:02 a.m. UTC | #6

> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of David Gibson
> Sent: Wednesday, July 17, 2019 11:07 AM
> To: Liu, Yi L <[e-mail]>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> > > From: [e-mail] [mailto:[e-mail]] On
> Behalf
> > > Of David Gibson
> > > Sent: Monday, July 15, 2019 10:55 AM
> > > To: Liu, Yi L <[e-mail]>
> > > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> > >
> > > On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > > > This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> > > > These two functions are used to propagate guest pasid allocation and
> > > > free requests to host via vfio container ioctl.
> > >
> > > As I said in an earlier comment, I think doing this on the device is
> > > conceptually incorrect.  I think we need an explcit notion of an SVM
> > > context (i.e. the namespace in which all the PASIDs live) - which will
> > > IIUC usually be shared amongst multiple devices.  The create and free
> > > PASID requests should be on that object.
> >
> > Actually, the allocation is not doing on this device. System wide, it is
> > done on a container. So not sure if it is the API interface gives you a
> > sense that this is done on device.
> 
> Sorry, I should have been clearer.  I can see that at the VFIO level
> it is done on the container.  However the function here takes a bus
> and devfn, so this qemu internal interface is per-device, which
> doesn't really make sense.

Got it. The reason here is to pass the bus and devfn info, so that VFIO
can figure out a container for the operation. So far in QEMU, there is
no good way to connect the vIOMMU emulator and VFIO regards to
SVM. hw/pci layer is a choice based on some previous discussion. But
yes, I agree with you that we may need to have an explicit notion for
SVM. Do you think it is good to introduce a new abstract layer for SVM
(may name as SVMContext). The idea would be that vIOMMU maintain
the SVMContext instances and expose explicit interface for VFIO to get
it. Then VFIO register notifiers on to the SVMContext. When vIOMMU
emulator wants to do PASID alloc/free, it fires the corresponding
notifier. After call into VFIO, the notifier function itself figure out the
container it is bound. In this way, it's the duty of vIOMMU emulator to
figure out a proper notifier to fire. From interface point of view, it is no
longer per-device. Also, it leaves the PASID management details to
vIOMMU emulator as it can be vendor specific. Does it make sense?
Also, I'd like to know if you have any other idea on it. That would
surely be helpful. :-)

> > Also, curious on the SVM context
> > concept, do you mean it a per-VM context or a per-SVM usage context?
> > May you elaborate a little more. :-)
> 
> Sorry, I'm struggling to find a good term for this.  By "context" I
> mean a namespace containing a bunch of PASID address spaces, those
> PASIDs are then visible to some group of devices.

I see. May be the SVMContext instance above can include multiple PASID
address spaces. And again, I think this relationship should be maintained
in vIOMMU emulator.

Thanks,
Yi Liu

> 
> >
> > Thanks,
> > Yi Liu
> >
> > > >
> > > > Cc: Kevin Tian <[e-mail]>
> > > > Cc: Jacob Pan <[e-mail]>
> > > > Cc: Peter Xu <[e-mail]>
> > > > Cc: Eric Auger <[e-mail]>
> > > > Cc: Yi Sun <[e-mail]>
> > > > Cc: David Gibson <[e-mail]>
> > > > Signed-off-by: Liu Yi L <[e-mail]>
> > > > Signed-off-by: Yi Sun <[e-mail]>
> > > > ---
> > > >  hw/vfio/pci.c | 61
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 61 insertions(+)
> > > >
> > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > > index ce3fe96..ab184ad 100644
> > > > --- a/hw/vfio/pci.c
> > > > +++ b/hw/vfio/pci.c
> > > > @@ -2690,6 +2690,65 @@ static void
> vfio_unregister_req_notifier(VFIOPCIDevice
> > > *vdev)
> > > >      vdev->req_enabled = false;
> > > >  }
> > > >
> > > > +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> > > > +                                               int32_t devfn,
> > > > +                                               uint32_t min_pasid,
> > > > +                                               uint32_t max_pasid)
> > > > +{
> > > > +    PCIDevice *pdev = bus->devices[devfn];
> > > > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > > +    struct vfio_iommu_type1_pasid_request req;
> > > > +    unsigned long argsz;
> > > > +    int pasid;
> > > > +
> > > > +    argsz = sizeof(req);
> > > > +    req.argsz = argsz;
> > > > +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> > > > +    req.min_pasid = min_pasid;
> > > > +    req.max_pasid = max_pasid;
> > > > +
> > > > +    rcu_read_lock();
> > > > +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > > +    if (pasid < 0) {
> > > > +        error_report("vfio_pci_device_request_pasid_alloc:"
> > > > +                     " request failed, contanier: %p", container);
> > > > +    }
> > > > +    rcu_read_unlock();
> > > > +    return pasid;
> > > > +}
> > > > +
> > > > +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> > > > +                                              int32_t devfn,
> > > > +                                              uint32_t pasid)
> > > > +{
> > > > +    PCIDevice *pdev = bus->devices[devfn];
> > > > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > > +    struct vfio_iommu_type1_pasid_request req;
> > > > +    unsigned long argsz;
> > > > +    int ret = 0;
> > > > +
> > > > +    argsz = sizeof(req);
> > > > +    req.argsz = argsz;
> > > > +    req.flag = VFIO_IOMMU_PASID_FREE;
> > > > +    req.pasid = pasid;
> > > > +
> > > > +    rcu_read_lock();
> > > > +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > > +    if (ret != 0) {
> > > > +        error_report("vfio_pci_device_request_pasid_free:"
> > > > +                     " request failed, contanier: %p", container);
> > > > +    }
> > > > +    rcu_read_unlock();
> > > > +    return ret;
> > > > +}
> > > > +
> > > > +static PCIPASIDOps vfio_pci_pasid_ops = {
> > > > +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> > > > +    .free_pasid = vfio_pci_device_request_pasid_free,
> > > > +};
> > > > +
> > > >  static void vfio_realize(PCIDevice *pdev, Error **errp)
> > > >  {
> > > >      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > > > @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error
> **errp)
> > > >      vfio_register_req_notifier(vdev);
> > > >      vfio_setup_resetfn_quirk(vdev);
> > > >
> > > > +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> > > > +
> > > >      return;
> > > >
> > > >  out_teardown:
> > >
> >
> 
> --
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

David Gibson July 23, 2019, 3:57 a.m. UTC | #7

On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
> > From: [e-mail] [mailto:[e-mail]] On Behalf
> > Of David Gibson
> > Sent: Wednesday, July 17, 2019 11:07 AM
> > To: Liu, Yi L <[e-mail]>
> > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> > 
> > On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> > > > From: [e-mail] [mailto:[e-mail]] On
> > Behalf
> > > > Of David Gibson
> > > > Sent: Monday, July 15, 2019 10:55 AM
> > > > To: Liu, Yi L <[e-mail]>
> > > > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> > > >
> > > > On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > > > > This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> > > > > These two functions are used to propagate guest pasid allocation and
> > > > > free requests to host via vfio container ioctl.
> > > >
> > > > As I said in an earlier comment, I think doing this on the device is
> > > > conceptually incorrect.  I think we need an explcit notion of an SVM
> > > > context (i.e. the namespace in which all the PASIDs live) - which will
> > > > IIUC usually be shared amongst multiple devices.  The create and free
> > > > PASID requests should be on that object.
> > >
> > > Actually, the allocation is not doing on this device. System wide, it is
> > > done on a container. So not sure if it is the API interface gives you a
> > > sense that this is done on device.
> > 
> > Sorry, I should have been clearer.  I can see that at the VFIO level
> > it is done on the container.  However the function here takes a bus
> > and devfn, so this qemu internal interface is per-device, which
> > doesn't really make sense.
> 
> Got it. The reason here is to pass the bus and devfn info, so that VFIO
> can figure out a container for the operation. So far in QEMU, there is
> no good way to connect the vIOMMU emulator and VFIO regards to
> SVM.

Right, and I think that's an indication that we're not modelling
something in qemu that we should be.

> hw/pci layer is a choice based on some previous discussion. But
> yes, I agree with you that we may need to have an explicit notion for
> SVM. Do you think it is good to introduce a new abstract layer for SVM
> (may name as SVMContext).

I think so, yes.

If nothing else, I expect we'll need this concept if we ever want to
be able to implement SVM for emulated devices (which could be useful
for debugging, even if it's not something you'd do in production).

> The idea would be that vIOMMU maintain
> the SVMContext instances and expose explicit interface for VFIO to get
> it. Then VFIO register notifiers on to the SVMContext. When vIOMMU
> emulator wants to do PASID alloc/free, it fires the corresponding
> notifier. After call into VFIO, the notifier function itself figure out the
> container it is bound. In this way, it's the duty of vIOMMU emulator to
> figure out a proper notifier to fire. From interface point of view, it is no
> longer per-device.

Exactly.

> Also, it leaves the PASID management details to
> vIOMMU emulator as it can be vendor specific. Does it make sense?
> Also, I'd like to know if you have any other idea on it. That would
> surely be helpful. :-)
> 
> > > Also, curious on the SVM context
> > > concept, do you mean it a per-VM context or a per-SVM usage context?
> > > May you elaborate a little more. :-)
> > 
> > Sorry, I'm struggling to find a good term for this.  By "context" I
> > mean a namespace containing a bunch of PASID address spaces, those
> > PASIDs are then visible to some group of devices.
> 
> I see. May be the SVMContext instance above can include multiple PASID
> address spaces. And again, I think this relationship should be maintained
> in vIOMMU emulator.
> 
> Thanks,
> Yi Liu
> 
> > 
> > >
> > > Thanks,
> > > Yi Liu
> > >
> > > > >
> > > > > Cc: Kevin Tian <[e-mail]>
> > > > > Cc: Jacob Pan <[e-mail]>
> > > > > Cc: Peter Xu <[e-mail]>
> > > > > Cc: Eric Auger <[e-mail]>
> > > > > Cc: Yi Sun <[e-mail]>
> > > > > Cc: David Gibson <[e-mail]>
> > > > > Signed-off-by: Liu Yi L <[e-mail]>
> > > > > Signed-off-by: Yi Sun <[e-mail]>
> > > > > ---
> > > > >  hw/vfio/pci.c | 61
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 61 insertions(+)
> > > > >
> > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > > > index ce3fe96..ab184ad 100644
> > > > > --- a/hw/vfio/pci.c
> > > > > +++ b/hw/vfio/pci.c
> > > > > @@ -2690,6 +2690,65 @@ static void
> > vfio_unregister_req_notifier(VFIOPCIDevice
> > > > *vdev)
> > > > >      vdev->req_enabled = false;
> > > > >  }
> > > > >
> > > > > +static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
> > > > > +                                               int32_t devfn,
> > > > > +                                               uint32_t min_pasid,
> > > > > +                                               uint32_t max_pasid)
> > > > > +{
> > > > > +    PCIDevice *pdev = bus->devices[devfn];
> > > > > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > > > +    struct vfio_iommu_type1_pasid_request req;
> > > > > +    unsigned long argsz;
> > > > > +    int pasid;
> > > > > +
> > > > > +    argsz = sizeof(req);
> > > > > +    req.argsz = argsz;
> > > > > +    req.flag = VFIO_IOMMU_PASID_ALLOC;
> > > > > +    req.min_pasid = min_pasid;
> > > > > +    req.max_pasid = max_pasid;
> > > > > +
> > > > > +    rcu_read_lock();
> > > > > +    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > > > +    if (pasid < 0) {
> > > > > +        error_report("vfio_pci_device_request_pasid_alloc:"
> > > > > +                     " request failed, contanier: %p", container);
> > > > > +    }
> > > > > +    rcu_read_unlock();
> > > > > +    return pasid;
> > > > > +}
> > > > > +
> > > > > +static int vfio_pci_device_request_pasid_free(PCIBus *bus,
> > > > > +                                              int32_t devfn,
> > > > > +                                              uint32_t pasid)
> > > > > +{
> > > > > +    PCIDevice *pdev = bus->devices[devfn];
> > > > > +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> > > > > +    VFIOContainer *container = vdev->vbasedev.group->container;
> > > > > +    struct vfio_iommu_type1_pasid_request req;
> > > > > +    unsigned long argsz;
> > > > > +    int ret = 0;
> > > > > +
> > > > > +    argsz = sizeof(req);
> > > > > +    req.argsz = argsz;
> > > > > +    req.flag = VFIO_IOMMU_PASID_FREE;
> > > > > +    req.pasid = pasid;
> > > > > +
> > > > > +    rcu_read_lock();
> > > > > +    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
> > > > > +    if (ret != 0) {
> > > > > +        error_report("vfio_pci_device_request_pasid_free:"
> > > > > +                     " request failed, contanier: %p", container);
> > > > > +    }
> > > > > +    rcu_read_unlock();
> > > > > +    return ret;
> > > > > +}
> > > > > +
> > > > > +static PCIPASIDOps vfio_pci_pasid_ops = {
> > > > > +    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
> > > > > +    .free_pasid = vfio_pci_device_request_pasid_free,
> > > > > +};
> > > > > +
> > > > >  static void vfio_realize(PCIDevice *pdev, Error **errp)
> > > > >  {
> > > > >      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > > > > @@ -2991,6 +3050,8 @@ static void vfio_realize(PCIDevice *pdev, Error
> > **errp)
> > > > >      vfio_register_req_notifier(vdev);
> > > > >      vfio_setup_resetfn_quirk(vdev);
> > > > >
> > > > > +    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
> > > > > +
> > > > >      return;
> > > > >
> > > > >  out_teardown:
> > > >
> > >
> > 
>

Yi Liu July 24, 2019, 4:57 a.m. UTC | #8

> From: [e-mail] [mailto:[e-mail]] On Behalf
> Of David Gibson
> Sent: Tuesday, July 23, 2019 11:58 AM
> To: Liu, Yi L <[e-mail]>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
> > > From: [e-mail] [mailto:[e-mail]]
> > > On Behalf Of David Gibson
> > > Sent: Wednesday, July 17, 2019 11:07 AM
> > > To: Liu, Yi L <[e-mail]>
> > > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> > > implementation
> > >
> > > On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> > > > > From: [e-mail]
> > > > > [mailto:[e-mail]] On
> > > Behalf
> > > > > Of David Gibson
> > > > > Sent: Monday, July 15, 2019 10:55 AM
> > > > > To: Liu, Yi L <[e-mail]>
> > > > > Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> > > > > implementation
> > > > >
> > > > > On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> > > > > > This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> > > > > > These two functions are used to propagate guest pasid
> > > > > > allocation and free requests to host via vfio container ioctl.
> > > > >
> > > > > As I said in an earlier comment, I think doing this on the
> > > > > device is conceptually incorrect.  I think we need an explcit
> > > > > notion of an SVM context (i.e. the namespace in which all the
> > > > > PASIDs live) - which will IIUC usually be shared amongst
> > > > > multiple devices.  The create and free PASID requests should be on that object.
> > > >
> > > > Actually, the allocation is not doing on this device. System wide,
> > > > it is done on a container. So not sure if it is the API interface
> > > > gives you a sense that this is done on device.
> > >
> > > Sorry, I should have been clearer.  I can see that at the VFIO level
> > > it is done on the container.  However the function here takes a bus
> > > and devfn, so this qemu internal interface is per-device, which
> > > doesn't really make sense.
> >
> > Got it. The reason here is to pass the bus and devfn info, so that
> > VFIO can figure out a container for the operation. So far in QEMU,
> > there is no good way to connect the vIOMMU emulator and VFIO regards
> > to SVM.
> 
> Right, and I think that's an indication that we're not modelling something in qemu
> that we should be.
> 
> > hw/pci layer is a choice based on some previous discussion. But yes, I
> > agree with you that we may need to have an explicit notion for SVM. Do
> > you think it is good to introduce a new abstract layer for SVM (may
> > name as SVMContext).
> 
> I think so, yes.
> 
> If nothing else, I expect we'll need this concept if we ever want to be able to
> implement SVM for emulated devices (which could be useful for debugging, even if
> it's not something you'd do in production).
> 
> > The idea would be that vIOMMU maintain the SVMContext instances and
> > expose explicit interface for VFIO to get it. Then VFIO register
> > notifiers on to the SVMContext. When vIOMMU emulator wants to do PASID
> > alloc/free, it fires the corresponding notifier. After call into VFIO,
> > the notifier function itself figure out the container it is bound. In
> > this way, it's the duty of vIOMMU emulator to figure out a proper
> > notifier to fire. From interface point of view, it is no longer
> > per-device.
> 
> Exactly.

Cool, let me prepare another version with the ideas. Thanks for your
review. :-)

Regards,
Yi Liu

> > Also, it leaves the PASID management details to vIOMMU emulator as it
> > can be vendor specific. Does it make sense?
> > Also, I'd like to know if you have any other idea on it. That would
> > surely be helpful. :-)
> >
> > > > Also, curious on the SVM context
> > > > concept, do you mean it a per-VM context or a per-SVM usage context?
> > > > May you elaborate a little more. :-)
> > >
> > > Sorry, I'm struggling to find a good term for this.  By "context" I
> > > mean a namespace containing a bunch of PASID address spaces, those
> > > PASIDs are then visible to some group of devices.
> >
> > I see. May be the SVMContext instance above can include multiple PASID
> > address spaces. And again, I think this relationship should be
> > maintained in vIOMMU emulator.

Eric Auger July 24, 2019, 9:33 a.m. UTC | #9

Hi Yi, David,

On 7/24/19 6:57 AM, Liu, Yi L wrote:
>> From: [e-mail] [mailto:[e-mail]] On Behalf
>> Of David Gibson
>> Sent: Tuesday, July 23, 2019 11:58 AM
>> To: Liu, Yi L <[e-mail]>
>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
>>
>> On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
>>>> From: [e-mail] [mailto:[e-mail]]
>>>> On Behalf Of David Gibson
>>>> Sent: Wednesday, July 17, 2019 11:07 AM
>>>> To: Liu, Yi L <[e-mail]>
>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
>>>> implementation
>>>>
>>>> On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
>>>>>> From: [e-mail]
>>>>>> [mailto:[e-mail]] On
>>>> Behalf
>>>>>> Of David Gibson
>>>>>> Sent: Monday, July 15, 2019 10:55 AM
>>>>>> To: Liu, Yi L <[e-mail]>
>>>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
>>>>>> implementation
>>>>>>
>>>>>> On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
>>>>>>> This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
>>>>>>> These two functions are used to propagate guest pasid
>>>>>>> allocation and free requests to host via vfio container ioctl.
>>>>>>
>>>>>> As I said in an earlier comment, I think doing this on the
>>>>>> device is conceptually incorrect.  I think we need an explcit
>>>>>> notion of an SVM context (i.e. the namespace in which all the
>>>>>> PASIDs live) - which will IIUC usually be shared amongst
>>>>>> multiple devices.  The create and free PASID requests should be on that object.
>>>>>
>>>>> Actually, the allocation is not doing on this device. System wide,
>>>>> it is done on a container. So not sure if it is the API interface
>>>>> gives you a sense that this is done on device.
>>>>
>>>> Sorry, I should have been clearer.  I can see that at the VFIO level
>>>> it is done on the container.  However the function here takes a bus
>>>> and devfn, so this qemu internal interface is per-device, which
>>>> doesn't really make sense.
>>>
>>> Got it. The reason here is to pass the bus and devfn info, so that
>>> VFIO can figure out a container for the operation. So far in QEMU,
>>> there is no good way to connect the vIOMMU emulator and VFIO regards
>>> to SVM.
>>
>> Right, and I think that's an indication that we're not modelling something in qemu
>> that we should be.
>>
>>> hw/pci layer is a choice based on some previous discussion. But yes, I
>>> agree with you that we may need to have an explicit notion for SVM. Do
>>> you think it is good to introduce a new abstract layer for SVM (may
>>> name as SVMContext).
>>
>> I think so, yes.
>>
>> If nothing else, I expect we'll need this concept if we ever want to be able to
>> implement SVM for emulated devices (which could be useful for debugging, even if
>> it's not something you'd do in production).
>>
>>> The idea would be that vIOMMU maintain the SVMContext instances and
>>> expose explicit interface for VFIO to get it. Then VFIO register
>>> notifiers on to the SVMContext. When vIOMMU emulator wants to do PASID
>>> alloc/free, it fires the corresponding notifier. After call into VFIO,
>>> the notifier function itself figure out the container it is bound. In
>>> this way, it's the duty of vIOMMU emulator to figure out a proper
>>> notifier to fire. From interface point of view, it is no longer
>>> per-device.
>>
>> Exactly.
> 
> Cool, let me prepare another version with the ideas. Thanks for your
> review. :-)
> 
> Regards,
> Yi Liu
> 
>>> Also, it leaves the PASID management details to vIOMMU emulator as it
>>> can be vendor specific. Does it make sense?
>>> Also, I'd like to know if you have any other idea on it. That would
>>> surely be helpful. :-)
>>>
>>>>> Also, curious on the SVM context
>>>>> concept, do you mean it a per-VM context or a per-SVM usage context?
>>>>> May you elaborate a little more. :-)
>>>>
>>>> Sorry, I'm struggling to find a good term for this.  By "context" I
>>>> mean a namespace containing a bunch of PASID address spaces, those
>>>> PASIDs are then visible to some group of devices.
>>>
>>> I see. May be the SVMContext instance above can include multiple PASID
>>> address spaces. And again, I think this relationship should be
>>> maintained in vIOMMU emulator.
> 
So if I understand we now head towards introducing new notifiers taking
a "SVMContext" as argument instead of an IOMMUMemoryRegion.

I think we need to be clear about how both abstractions (SVMContext and
IOMMUMemoryRegion) differ. I would also need "SVMContext" abstraction
for 2stage SMMU integration (to notify stage 1 config changes and MSI
bindings) so I would need this new object to be not too much tied to SVM
use case.

Thanks

Eric

David Gibson July 25, 2019, 3:40 a.m. UTC | #10

On Wed, Jul 24, 2019 at 11:33:06AM +0200, Auger Eric wrote:
> Hi Yi, David,
> 
> On 7/24/19 6:57 AM, Liu, Yi L wrote:
> >> From: [e-mail] [mailto:[e-mail]] On Behalf
> >> Of David Gibson
> >> Sent: Tuesday, July 23, 2019 11:58 AM
> >> To: Liu, Yi L <[e-mail]>
> >> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> >>
> >> On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
> >>>> From: [e-mail] [mailto:[e-mail]]
> >>>> On Behalf Of David Gibson
> >>>> Sent: Wednesday, July 17, 2019 11:07 AM
> >>>> To: Liu, Yi L <[e-mail]>
> >>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> >>>> implementation
> >>>>
> >>>> On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> >>>>>> From: [e-mail]
> >>>>>> [mailto:[e-mail]] On
> >>>> Behalf
> >>>>>> Of David Gibson
> >>>>>> Sent: Monday, July 15, 2019 10:55 AM
> >>>>>> To: Liu, Yi L <[e-mail]>
> >>>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> >>>>>> implementation
> >>>>>>
> >>>>>> On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> >>>>>>> This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> >>>>>>> These two functions are used to propagate guest pasid
> >>>>>>> allocation and free requests to host via vfio container ioctl.
> >>>>>>
> >>>>>> As I said in an earlier comment, I think doing this on the
> >>>>>> device is conceptually incorrect.  I think we need an explcit
> >>>>>> notion of an SVM context (i.e. the namespace in which all the
> >>>>>> PASIDs live) - which will IIUC usually be shared amongst
> >>>>>> multiple devices.  The create and free PASID requests should be on that object.
> >>>>>
> >>>>> Actually, the allocation is not doing on this device. System wide,
> >>>>> it is done on a container. So not sure if it is the API interface
> >>>>> gives you a sense that this is done on device.
> >>>>
> >>>> Sorry, I should have been clearer.  I can see that at the VFIO level
> >>>> it is done on the container.  However the function here takes a bus
> >>>> and devfn, so this qemu internal interface is per-device, which
> >>>> doesn't really make sense.
> >>>
> >>> Got it. The reason here is to pass the bus and devfn info, so that
> >>> VFIO can figure out a container for the operation. So far in QEMU,
> >>> there is no good way to connect the vIOMMU emulator and VFIO regards
> >>> to SVM.
> >>
> >> Right, and I think that's an indication that we're not modelling something in qemu
> >> that we should be.
> >>
> >>> hw/pci layer is a choice based on some previous discussion. But yes, I
> >>> agree with you that we may need to have an explicit notion for SVM. Do
> >>> you think it is good to introduce a new abstract layer for SVM (may
> >>> name as SVMContext).
> >>
> >> I think so, yes.
> >>
> >> If nothing else, I expect we'll need this concept if we ever want to be able to
> >> implement SVM for emulated devices (which could be useful for debugging, even if
> >> it's not something you'd do in production).
> >>
> >>> The idea would be that vIOMMU maintain the SVMContext instances and
> >>> expose explicit interface for VFIO to get it. Then VFIO register
> >>> notifiers on to the SVMContext. When vIOMMU emulator wants to do PASID
> >>> alloc/free, it fires the corresponding notifier. After call into VFIO,
> >>> the notifier function itself figure out the container it is bound. In
> >>> this way, it's the duty of vIOMMU emulator to figure out a proper
> >>> notifier to fire. From interface point of view, it is no longer
> >>> per-device.
> >>
> >> Exactly.
> > 
> > Cool, let me prepare another version with the ideas. Thanks for your
> > review. :-)
> > 
> > Regards,
> > Yi Liu
> > 
> >>> Also, it leaves the PASID management details to vIOMMU emulator as it
> >>> can be vendor specific. Does it make sense?
> >>> Also, I'd like to know if you have any other idea on it. That would
> >>> surely be helpful. :-)
> >>>
> >>>>> Also, curious on the SVM context
> >>>>> concept, do you mean it a per-VM context or a per-SVM usage context?
> >>>>> May you elaborate a little more. :-)
> >>>>
> >>>> Sorry, I'm struggling to find a good term for this.  By "context" I
> >>>> mean a namespace containing a bunch of PASID address spaces, those
> >>>> PASIDs are then visible to some group of devices.
> >>>
> >>> I see. May be the SVMContext instance above can include multiple PASID
> >>> address spaces. And again, I think this relationship should be
> >>> maintained in vIOMMU emulator.
> > 
> So if I understand we now head towards introducing new notifiers taking
> a "SVMContext" as argument instead of an IOMMUMemoryRegion.
> 
> I think we need to be clear about how both abstractions (SVMContext and
> IOMMUMemoryRegion) differ. I would also need "SVMContext" abstraction
> for 2stage SMMU integration (to notify stage 1 config changes and MSI
> bindings) so I would need this new object to be not too much tied to SVM
> use case.

That's my suggestion.  I don't really have any authority to decide..

Yi Liu July 26, 2019, 5:18 a.m. UTC | #11

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:[e-mail]]
> Sent: Wednesday, July 24, 2019 5:33 PM
> To: Liu, Yi L <[e-mail]>; David Gibson <[e-mail]>
> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
> 
> Hi Yi, David,
> 
> On 7/24/19 6:57 AM, Liu, Yi L wrote:
> >> From: [e-mail] [mailto:[e-mail]] On
> >> Behalf Of David Gibson
> >> Sent: Tuesday, July 23, 2019 11:58 AM
> >> To: Liu, Yi L <[e-mail]>
> >> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> >> implementation
> >>
> >> On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
> >>>> From: [e-mail] [mailto:[e-mail]]
> >>>> On Behalf Of David Gibson
> >>>> Sent: Wednesday, July 17, 2019 11:07 AM
> >>>> To: Liu, Yi L <[e-mail]>
> >>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> >>>> implementation
> >>>>
> >>>> On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
> >>>>>> From: [e-mail]
> >>>>>> [mailto:[e-mail]] On
> >>>> Behalf
> >>>>>> Of David Gibson
> >>>>>> Sent: Monday, July 15, 2019 10:55 AM
> >>>>>> To: Liu, Yi L <[e-mail]>
> >>>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
> >>>>>> implementation
> >>>>>>
> >>>>>> On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
> >>>>>>> This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
> >>>>>>> These two functions are used to propagate guest pasid allocation
> >>>>>>> and free requests to host via vfio container ioctl.
> >>>>>>
> >>>>>> As I said in an earlier comment, I think doing this on the device
> >>>>>> is conceptually incorrect.  I think we need an explcit notion of
> >>>>>> an SVM context (i.e. the namespace in which all the PASIDs live)
> >>>>>> - which will IIUC usually be shared amongst multiple devices.
> >>>>>> The create and free PASID requests should be on that object.
> >>>>>
> >>>>> Actually, the allocation is not doing on this device. System wide,
> >>>>> it is done on a container. So not sure if it is the API interface
> >>>>> gives you a sense that this is done on device.
> >>>>
> >>>> Sorry, I should have been clearer.  I can see that at the VFIO
> >>>> level it is done on the container.  However the function here takes
> >>>> a bus and devfn, so this qemu internal interface is per-device,
> >>>> which doesn't really make sense.
> >>>
> >>> Got it. The reason here is to pass the bus and devfn info, so that
> >>> VFIO can figure out a container for the operation. So far in QEMU,
> >>> there is no good way to connect the vIOMMU emulator and VFIO regards
> >>> to SVM.
> >>
> >> Right, and I think that's an indication that we're not modelling
> >> something in qemu that we should be.
> >>
> >>> hw/pci layer is a choice based on some previous discussion. But yes,
> >>> I agree with you that we may need to have an explicit notion for
> >>> SVM. Do you think it is good to introduce a new abstract layer for
> >>> SVM (may name as SVMContext).
> >>
> >> I think so, yes.
> >>
> >> If nothing else, I expect we'll need this concept if we ever want to
> >> be able to implement SVM for emulated devices (which could be useful
> >> for debugging, even if it's not something you'd do in production).
> >>
> >>> The idea would be that vIOMMU maintain the SVMContext instances and
> >>> expose explicit interface for VFIO to get it. Then VFIO register
> >>> notifiers on to the SVMContext. When vIOMMU emulator wants to do
> >>> PASID alloc/free, it fires the corresponding notifier. After call
> >>> into VFIO, the notifier function itself figure out the container it
> >>> is bound. In this way, it's the duty of vIOMMU emulator to figure
> >>> out a proper notifier to fire. From interface point of view, it is
> >>> no longer per-device.
> >>
> >> Exactly.
> >
> > Cool, let me prepare another version with the ideas. Thanks for your
> > review. :-)
> >
> > Regards,
> > Yi Liu
> >
> >>> Also, it leaves the PASID management details to vIOMMU emulator as
> >>> it can be vendor specific. Does it make sense?
> >>> Also, I'd like to know if you have any other idea on it. That would
> >>> surely be helpful. :-)
> >>>
> >>>>> Also, curious on the SVM context
> >>>>> concept, do you mean it a per-VM context or a per-SVM usage context?
> >>>>> May you elaborate a little more. :-)
> >>>>
> >>>> Sorry, I'm struggling to find a good term for this.  By "context" I
> >>>> mean a namespace containing a bunch of PASID address spaces, those
> >>>> PASIDs are then visible to some group of devices.
> >>>
> >>> I see. May be the SVMContext instance above can include multiple
> >>> PASID address spaces. And again, I think this relationship should be
> >>> maintained in vIOMMU emulator.
> >
> So if I understand we now head towards introducing new notifiers taking a
> "SVMContext" as argument instead of an IOMMUMemoryRegion.

yes, this is the rough idea.
 
> I think we need to be clear about how both abstractions (SVMContext and
> IOMMUMemoryRegion) differ. I would also need "SVMContext" abstraction for
> 2stage SMMU integration (to notify stage 1 config changes and MSI
> bindings) so I would need this new object to be not too much tied to SVM use case.

I agree. SVMContext is just a proposed name. We may have better naming for it
as long as the thing we want to have is a new abstract layer between VFIO and
vIOMMU. Per my understanding, the IOMMUMemoryRegion abstraction is for
the notifications around guest memory changes. e.g. VFIO needs to be notified
when there is MAP/UNMAP happened. However, for the SVMContext, it aims to
be an abstraction for SVM/PASID related operations, which has no direct
relationship with memory. e.g. for VT-d, pasid allocation, pasid bind/unbind,
pasid based-iotlb flush. I think pasid bind/unbind and pasid based-iotlb flush is
equivalent with the stage 1 config changes in SMMU. If you agree to use it
all the same, how about naming it as IOMMUConext? Also, pls feel free to
propose your suggestion. :-)

Thanks,
Yi Liu

changes.

> Thanks
> 
> Eric

Eric Auger Aug. 2, 2019, 7:36 a.m. UTC | #12

Hi Yi,

On 7/26/19 7:18 AM, Liu, Yi L wrote:
> Hi Eric,
> 
>> -----Original Message-----
>> From: Auger Eric [mailto:[e-mail]]
>> Sent: Wednesday, July 24, 2019 5:33 PM
>> To: Liu, Yi L <[e-mail]>; David Gibson <[e-mail]>
>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free implementation
>>
>> Hi Yi, David,
>>
>> On 7/24/19 6:57 AM, Liu, Yi L wrote:
>>>> From: [e-mail] [mailto:[e-mail]] On
>>>> Behalf Of David Gibson
>>>> Sent: Tuesday, July 23, 2019 11:58 AM
>>>> To: Liu, Yi L <[e-mail]>
>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
>>>> implementation
>>>>
>>>> On Mon, Jul 22, 2019 at 07:02:51AM +0000, Liu, Yi L wrote:
>>>>>> From: [e-mail] [mailto:[e-mail]]
>>>>>> On Behalf Of David Gibson
>>>>>> Sent: Wednesday, July 17, 2019 11:07 AM
>>>>>> To: Liu, Yi L <[e-mail]>
>>>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
>>>>>> implementation
>>>>>>
>>>>>> On Tue, Jul 16, 2019 at 10:25:55AM +0000, Liu, Yi L wrote:
>>>>>>>> From: [e-mail]
>>>>>>>> [mailto:[e-mail]] On
>>>>>> Behalf
>>>>>>>> Of David Gibson
>>>>>>>> Sent: Monday, July 15, 2019 10:55 AM
>>>>>>>> To: Liu, Yi L <[e-mail]>
>>>>>>>> Subject: Re: [RFC v1 05/18] vfio/pci: add pasid alloc/free
>>>>>>>> implementation
>>>>>>>>
>>>>>>>> On Fri, Jul 05, 2019 at 07:01:38PM +0800, Liu Yi L wrote:
>>>>>>>>> This patch adds vfio implementation PCIPASIDOps.alloc_pasid/free_pasid().
>>>>>>>>> These two functions are used to propagate guest pasid allocation
>>>>>>>>> and free requests to host via vfio container ioctl.
>>>>>>>>
>>>>>>>> As I said in an earlier comment, I think doing this on the device
>>>>>>>> is conceptually incorrect.  I think we need an explcit notion of
>>>>>>>> an SVM context (i.e. the namespace in which all the PASIDs live)
>>>>>>>> - which will IIUC usually be shared amongst multiple devices.
>>>>>>>> The create and free PASID requests should be on that object.
>>>>>>>
>>>>>>> Actually, the allocation is not doing on this device. System wide,
>>>>>>> it is done on a container. So not sure if it is the API interface
>>>>>>> gives you a sense that this is done on device.
>>>>>>
>>>>>> Sorry, I should have been clearer.  I can see that at the VFIO
>>>>>> level it is done on the container.  However the function here takes
>>>>>> a bus and devfn, so this qemu internal interface is per-device,
>>>>>> which doesn't really make sense.
>>>>>
>>>>> Got it. The reason here is to pass the bus and devfn info, so that
>>>>> VFIO can figure out a container for the operation. So far in QEMU,
>>>>> there is no good way to connect the vIOMMU emulator and VFIO regards
>>>>> to SVM.
>>>>
>>>> Right, and I think that's an indication that we're not modelling
>>>> something in qemu that we should be.
>>>>
>>>>> hw/pci layer is a choice based on some previous discussion. But yes,
>>>>> I agree with you that we may need to have an explicit notion for
>>>>> SVM. Do you think it is good to introduce a new abstract layer for
>>>>> SVM (may name as SVMContext).
>>>>
>>>> I think so, yes.
>>>>
>>>> If nothing else, I expect we'll need this concept if we ever want to
>>>> be able to implement SVM for emulated devices (which could be useful
>>>> for debugging, even if it's not something you'd do in production).
>>>>
>>>>> The idea would be that vIOMMU maintain the SVMContext instances and
>>>>> expose explicit interface for VFIO to get it. Then VFIO register
>>>>> notifiers on to the SVMContext. When vIOMMU emulator wants to do
>>>>> PASID alloc/free, it fires the corresponding notifier. After call
>>>>> into VFIO, the notifier function itself figure out the container it
>>>>> is bound. In this way, it's the duty of vIOMMU emulator to figure
>>>>> out a proper notifier to fire. From interface point of view, it is
>>>>> no longer per-device.
>>>>
>>>> Exactly.
>>>
>>> Cool, let me prepare another version with the ideas. Thanks for your
>>> review. :-)
>>>
>>> Regards,
>>> Yi Liu
>>>
>>>>> Also, it leaves the PASID management details to vIOMMU emulator as
>>>>> it can be vendor specific. Does it make sense?
>>>>> Also, I'd like to know if you have any other idea on it. That would
>>>>> surely be helpful. :-)
>>>>>
>>>>>>> Also, curious on the SVM context
>>>>>>> concept, do you mean it a per-VM context or a per-SVM usage context?
>>>>>>> May you elaborate a little more. :-)
>>>>>>
>>>>>> Sorry, I'm struggling to find a good term for this.  By "context" I
>>>>>> mean a namespace containing a bunch of PASID address spaces, those
>>>>>> PASIDs are then visible to some group of devices.
>>>>>
>>>>> I see. May be the SVMContext instance above can include multiple
>>>>> PASID address spaces. And again, I think this relationship should be
>>>>> maintained in vIOMMU emulator.
>>>
>> So if I understand we now head towards introducing new notifiers taking a
>> "SVMContext" as argument instead of an IOMMUMemoryRegion.
> 
> yes, this is the rough idea.
>  
>> I think we need to be clear about how both abstractions (SVMContext and
>> IOMMUMemoryRegion) differ. I would also need "SVMContext" abstraction for
>> 2stage SMMU integration (to notify stage 1 config changes and MSI
>> bindings) so I would need this new object to be not too much tied to SVM use case.
> 
> I agree. SVMContext is just a proposed name. We may have better naming for it
> as long as the thing we want to have is a new abstract layer between VFIO and
> vIOMMU. Per my understanding, the IOMMUMemoryRegion abstraction is for
> the notifications around guest memory changes. e.g. VFIO needs to be notified
> when there is MAP/UNMAP happened. However, for the SVMContext, it aims to
> be an abstraction for SVM/PASID related operations, which has no direct
> relationship with memory. e.g. for VT-d, pasid allocation, pasid bind/unbind,
> pasid based-iotlb flush. I think pasid bind/unbind and pasid based-iotlb flush is
> equivalent with the stage 1 config changes in SMMU. If you agree to use it
> all the same, how about naming it as IOMMUConext? Also, pls feel free to
> propose your suggestion. :-)
Sorry for the delay. Yes IOMMUContext sounds OK to me. Looking forward
to reading your next revision.

Thanks

Eric
> 
> Thanks,
> Yi Liu
> 
> changes.
> 
>> Thanks
>>
>> Eric
>

Patch

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ce3fe96..ab184ad 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2690,6 +2690,65 @@  static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
     vdev->req_enabled = false;
 }
 
+static int vfio_pci_device_request_pasid_alloc(PCIBus *bus,
+                                               int32_t devfn,
+                                               uint32_t min_pasid,
+                                               uint32_t max_pasid)
+{
+    PCIDevice *pdev = bus->devices[devfn];
+    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+    VFIOContainer *container = vdev->vbasedev.group->container;
+    struct vfio_iommu_type1_pasid_request req;
+    unsigned long argsz;
+    int pasid;
+
+    argsz = sizeof(req);
+    req.argsz = argsz;
+    req.flag = VFIO_IOMMU_PASID_ALLOC;
+    req.min_pasid = min_pasid;
+    req.max_pasid = max_pasid;
+
+    rcu_read_lock();
+    pasid = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
+    if (pasid < 0) {
+        error_report("vfio_pci_device_request_pasid_alloc:"
+                     " request failed, contanier: %p", container);
+    }
+    rcu_read_unlock();
+    return pasid;
+}
+
+static int vfio_pci_device_request_pasid_free(PCIBus *bus,
+                                              int32_t devfn,
+                                              uint32_t pasid)
+{
+    PCIDevice *pdev = bus->devices[devfn];
+    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+    VFIOContainer *container = vdev->vbasedev.group->container;
+    struct vfio_iommu_type1_pasid_request req;
+    unsigned long argsz;
+    int ret = 0;
+
+    argsz = sizeof(req);
+    req.argsz = argsz;
+    req.flag = VFIO_IOMMU_PASID_FREE;
+    req.pasid = pasid;
+
+    rcu_read_lock();
+    ret = ioctl(container->fd, VFIO_IOMMU_PASID_REQUEST, &req);
+    if (ret != 0) {
+        error_report("vfio_pci_device_request_pasid_free:"
+                     " request failed, contanier: %p", container);
+    }
+    rcu_read_unlock();
+    return ret;
+}
+
+static PCIPASIDOps vfio_pci_pasid_ops = {
+    .alloc_pasid = vfio_pci_device_request_pasid_alloc,
+    .free_pasid = vfio_pci_device_request_pasid_free,
+};
+
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
@@ -2991,6 +3050,8 @@  static void vfio_realize(PCIDevice *pdev, Error **errp)
     vfio_register_req_notifier(vdev);
     vfio_setup_resetfn_quirk(vdev);
 
+    pci_setup_pasid_ops(pdev, &vfio_pci_pasid_ops);
+
     return;
 
 out_teardown: