> From: Peter Xu <pet...@redhat.com> > Sent: Wednesday, March 25, 2020 11:07 PM > To: Liu, Yi L <yi.l....@intel.com> > Cc: qemu-devel@nongnu.org; alex.william...@redhat.com; > eric.au...@redhat.com; pbonz...@redhat.com; m...@redhat.com; > da...@gibson.dropbear.id.au; Tian, Kevin <kevin.t...@intel.com>; Tian, Jun J > <jun.j.t...@intel.com>; Sun, Yi Y <yi.y....@intel.com>; k...@vger.kernel.org; > Wu, > Hao <hao...@intel.com>; jean-phili...@linaro.org; Jacob Pan > <jacob.jun....@linux.intel.com>; Yi Sun <yi.y....@linux.intel.com>; Richard > Henderson <r...@twiddle.net> > Subject: Re: [PATCH v1 15/22] intel_iommu: replay guest pasid bindings to host > > On Wed, Mar 25, 2020 at 01:14:26PM +0000, Liu, Yi L wrote: > > [...] > > > > > +/** > > > > + * Caller of this function should hold iommu_lock. > > > > + */ > > > > +static bool vtd_sm_pasid_table_walk_one(IntelIOMMUState *s, > > > > + dma_addr_t pt_base, > > > > + int start, > > > > + int end, > > > > + vtd_pasid_table_walk_info > > > > *info) > > > > +{ > > > > + VTDPASIDEntry pe; > > > > + int pasid = start; > > > > + int pasid_next; > > > > + VTDPASIDAddressSpace *vtd_pasid_as; > > > > + VTDPASIDCacheEntry *pc_entry; > > > > + > > > > + while (pasid < end) { > > > > + pasid_next = pasid + 1; > > > > + > > > > + if (!vtd_get_pe_in_pasid_leaf_table(s, pasid, pt_base, &pe) > > > > + && vtd_pe_present(&pe)) { > > > > + vtd_pasid_as = vtd_add_find_pasid_as(s, > > > > + info->vtd_bus, info->devfn, > > > > pasid); > > > > > > For this chunk: > > > > > > > + pc_entry = &vtd_pasid_as->pasid_cache_entry; > > > > + if (s->pasid_cache_gen == pc_entry->pasid_cache_gen) { > > > > + vtd_update_pe_in_cache(s, vtd_pasid_as, &pe); > > > > + } else { > > > > + vtd_fill_in_pe_in_cache(s, vtd_pasid_as, &pe); > > > > + } > > > > > > We already got &pe, then would it be easier to simply call: > > > > > > vtd_update_pe_in_cache(s, vtd_pasid_as, &pe); > > > > > > ? > > > > If the pasid_cache_gen is equal to iommu_state's, then it means there is > > a chance that the cached pasid entry is equal to pe here. While for the > > else case, it is surely there is no valid pasid entry in the pasid_as. And > > the difference between vtd_update_pe_in_cache() and > > vtd_fill_in_pe_in_cache() is whether do a compare between the new pasid > > entry > > and cached pasid entry. > > > > > Since IIUC the cache_gen is only helpful to avoid looking up the &pe. > > > And the vtd_pasid_entry_compare() check should be even more solid than > > > the cache_gen. > > > > But if the cache_gen is not equal the one in iommu_state, then the cached > > pasid entry is not valid at all. The compare is only needed after the > > cache_gen > > is checked. > > Wait... If "the pasid entry context" is already exactly the same as > the "cached pasid entry context", why we still care the generation > number? I'd just update the generation to latest and cache it again. > Maybe there's a tricky point when &pe==cache but generation number is > old, then IIUC what we can do better is simply update the generation > number to latest. > > But OK - let's keep that. I don't see anything incorrect with current > code either.
I see. Actually, I think it's also fine to follow your suggestion to all vtd_update_pe_in_cache(s, vtd_pasid_as, &pe); for the else switch. If switch to use replay for PSI, then I may drop vtd_fill_in_pe_in_cache() as it is introduced mainly for PSI. Regards, Yi Liu