Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Tian, Kevin]

> From: Peter Xu [mailto:address@hidden
> Sent: Friday, March 9, 2018 3:59 PM
> 
> On Thu, Mar 08, 2018 at 09:39:18AM +0000, Liu, Yi L wrote:
> > > From: Peter Xu [mailto:address@hidden
> > > Sent: Tuesday, March 6, 2018 7:44 PM
> > > Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID tagged
> > > AddressSpace
> > >
> > > On Thu, Mar 01, 2018 at 06:33:35PM +0800, Liu, Yi L wrote:
> > > > This patch shows the idea of how a device is binded to a PASID tagged
> > > > AddressSpace.
> > > >
> > > > when Intel vIOMMU emulator detected a pasid table entry
> programming
> > > > from guest. Intel vIOMMU emulator firstly finds a
> VTDPASIDAddressSpace
> > > > with the pasid field of pasid cache invalidate request.
> > > >
> > > > * If it is to bind a device to a guest process, needs add the device
> > > >   to the device list behind the VTDPASIDAddressSpace. And if the
> device
> > > >   is assigned device, need to register sva_notfier for future tlb
> > > >   flushing if any mapping changed to the process address space.
> > > >
> > > > * If it is to unbind a device from a guest process, then need to remove
> > > >   the device from the device list behind the VTDPASIDAddressSpace.
> > > >   And also needs to unregister the sva_notfier if the device is assigned
> > > >   device.
> > > >
> > > > This patch hasn't added the unbind logic. It depends on guest pasid
> > > > table entry parsing which requires further emulation. Here just want
> > > > to show the idea for the PASID tagged AddressSpace management
> framework.
> > > > Full unregister logic would be included in future virt-SVA patchset.
> > > >
> > > > Signed-off-by: Liu, Yi L <address@hidden>
> > > > ---
> > > >  hw/i386/intel_iommu.c          | 119
> > > +++++++++++++++++++++++++++++++++++++++++
> > > >  hw/i386/intel_iommu_internal.h |  10 ++++
> > > >  2 files changed, 129 insertions(+)
> > > >
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > index b8e8dbb..ed07035 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -1801,6 +1801,118 @@ static bool
> vtd_process_iotlb_desc(IntelIOMMUState
> > > *s, VTDInvDesc *inv_desc)
> > > >      return true;
> > > >  }
> > > >
> > > > +static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState
> *s,
> > > > +                                              uint32_t pasid)
> > > > +{
> > > > +    VTDPASIDAddressSpace *vtd_pasid_as = NULL;
> > > > +    IntelPASIDNode *node;
> > > > +    char name[128];
> > > > +
> > > > +    QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> > > > +        vtd_pasid_as = node->pasid_as;
> > > > +        if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> > > > +            return vtd_pasid_as;
> > > > +        }
> > > > +    }
> > >
> > > This seems to be a per-iommu pasid table.  However from the spec it
> > > looks more like that should be per-domain (I'm seeing figure 3-8).
> > > For example, each domain should be able to have its own pasid table.
> > > Then IIUC a pasid context will need a (domain, pasid) tuple to
> > > identify, not only the pasid itself?
> >
> > Yes, this is a per-iommu table here. Actually, how we assemble the
> > table here depends on the PASID namespace. You may refer to the
> > iommu driver code. intel-svm.c, it's actually per-iommu.
> >
> >             /* Do not use PASID 0 in caching mode (virtualised IOMMU)
> */
> >             ret = idr_alloc(&iommu->pasid_idr, svm,
> >                             !!cap_caching_mode(iommu->cap),
> >                             pasid_max - 1, GFP_KERNEL);
> 
> Thanks for the pointer.
> 
> However from the spec, I see that PASID table pointer is per-context,
> IMHO which means that the spec will allow the PASID table to be
> different from one device to another.  Even if current Linux is
> sharing a single PASID table now, I don't know whether that can be
> expanded in the future.  Also, what if we run a guest with another OS
> besides Linux?
> 
> After all we are emulating the device, so IIUC the only thing we
> should follow is the spec.
> 
> >
> > >
> > > And, do we need to destroy the pasid context after it's freed by the
> > > guest?  Here it seems that we'll cache it forever.
> >
> > If we need to do it. A PASID can be bind to multiple devices. If there
> > is no device binding on it, then needs to destroy it. This may be done
> > by refcount. As I mentioned in the description, that requires further
> > vIOMMU emulation, so I didn't include it. But it should be covered
> > in final version. Good catch.
> >
> > >
> > > > +
> > > > +    vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> > > > +    vtd_pasid_as->iommu_state = s;
> > > > +    snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> > > > +    address_space_init(&vtd_pasid_as->as, NULL, "pasid");
> > >
> > > I saw that this is only inited and never used.  Could I ask when this
> > > will be used?
> >
> > AddressSpace is actually introduced for future support of emulated
> > SVA capable devices and possible 1st level paging shadowing(similar
> > to the 2nd level page table shadowing you upstreamed).
> 
> I am not sure whether that can be useful even if there will be such a
> device.  The reason is that if you see current with-IOMMU guests, they
> are actually "somehow" bypassing the address space framework by
> calling the IOMMU MR's translate() to do the page walking. If there
> will be an emulated device that (for example) supports PASID, and with
> the 1st page table enabled, I think it'll also work naturally with
> current translate() interface, just that in the VT-d code we'll find
> that we'll need to walk a process page table this time rather than the
> IOMMU device page table.
> 
> And no matter what, I would prefer you drop this address space until
> it'll be firstly used.
> 
> >
> > >
> > > > +    QLIST_INIT(&vtd_pasid_as->device_list);
> > > > +
> > > > +    node = g_malloc0(sizeof(*node));
> > > > +    node->pasid_as = vtd_pasid_as;
> > > > +    QLIST_INSERT_HEAD(&s->pasid_as_list, node, next);
> > > > +
> > > > +    return vtd_pasid_as;
> > > > +}
> > > > +
> > > > +static void vtd_bind_device_to_pasid_as(VTDPASIDAddressSpace
> *vtd_pasid_as,
> > > > +                                        PCIBus *bus, uint8_t devfn)
> > > > +{
> > > > +    VTDDeviceNode *node = NULL;
> > > > +
> > > > +    QLIST_FOREACH(node, &(vtd_pasid_as->device_list), next) {
> > > > +        if (node->bus == bus && node->devfn == devfn) {
> > > > +            return;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    node = g_malloc0(sizeof(*node));
> > > > +    node->bus = bus;
> > > > +    node->devfn = devfn;
> > > > +    QLIST_INSERT_HEAD(&(vtd_pasid_as->device_list), node, next);
> > >
> > > So here I have the same confusion - IIUC according to the spec two
> > > devices can have differnet pasid tables, however they can be sharing
> > > the same PASID number (e.g., pasid=1) in the table.
> >
> > Do you mean the pasid table in iommu driver? I can not say it is
> impossible,
> > but you may notice that in current iommu driver, the devices behind a
> single
> > iommu unit shared pasid table.
> >
> > > Here since
> > > vtd_pasid_as is only per-IOMMU, could it possible that we put multiple
> > > devices under same PASID context while actually they are not sharing
> > > the same process page table?  Problematic?
> >
> > You are correct, two devices may under same PASID context. For the case
> > you described, I don't think it is allowed as it breaks the PASID concept.
> > Software should avoid it.
> 
> Yeh, so here my question would be the same as above: is it following
> the spec that all devices _must_ share a PASID table between devices,
> or it is just that we _can_ share it as a first version of Linux SVA
> implementation?
> 

the spec defines PASID table as per device. Software may decide whether
to share PASID table between devices based on its needs (e.g. with kernel
drivers sharing PASID table can reduce footprint, but with user space drivers
then per-device PASID table is necessary to ensure isolation). VT-d emulation
code shouldn't stick to one specific software usage here...

Thanks
Kevin