Re: [PATCH] xen: arch_mfn_in_directmap: Reconciliate the name and the implementation

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 16/12/2021 16:11, Jan Beulich wrote:
> On 16.12.2021 16:22, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > The name of arch_mfn_in_directmap() suggests that it will check against
> > that the passed MFN should be in the directmap.
> >
> > However, the current callers are passing the next MFN and the
> > implementation will return true for up to one MFN past the directmap.
> >
> > It would be more meaningful to test the exact MFN rather than the
> > next one. So rework the implementation and the caller to match the
> > name.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ---
> >
> > Looking at the history, it looks like the check in init_node_heap()
> > was <= and it was simply moved to a new helper without any adjustment
> > as part of c6fdc9696a "boot allocator: use arch helper for virt_to_mfn
> > on DIRECTMAP_VIRT region".
> Really the original intention was to check the entire range; maybe it
> would be better to express this again by ...
>
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -641,7 +641,7 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
> >   {
> >       unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
> >   
> > -    return mfn <= (virt_to_mfn(eva - 1) + 1);
> > +    return mfn < (virt_to_mfn(eva - 1) + 1);
> >   }
> ... this instead:
>
> static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
> {
>      unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>
>      return mfn + nr <= (virt_to_mfn(eva - 1) + 1);
> }
>
> This would allow an arch hook to also go beyond verifying just the last
> MFN.
I am fine with this approach. I will respin the patch.

Cheers,

--
Julien Grall