Re: [Qemu-devel] [RFC v1 11/18] intel_iommu: create VTDAddressSpace per BDF+PASID

[Liu, Yi L]

> From: address@hidden [mailto:address@hidden] On Behalf
> Of Peter Xu
> Sent: Tuesday, July 9, 2019 2:39 PM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v1 11/18] intel_iommu: create VTDAddressSpace per BDF+PASID
> 
> On Fri, Jul 05, 2019 at 07:01:44PM +0800, Liu Yi L wrote:
> 
> [...]
> 
> > +/**
> > + * This function finds or adds a VTDAddressSpace for a device when
> > + * it is bound to a pasid
> > + */
> > +static VTDAddressSpace *vtd_add_find_pasid_as(IntelIOMMUState *s,
> > +                                              PCIBus *bus,
> > +                                              int devfn,
> > +                                              uint32_t pasid,
> > +                                              bool allocate)
> > +{
> > +    char key[32];
> > +    char *new_key;
> > +    VTDAddressSpace *vtd_pasid_as;
> > +    uint16_t sid;
> > +
> > +    sid = vtd_make_source_id(pci_bus_num(bus), devfn);
> > +    vtd_get_pasid_key(&key[0], 32, pasid, sid);
> > +    vtd_pasid_as = g_hash_table_lookup(s->vtd_pasid_as, &key[0]);
> > +
> > +    if (!vtd_pasid_as && allocate) {
> > +        new_key = g_malloc(32);
> > +        vtd_get_pasid_key(&new_key[0], 32, pasid, sid);
> > +        /*
> > +         * Initiate the vtd_pasid_as structure.
> > +         *
> > +         * This structure here is used to track the guest pasid
> > +         * binding and also serves as pasid-cache mangement entry.
> > +         *
> > +         * TODO: in future, if wants to support the SVA-aware DMA
> > +         *       emulation, the vtd_pasid_as should be fully initialized.
> > +         *       e.g. the address_space and memory region fields.
> > +         */
> 
> I'm not very sure about this part.  IMHO all those memory regions are
> used to inlay the whole IOMMU idea into QEMU's memory API framework.
> Now even without the whole PASID support we've already have a workable
> vtd_iommu_translate() that will intercept device DMA operations and we
> can try to translate the IOVA to anything we want.  Now the iommu_idx
> parameter of vtd_iommu_translate() is never used (I'd say until now I
> still don't sure on whether the "iommu_idx" idea is the best we can
> have... I've tried to debate on that but... anyway I assume for Intel
> we can think it as the "pasid" information or at least contains it),
> however in the further we can have that PASID/iommu_idx/whatever
> passed into this translate() function too, then we can walk the 1st
> level page table there if we found that this device had enabled the
> 1st level mapping (or even nested).  I don't see what else we need to
> do to play with extra memory regions.

Not sure if passing a PASID to translate() function is good since we may
need to pass PASID parameter through all the QEMU AddressSpace read/
write stack.

Actually, I did some experiment with a simple emulated SVA-capable device
some time ago (no iommu_idx at that time). Per my understanding, a
SVA capable device model needs to fetch an AddressSpace with a PASID
and then call dma_memory_rw() which will invoke the QEMU AddressSpace
read/write stack, then finally call into vtd_iommu_translate(), and in
translate() we can get the VTDAddressSpace instance and it has a flag
"pasid_allocated". If it is true, translate the input address with page table
behind the PASID from the "pasid" field in VTDAddressSpace. I guess this
may introduce the least changes to existing logic.

> 
> Conclusion: I feel like SVA can use its own structure here instead of
> reusing VTDAddressSpace, because I think those memory regions can
> probably be useless.  Even it will, we can refactor the code later,
> but I really doubt it...

Hmmm, right, even necessary, SVA will require less memory regions. I can
switch to use a structure named VTDPASIDAddressSpace or alike.

Thanks,
Yi Liu

> > +        vtd_pasid_as = g_malloc0(sizeof(VTDAddressSpace));
> > +        vtd_pasid_as->iommu_state = s;
> > +        vtd_pasid_as->bus = bus;
> > +        vtd_pasid_as->devfn = devfn;
> > +        vtd_pasid_as->context_cache_entry.context_cache_gen = 0;
> > +        vtd_pasid_as->pasid = pasid;
> > +        vtd_pasid_as->pasid_allocated = true;
> > +        vtd_pasid_as->pasid_cache_entry.pasid_cache_gen = 0;
> > +        g_hash_table_insert(s->vtd_pasid_as, new_key, vtd_pasid_as);
> > +    }
> > +    return vtd_pasid_as;
> > +}
> 
> Regards,
> 
> --
> Peter Xu