Re: [Qemu-devel] [RFC v1 03/18] hw/pci: introduce PCIPASIDOps to PCIDevice

[Liu, Yi L]

> From: address@hidden [mailto:address@hidden]
> Sent: Thursday, July 11, 2019 11:52 AM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v1 03/18] hw/pci: introduce PCIPASIDOps to PCIDevice
> 
> On Wed, Jul 10, 2019 at 11:08:15AM +0000, Liu, Yi L wrote:
> > > From: Peter Xu [mailto:address@hidden]
> > > Sent: Tuesday, July 9, 2019 10:12 AM
> > > To: Liu, Yi L <address@hidden>
> > > Subject: Re: [RFC v1 03/18] hw/pci: introduce PCIPASIDOps to PCIDevice
> > >
> > > On Fri, Jul 05, 2019 at 07:01:36PM +0800, Liu Yi L wrote:
> > > > +void pci_setup_pasid_ops(PCIDevice *dev, PCIPASIDOps *ops)
> > > > +{
> > > > +    assert(ops && !dev->pasid_ops);
> > > > +    dev->pasid_ops = ops;
> > > > +}
> > > > +
> > > > +bool pci_device_is_ops_set(PCIBus *bus, int32_t devfn)
> > >
> > > Name should be "pci_device_is_pasid_ops_set".  Or maybe you can simply
> > > drop this function because as long as you check it in helper functions
> > > like [1] below always then it seems even unecessary.
> >
> > yes, the name should be "pci_device_is_pasid_ops_set". I noticed your
> > comments on the necessity in another, let's talk in that thread. :-)
> >
> > > > +{
> > > > +    PCIDevice *dev;
> > > > +
> > > > +    if (!bus) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    dev = bus->devices[devfn];
> > > > +    return !!(dev && dev->pasid_ops);
> > > > +}
> > > > +
> > > > +int pci_device_request_pasid_alloc(PCIBus *bus, int32_t devfn,
> > > > +                                   uint32_t min_pasid, uint32_t 
> > > > max_pasid)
> > >
> > > From VT-d spec I see that the virtual command "allocate pasid" does
> > > not have bdf information so it's global, but here we've got bus/devfn.
> > > I'm curious is that reserved for ARM or some other arch?
> >
> > You are right. VT-d spec doesn’t have bdf info. But we need to pass the
> > allocation request via vfio. So this function has bdf info. In vIOMMU side,
> > it should select a vfio-pci device and invoke this callback when it wants to
> > request PASID alloc/free.
> 
> That doesn't seem conceptually right.  IIUC, the pasids "belong" to a
> sort of SVM context.  It seems to be the alloc should be on that
> object - and that object would already have some connection to any
> relevant vfio containers.  At the vfio level this seems like it should
> be a container operation rather than a device operation.

Hi David,

Yeah, I agree it should finally be a container operation. Actually, in the
callback implementation, it is a container operation. May refer to the
implementation in below patch. :-)

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

Thanks,
Yi Liu

> > > > +{
> > > > +    PCIDevice *dev;
> > > > +
> > > > +    if (!bus) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    dev = bus->devices[devfn];
> > > > +    if (dev && dev->pasid_ops && dev->pasid_ops->alloc_pasid) {
> > >
> > > [1]
> > >
> > > > +        return dev->pasid_ops->alloc_pasid(bus, devfn, min_pasid, 
> > > > max_pasid);
> >
> > Thanks,
> > Yi Liu
> 
> --
> 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