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

[Liu, Yi L]

> From: Peter Xu [mailto:address@hidden
> Sent: Friday, March 9, 2018 3:59 PM
> Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID tagged
> AddressSpace
> 
> 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.

Agree. just echo Kevin's reply here. Let' me re-consider a way to maintain all 
the
PASID tagged address space here.

> 
> >
> > >
> > > 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.

yeah, I would. May add parameter like pasid in the existing MR translate() 
interface
to meet the requirement.

> >
> > >
> > > > +    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?

Thanks,
Yi Liu