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

[Julien Grall <julien@xxxxxxx>]

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:
> > > In order for individual IOMMU drivers (and from an abstract pov also
> > > architectures) to be able to adjust, ahead of actual mapping requests,
> > > their data structures when they might cover only a sub-range of all
> > > possible GFNs, introduce a notification call used by various code paths
> > > potentially installing a fresh mapping of a never used GFN (for a
> > > particular domain).
> > If I understand this correctly, this is mostly targeting IOMMNU driver
> > where page-table are not shared with the processor. Right?
> Yes - with shared page tables there's no separate handling of
> IOMMU (un)mappings in the first place.
>
> > > 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.
> > > --- 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)?
Cheers,

--
Julien Grall

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