Re: [Xen-devel] [PATCH v2 1/2] introduce GFN notification for translated domains

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 21/11/2019 10:41, Jan Beulich wrote:
> On 21.11.2019 11:07, Julien Grall wrote:
> > On 21/11/2019 09:04, Jan Beulich wrote:
> > > On 20.11.2019 21:22, Julien Grall wrote:
> > > > On 14/11/2019 16:43, Jan Beulich wrote:
> > > > > TBD: Does Arm actually have anything to check against in its
> > > > >         arch_notify_gfn()?
> > > > I understand that we want to keep the code mostly generic, but I am a
> > > > bit concerned of the extra cost to use notify_gfn() (and indirectly
> > > > iommu_notify_gfn()) for doing nothing.
> > > >
> > > > I can't see any direct use of this for the foreseable future on Arm. So
> > > > could we gate this under a config option?
> > > This is an option, sure. Alternatively I could see about making this
> > > an inline function, but iirc there were header dependency issues.
> > > Then again - is a call to a function doing almost nothing really so
> > > much extra overhead on Arm.
> > AFAICT, this is a workaround for AMD driver. So any impact (no matter
> > the size) feels not right for Arm.
> >
> > In this particular case, the only thing I request is to protect the
> > notify_gfn & callback with !CONFIG_IOMMU_FORCE_SHARE.
> Oh, there already is a suitable config option. Will do (and
> cover share_p2m_table() at the same time).
>
> > > > > --- a/xen/common/grant_table.c
> > > > > +++ b/xen/common/grant_table.c
> > > > > @@ -946,6 +946,16 @@ map_grant_ref(
> > > > >             return;
> > > > >         }
> > > > >     
> > > > > +    if ( paging_mode_translate(ld) /* && (op->flags & GNTMAP_host_map) */ &&
> > > > I think this wants an explanation in the code why the check is commented.
> > > Hmm, in such a case I'd rather omit the commented condition. It
> > > being commented has the purpose of documenting itself.
> > I fail to understand why GNTMAP_host_map would always be true in the case.
> >
> > AFAIU the code, this is only correct for paging_mode_external(ld) == 1.
> > Does it mean that paging_mode_translate(ld) and paging_mode_external(ld)
> > are always equal? If so, what's the point of having two macro (and two
> > flags)?
> Historical reasons. Nowadays translate == external == refcounts on
> x86. But since this is common code, perhaps I better un-comment that
> part of the conditional.
For this patch, this would be the ideal solution.

We might want to consider to reduce to one macro (maybe 
paging_mode_translate()) if we don't expect new architecture to return a 
different value for those 3 macros.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel