> From: Peter Xu <peterx@xxxxxxxxxx>
> Sent: Thursday, April 2, 2020 9:45 PM
> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> 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
