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.
--
Peter Xu
