Hi Jan,
On 2/26/2024 4:25 PM, Jan Beulich wrote:
On 26.02.2024 02:19, Henry Wang wrote:
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -428,6 +428,19 @@ static inline void page_set_xenheap_gfn(struct page_info
*p, gfn_t gfn)
} while ( (y = cmpxchg(&p->u.inuse.type_info, x, nx)) != x );
}
+#define MAGIC_PAGE_N_GPFN(n) ((GUEST_MAGIC_BASE >> PAGE_SHIFT) + n)
+static inline bool is_magic_gpfn(xen_pfn_t gpfn)
+{
+ unsigned int i;
+ for ( i = 0; i < NR_MAGIC_PAGES; i++ )
Nit: Blank line please between declaration(s) and statement(s).
Thanks, will correct in next version.
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -628,4 +628,9 @@ static inline bool arch_mfns_in_directmap(unsigned long
mfn, unsigned long nr)
return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1);
}
+static inline bool is_magic_gpfn(xen_pfn_t gpfn)
+{
+ return false;
+}
I don't think every arch should need to gain such a dummy function.
Thanks for raising the concern here and about the
is_domain_direct_mapped(), I will try to do some clean-ups if necessary
in next version.
Plus
the function name doesn't clarify at all what kind of "magic" this is
about. Plus I think the (being phased out) term "gpfn" would better not
be used in new functions anymore. Instead type-safe gfn_t would likely
better be used as parameter type.
Sure, I will use gfn_t in the next version.
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -219,7 +219,7 @@ static void populate_physmap(struct memop_args *a)
}
else
{
- if ( is_domain_direct_mapped(d) )
+ if ( is_domain_direct_mapped(d) && !is_magic_gpfn(gpfn) )
{
mfn = _mfn(gpfn);
I wonder whether is_domain_direct_mapped() shouldn't either be cloned
into e.g. is_gfn_direct_mapped(d, gfn), or be adjusted in-place to gain
such a (then optional) 2nd parameter. (Of course there again shouldn't be
a need for every domain to define a stub is_domain_direct_mapped() - if
not defined by an arch header, the stub can be supplied in a single
central place.)
Same here, it looks like you prefer the centralized
is_domain_direct_mapped() now, as we are having more archs. I can do the
clean-up when sending v2. Just out of curiosity, do you think it is a
good practice to place the is_domain_direct_mapped() implementation in
xen/domain.h with proper arch #ifdefs? If not do you have any better
ideas? Thanks!
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -476,6 +476,12 @@ typedef uint64_t xen_callback_t;
#define GUEST_MAGIC_BASE xen_mk_ullong(0x39000000)
#define GUEST_MAGIC_SIZE xen_mk_ullong(0x01000000)
+#define NR_MAGIC_PAGES 4
+#define CONSOLE_PFN_OFFSET 0
+#define XENSTORE_PFN_OFFSET 1
+#define MEMACCESS_PFN_OFFSET 2
+#define VUART_PFN_OFFSET 3
+
#define GUEST_RAM_BANKS 2
Of these only NR_MAGIC_PAGES is really used in Xen, afaics.
Also while this is added to a tools-only section, I'm also concerned of
the ongoing additions here without suitable XEN_ prefixes. Any number
of kinds of magic pages may exist for other reasons in a platform; which
ones are meant would therefore better be sufficiently clear from the
identifier used.
Yes you are correct, like I replied in another thread, I will undo the
changes in next version.
Kind regards,
Henry
Jan
