RE: [PATCH v1 15/22] intel_iommu: replay guest pasid bindings to host

[Liu, Yi L]

> From: Peter Xu <address@hidden>
> Sent: Wednesday, March 25, 2020 11:07 PM
> To: Liu, Yi L <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; Tian, Kevin <address@hidden>; Tian, Jun J
> <address@hidden>; Sun, Yi Y <address@hidden>; address@hidden; Wu,
> Hao <address@hidden>; address@hidden; Jacob Pan
> <address@hidden>; Yi Sun <address@hidden>; Richard
> Henderson <address@hidden>
> 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