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 2:00 AM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [PATCH v1 15/22] intel_iommu: replay guest pasid bindings to host
> 
> On Sun, Mar 22, 2020 at 05:36:12AM -0700, Liu Yi L wrote:
> > This patch adds guest pasid bindings replay for domain
> > selective pasid cache invalidation(dsi) and global pasid
> > cache invalidation by walking guest pasid table.
> >
> > Reason:
> > Guest OS may flush the pasid cache with a larger granularity.
> > e.g. guest does a svm_bind() but flush the pasid cache with
> > global or domain selective pasid cache invalidation instead
> > of pasid selective(psi) pasid cache invalidation. Regards to
> > such case, it works in host. Per spec, a global or domain
> > selective pasid cache invalidation should be able to cover
> > what a pasid selective invalidation does. The only concern
> > is performance deduction since dsi and global cache invalidation
> > will flush more than psi. To align with native, vIOMMU needs
> > emulator needs to do replay for the two invalidation granularity
> > to reflect the latest pasid bindings in guest pasid table.
> 
> This is actually related to my question in the other patch on whether
> the replay can and should also work for the PSI case too.  I'm still
> confused on why the guest cannot use a PSI for a newly created PASID
> entry for one device?

Use a PSI for a newly created PASID entry for one device is the correct
way. But spec doesn't include the device info in the invalidation descriptor.
Reason is there is DID info which is enough.

I think the replay code and the PSI code should be designed with
the same idea. With a Step 1 loop all existing pasid_as, and Step 2
to loop all assigned devices. I'll try to make them share the low
level code. e.g. the most code in PSI handling.

> 
> >
> > Cc: Kevin Tian <address@hidden>
> > Cc: Jacob Pan <address@hidden>
> > Cc: Peter Xu <address@hidden>
> > Cc: Yi Sun <address@hidden>
> > Cc: Paolo Bonzini <address@hidden>
> > Cc: Richard Henderson <address@hidden>
> > Signed-off-by: Liu Yi L <address@hidden>
> > ---
> >  hw/i386/intel_iommu.c          | 128
> ++++++++++++++++++++++++++++++++++++++++-
> >  hw/i386/intel_iommu_internal.h |   1 +
> >  2 files changed, 127 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 0423c83..8ec638f 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2717,6 +2717,130 @@ static VTDPASIDAddressSpace
> *vtd_add_find_pasid_as(IntelIOMMUState *s,
> >      return vtd_pasid_as;
> >  }
> >
> > +/**
> > + * Constant information used during pasid table walk
> > +   @vtd_bus, @devfn: device info
> > + * @flags: indicates if it is domain selective walk
> > + * @did: domain ID of the pasid table walk
> > + */
> > +typedef struct {
> > +    VTDBus *vtd_bus;
> > +    uint16_t devfn;
> > +#define VTD_PASID_TABLE_DID_SEL_WALK   (1ULL << 0);
> > +    uint32_t flags;
> > +    uint16_t did;
> > +} vtd_pasid_table_walk_info;
> 
> So this is going to be similar to VTDPASIDCacheInfo as I mentioned.
> Maybe you can use a shared object for both?

Aha, similar. high chance to reuse for both. :-)

> > +
> > +/**
> > + * 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.

> > +        }
> > +        pasid = pasid_next;
> > +    }
> > +    return true;
> > +}
> > +
> > +/*
> > + * Currently, VT-d scalable mode pasid table is a two level table,
> > + * this function aims to loop a range of PASIDs in a given pasid
> > + * table to identify the pasid config in guest.
> > + * Caller of this function should hold iommu_lock.
> > + */
> > +static void vtd_sm_pasid_table_walk(IntelIOMMUState *s,
> > +                                    dma_addr_t pdt_base,
> > +                                    int start,
> > +                                    int end,
> > +                                    vtd_pasid_table_walk_info *info)
> > +{
> > +    VTDPASIDDirEntry pdire;
> > +    int pasid = start;
> > +    int pasid_next;
> > +    dma_addr_t pt_base;
> > +
> > +    while (pasid < end) {
> > +        pasid_next = pasid + VTD_PASID_TBL_ENTRY_NUM;
> > +        if (!vtd_get_pdire_from_pdir_table(pdt_base, pasid, &pdire)
> > +            && vtd_pdire_present(&pdire)) {
> > +            pt_base = pdire.val & VTD_PASID_TABLE_BASE_ADDR_MASK;
> > +            if (!vtd_sm_pasid_table_walk_one(s,
> > +                              pt_base, pasid, pasid_next, info)) {
> 
> vtd_sm_pasid_table_walk_one() never returns false.  Remove this check?
> Maybe also let vtd_sm_pasid_table_walk_one() to return nothing.

Oops. Could make it as void.

Regards,
Yi Liu