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
