RE: [PATCH v2 13/22] intel_iommu: add PASID cache management infrastructure

[Liu, Yi L]

> From: Peter Xu <address@hidden>
> Sent: Thursday, April 2, 2020 9:45 PM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [PATCH v2 13/22] intel_iommu: add PASID cache management
> infrastructure
> 
> On Thu, Apr 02, 2020 at 06:46:11AM +0000, Liu, Yi L wrote:
> 
> [...]
> 
> > > > +/**
> > > > + * This function replay the guest pasid bindings to hots by
> > > > + * walking the guest PASID table. This ensures host will have
> > > > + * latest guest pasid bindings. Caller should hold iommu_lock.
> > > > + */
> > > > +static void vtd_replay_guest_pasid_bindings(IntelIOMMUState *s,
> > > > +                                            VTDPASIDCacheInfo
> > > > +*pc_info) {
> > > > +    VTDHostIOMMUContext *vtd_dev_icx;
> > > > +    int start = 0, end = VTD_HPASID_MAX;
> > > > +    vtd_pasid_table_walk_info walk_info = {.flags = 0};
> > >
> > > So vtd_pasid_table_walk_info is still used.  I thought we had
> > > reached a consensus that this can be dropped?
> >
> > yeah, I did have considered your suggestion and plan to do it. But
> > when I started coding, it looks a little bit weird to me:
> > For one, there is an input VTDPASIDCacheInfo in this function. It may
> > be nature to think about passing the parameter to further calling
> > (vtd_replay_pasid_bind_for_dev()). But, we can't do that. The
> > vtd_bus/devfn fields should be filled when looping the assigned
> > devices, not the one passed by vtd_replay_guest_pasid_bindings() caller.
> 
> Hacky way is we can directly modify VTDPASIDCacheInfo* with bus/devfn for the
> loop.  Otherwise we can duplicate the object when looping, so that we can 
> avoid
> introducing a new struct which seems to contain mostly the same information.

I see. Please see below reply.

> > For two, reusing the VTDPASIDCacheInfo for passing walk info may
> > require the final user do the same thing as what the
> > vtd_replay_guest_pasid_bindings() has done here.
> 
> I don't see it happen, could you explain?

my concern is around flags field in VTDPASIDCacheInfo. The flags not
only indicates the invalidation granularity, but also indicates the
field presence. e.g. VTD_PASID_CACHE_DEVSI indicates the vtd_bus/devfn
fields are valid. If reuse it to pass walk info to vtd_sm_pasid_table_walk_one,
it would be meaningless as vtd_bus/devfn fields are always valid. But
I'm fine to reuse it's more prefered. Instead of modifying the vtd_bus/devn
in VTDPASIDCacheInfo*, I'd rather to define another VTDPASIDCacheInfo variable
and pass it to vtd_sm_pasid_table_walk_one. This may not affect the future
caller of vtd_replay_guest_pasid_bindings() as vtd_bus/devfn field are not
designed to bring something back to caller.

struct VTDPASIDCacheInfo {
#define VTD_PASID_CACHE_FORCE_RESET    (1ULL << 0)
#define VTD_PASID_CACHE_GLOBAL         (1ULL << 1)
#define VTD_PASID_CACHE_DOMSI          (1ULL << 2)
#define VTD_PASID_CACHE_PASIDSI        (1ULL << 3)
#define VTD_PASID_CACHE_DEVSI          (1ULL << 4)
    uint32_t flags;
    uint16_t domain_id;
    uint32_t pasid;
    VTDBus *vtd_bus;
    uint16_t devfn;
}; 

> >
> > So kept the vtd_pasid_table_walk_info.
> 
> [...]
> 
> > > > +/**
> > > > + * This function syncs the pasid bindings between guest and host.
> > > > + * It includes updating the pasid cache in vIOMMU and updating
> > > > +the
> > > > + * pasid bindings per guest's latest pasid entry presence.
> > > > + */
> > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
> > > > +                                 VTDPASIDCacheInfo *pc_info) {
> > > > +    /*
> > > > +     * Regards to a pasid cache invalidation, e.g. a PSI.
> > > > +     * it could be either cases of below:
> > > > +     * a) a present pasid entry moved to non-present
> > > > +     * b) a present pasid entry to be a present entry
> > > > +     * c) a non-present pasid entry moved to present
> > > > +     *
> > > > +     * Different invalidation granularity may affect different device
> > > > +     * scope and pasid scope. But for each invalidation granularity,
> > > > +     * it needs to do two steps to sync host and guest pasid binding.
> > > > +     *
> > > > +     * Here is the handling of a PSI:
> > > > +     * 1) loop all the existing vtd_pasid_as instances to update them
> > > > +     *    according to the latest guest pasid entry in pasid table.
> > > > +     *    this will make sure affected existing vtd_pasid_as instances
> > > > +     *    cached the latest pasid entries. Also, during the loop, the
> > > > +     *    host should be notified if needed. e.g. pasid unbind or pasid
> > > > +     *    update. Should be able to cover case a) and case b).
> > > > +     *
> > > > +     * 2) loop all devices to cover case c)
> > > > +     *    - For devices which have HostIOMMUContext instances,
> > > > +     *      we loop them and check if guest pasid entry exists. If yes,
> > > > +     *      it is case c), we update the pasid cache and also notify
> > > > +     *      host.
> > > > +     *    - For devices which have no HostIOMMUContext, it is not
> > > > +     *      necessary to create pasid cache at this phase since it
> > > > +     *      could be created when vIOMMU does DMA address translation.
> > > > +     *      This is not yet implemented since there is no emulated
> > > > +     *      pasid-capable devices today. If we have such devices in
> > > > +     *      future, the pasid cache shall be created there.
> > > > +     * Other granularity follow the same steps, just with different 
> > > > scope
> > > > +     *
> > > > +     */
> > > > +
> > > > +    vtd_iommu_lock(s);
> > > > +    /* Step 1: loop all the exisitng vtd_pasid_as instances */
> > > > +    g_hash_table_foreach_remove(s->vtd_pasid_as,
> > > > +                                vtd_flush_pasid, pc_info);
> > >
> > > OK the series is evolving along with our discussions, and /me too on
> > > understanding your series... Now I'm not very sure whether this operation 
> > > is still
> useful...
> > >
> > > The major point is you'll need to do pasid table walk for all the
> > > registered devices below.  So IIUC vtd_replay_guest_pasid_bindings()
> > > will be able to also detect addition, removal or modification of
> > > pasid address spaces.  Am I right?
> >
> > It's true if there is only assigned pasid-capable devices. If there is
> > emualted pasid-capable device, it would be a problem as emualted
> > devices won't register HostIOMMUContext. Somehow, the pasid cahce
> > invalidation for emualted device would be missed. So I chose to make
> > the step 1 cover the "real" cache invalidation(a.k.a. removal), while
> > step 2 to cover addition and modification.
> 
> OK.  Btw, I think modification should still belongs to step 1 then (I think 
> you're doing
> that, though).

Oh, yes, modification is done in step 1... step 2 is only for addition.

Regards,
Yi Liu