> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On Behalf > Of Peter Xu > Sent: Tuesday, July 9, 2019 2:39 PM > To: Liu, Yi L <yi.l.liu@xxxxxxxxx> > 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
