Hi Jan,
On 2/27/2024 9:51 PM, Jan Beulich wrote:
On 27.02.2024 14:35, Henry Wang wrote:
Hi Jan,
On 2/27/2024 9:27 PM, Jan Beulich wrote:
On 27.02.2024 14:24, Henry Wang wrote:
On 2/26/2024 4:25 PM, Jan Beulich wrote:
On 26.02.2024 02:19, Henry Wang wrote:
--- 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?
arch #ifdefs? I'd like to avoid such, if at all possible. Generic fallbacks
generally ought to key their conditionals to the very identifier not
(already) being defined.
I meant something like this (as I saw CDF_directmap is currently gated
in this way in xen/domain.h):
#ifdef CONFIG_ARM
#define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
#else
#define is_domain_direct_mapped(d) ((void)(d), 0)
#endif
I am having trouble to think of another way to keep the function in a
centralized place while
avoiding the #ifdefs. Would you mind elaborating a bit? Thanks!
What is already there is fine to change. I took your earlier reply to
mean that you want to add an "#ifndef CONFIG_ARM" to put in place some
new fallback handler.
Of course the above could also be done without any direct CONFIG_ARM
dependency. For example, CDF_directmap could simply evaluate to zero
when direct mapped memory isn't supported.
Yes correct, that will indeed simplify the code a lot. I can do the
change accordingly in follow-up versions.
Kind regards,
Henry
Jan
