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

[Liu, Yi L]

Hi Peter,

> From: Peter Xu <address@hidden>
> Sent: Saturday, April 4, 2020 12:20 AM
> To: Liu, Yi L <address@hidden>
> 
> 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.

got it. I'll drop the page_walk struct and add additional comments. :-)

Regards,
Yi Liu