> From: Peter Xu [mailto:pet...@redhat.com]
> 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 <yi.l....@linux.intel.com>
> > ---
> > 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
