[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.