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

[Peter Xu]

On Fri, Apr 03, 2020 at 03:05:57PM +0000, Liu, Yi L wrote:
> > 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.

Yeah, let's give it a shot.  I know it's not ideal, but IMHO it's
still better than defining the page_walk struct and that might confuse
readers on what's the difference between the two.  When duplicating
the object, we can add some comment explaining this.

Thanks,

-- 
Peter Xu