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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan