[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/common: Do not allocate magic pages 1:1 for direct mapped domains
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |