From: Julien Grall <jgrall@amazon.com>

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@amazon.com>

---

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".
---
 xen/arch/x86/include/asm/mm.h | 2 +-
 xen/common/page_alloc.c       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index 5dbcee869624..a38e13e3c11e 100644
--- 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);
 }
 
 #endif /* __ASM_X86_MM_H__ */
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 38eea879c04a..d4789f81cd31 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -588,7 +588,7 @@ static unsigned long init_node_heap(int node, unsigned long mfn,
         needed = 0;
     }
     else if ( *use_tail && nr >= needed &&
-              arch_mfn_in_directmap(mfn + nr) &&
+              arch_mfn_in_directmap(mfn + nr - 1) &&
               (!xenheap_bits ||
                !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
     {
@@ -597,7 +597,7 @@ static unsigned long init_node_heap(int node, unsigned long mfn,
                       PAGE_SIZE - sizeof(**avail) * NR_ZONES;
     }
     else if ( nr >= needed &&
-              arch_mfn_in_directmap(mfn + needed) &&
+              arch_mfn_in_directmap(mfn + needed - 1) &&
               (!xenheap_bits ||
                !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
     {
-- 
2.32.0

On 16.12.2021 16:22, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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@amazon.com>
> 
> ---
> 
> 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

Hi Jan,

On 16/12/2021 16:11, Jan Beulich wrote:
> On 16.12.2021 16:22, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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@amazon.com>
>>
>> ---
>>
>> 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