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: 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);

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

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

Does it make sense?

> 
> Please correct me if needed.
> 
> > +
> > +    pci_device_sva_register_notifier(bus, devfn, &vtd_pasid_as->sva_ctx);
> > +
> > +    return;
> > +}
> > +
> > +static bool vtd_process_pc_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
> > +{
> > +
> > +    IntelIOMMUAssignedDeviceNode *node = NULL;
> > +    int ret = 0;
> > +
> > +    uint16_t domain_id;
> > +    uint32_t pasid;
> > +    VTDPASIDAddressSpace *vtd_pasid_as;
> > +
> > +    if ((inv_desc->lo & VTD_INV_DESC_PASIDC_RSVD_LO) ||
> > +        (inv_desc->hi & VTD_INV_DESC_PASIDC_RSVD_HI)) {
> > +        return false;
> > +    }
> > +
> > +    domain_id = VTD_INV_DESC_PASIDC_DID(inv_desc->lo);
> > +
> > +    switch (inv_desc->lo & VTD_INV_DESC_PASIDC_G) {
> > +    case VTD_INV_DESC_PASIDC_ALL_ALL:
> > +        /* TODO: invalidate all pasid related cache */
> 
> I think it's fine as RFC, but we'd better have this in the final
> version?

Definitely.

> 
> IIUC you'll need caching-mode too for virt-sva, and here you'll
> possibly need to walk and scan every context entry that has the same
> domain ID specified in the invalidation request.  Maybe further you'll
> need to scan the pasid entries too, register notifiers when needed.

Sure, should be in the full virt-sva series.

Thanks,
Yi Liu