> From: Peter Xu <peterx@xxxxxxxxxx>
> Sent: Wednesday, March 25, 2020 11:07 PM
> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Cc: qemu-devel@xxxxxxxxxx; alex.williamson@xxxxxxxxxx;
> eric.auger@xxxxxxxxxx; pbonzini@xxxxxxxxxx; mst@xxxxxxxxxx;
> david@xxxxxxxxxxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; Tian, Jun J
> <jun.j.tian@xxxxxxxxx>; Sun, Yi Y <yi.y.sun@xxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; Wu,
> Hao <hao.wu@xxxxxxxxx>; jean-philippe@xxxxxxxxxx; Jacob Pan
> <jacob.jun.pan@xxxxxxxxxxxxxxx>; Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>; Richard
> Henderson <rth@xxxxxxxxxxx>
> 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
