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

[Peter Xu <peterx@redhat.com>]

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.

> 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?

> 
> 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).

Thanks,

-- 
Peter Xu