While it is correct that in shim-exclusive mode log-dirty handling is
all unreachable code, the present conditional still isn't correct: In a
HVM=n and SHADOW_PAGING=n configuration log-dirty code also is all
unreachable (and hence violating Misra rule 2.1).
As we're aiming at moving away from special casing PV_SHIM_EXCLUSIVE=y,
don't retain that part of the conditional.
Because of hypercall-defs.c we need to carry out the dependency by
introducing a new auxiliary PAGING control.
Since compiling out mm/paging.c altogether would entail further changes,
merely conditionalize the one function in there (paging_enable()) which
would otherwise remain unreachable (Misra rule 2.1 again) when PAGING=n.
Fixes: 23d4e0d17b76 ("x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course PAGING is at risk of being confused with MEM_PAGING. It not
having a prompt, I hope that's tolerable, as I can't really think of a
better name.
Other PG_log_dirty pre-processor conditionals then likely also want
replacing. mm/paging.c and mm/p2m-basic.c could also be compiled out
altogether when PAGING=n, at the expense of introducing a few more
stubs.
FTAOD, the Fixes: tag being referenced does not mean this patch corrects
the far more recently introduced build issue with the combination of the
two features. That's still work that I expect Penny to carry out (with
there still being the option of reverting the final part of the earlier
series).
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -162,6 +162,9 @@ config SHADOW_PAGING
If unsure, say Y.
+config PAGING
+ def_bool HVM || SHADOW_PAGING
+
config BIGMEM
bool "big memory support"
default n
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -213,11 +213,15 @@ long arch_do_domctl(
{
case XEN_DOMCTL_shadow_op:
+#ifdef CONFIG_PAGING
ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
if ( ret == -ERESTART )
return hypercall_create_continuation(
__HYPERVISOR_paging_domctl_cont, "h", u_domctl);
copyback = true;
+#else
+ ret = -EOPNOTSUPP;
+#endif
break;
case XEN_DOMCTL_ioport_permission:
--- a/xen/arch/x86/include/asm/paging.h
+++ b/xen/arch/x86/include/asm/paging.h
@@ -55,7 +55,7 @@
#define PG_translate 0
#define PG_external 0
#endif
-#if defined(CONFIG_HVM) || !defined(CONFIG_PV_SHIM_EXCLUSIVE)
+#ifdef CONFIG_PAGING
/* Enable log dirty mode */
#define PG_log_dirty (XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY << PG_mode_shift)
#else
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -864,6 +864,7 @@ void paging_final_teardown(struct domain
p2m_final_teardown(d);
}
+#ifdef CONFIG_PAGING
/* Enable an arbitrary paging-assistance mode. Call once at domain
* creation. */
int paging_enable(struct domain *d, u32 mode)
@@ -889,6 +890,7 @@ int paging_enable(struct domain *d, u32
else
return shadow_enable(d, mode);
}
+#endif
#ifdef CONFIG_HVM
/* Called from the guest to indicate that a process is being torn down
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -197,9 +197,11 @@ dm_op(domid_t domid, unsigned int nr_buf
#ifdef CONFIG_SYSCTL
sysctl(xen_sysctl_t *u_sysctl)
#endif
+#if defined(CONFIG_X86) && defined(CONFIG_PAGING)
+paging_domctl_cont(xen_domctl_t *u_domctl)
+#endif
#ifndef CONFIG_PV_SHIM_EXCLUSIVE
domctl(xen_domctl_t *u_domctl)
-paging_domctl_cont(xen_domctl_t *u_domctl)
platform_op(xen_platform_op_t *u_xenpf_op)
#endif
#ifdef CONFIG_HVM
@@ -296,7 +298,7 @@ dm_op compa
hypfs_op do do do do do
#endif
mca do do - - -
-#ifndef CONFIG_PV_SHIM_EXCLUSIVE
+#if defined(CONFIG_X86) && defined(CONFIG_PAGING)
paging_domctl_cont do do do do -
#endifOn 05/08/2025 8:59 am, Jan Beulich wrote:
> While it is correct that in shim-exclusive mode log-dirty handling is
> all unreachable code, the present conditional still isn't correct: In a
> HVM=n and SHADOW_PAGING=n configuration log-dirty code also is all
> unreachable (and hence violating Misra rule 2.1).
>
> As we're aiming at moving away from special casing PV_SHIM_EXCLUSIVE=y,
> don't retain that part of the conditional.
>
> Because of hypercall-defs.c we need to carry out the dependency by
> introducing a new auxiliary PAGING control.
>
> Since compiling out mm/paging.c altogether would entail further changes,
> merely conditionalize the one function in there (paging_enable()) which
> would otherwise remain unreachable (Misra rule 2.1 again) when PAGING=n.
>
> Fixes: 23d4e0d17b76 ("x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Of course PAGING is at risk of being confused with MEM_PAGING. It not
> having a prompt, I hope that's tolerable, as I can't really think of a
> better name.
Honestly, PAGING is a bad name start with. While it is to do with the
handling of pages, it is specifically not what people think of when
using the term paging.
Hence why we've got MEM_PAGING too, which is referring to what most
people think of when told the name paging.
I don't have a better suggestion, given the current codebase, but I'm
open to suggestions.
~Andrew
On 2025-08-05 03:59, Jan Beulich wrote:
> While it is correct that in shim-exclusive mode log-dirty handling is
> all unreachable code, the present conditional still isn't correct: In a
> HVM=n and SHADOW_PAGING=n configuration log-dirty code also is all
> unreachable (and hence violating Misra rule 2.1).
>
> As we're aiming at moving away from special casing PV_SHIM_EXCLUSIVE=y,
> don't retain that part of the conditional.
>
> Because of hypercall-defs.c we need to carry out the dependency by
> introducing a new auxiliary PAGING control.
>
> Since compiling out mm/paging.c altogether would entail further changes,
> merely conditionalize the one function in there (paging_enable()) which
> would otherwise remain unreachable (Misra rule 2.1 again) when PAGING=n.
>
> Fixes: 23d4e0d17b76 ("x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course PAGING is at risk of being confused with MEM_PAGING. It not
> having a prompt, I hope that's tolerable, as I can't really think of a
> better name.
>
> Other PG_log_dirty pre-processor conditionals then likely also want
> replacing. mm/paging.c and mm/p2m-basic.c could also be compiled out
> altogether when PAGING=n, at the expense of introducing a few more
> stubs.
>
> FTAOD, the Fixes: tag being referenced does not mean this patch corrects
> the far more recently introduced build issue with the combination of the
> two features. That's still work that I expect Penny to carry out (with
> there still being the option of reverting the final part of the earlier
> series).
>
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -864,6 +864,7 @@ void paging_final_teardown(struct domain
> p2m_final_teardown(d);
> }
>
> +#ifdef CONFIG_PAGING
The file already has a lot of uses of #if PG_log_dirty with similar
meaning, if I am not mistaken, so using that would make it more
consistent. But CONFIG_PAGING is directly tied to the Kconfig, so maybe
it is better? Just something I noticed.
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Regards,
Jason
> /* Enable an arbitrary paging-assistance mode. Call once at domain
> * creation. */
> int paging_enable(struct domain *d, u32 mode)
> @@ -889,6 +890,7 @@ int paging_enable(struct domain *d, u32
> else
> return shadow_enable(d, mode);
> }
> +#endif
>
> #ifdef CONFIG_HVM
> /* Called from the guest to indicate that a process is being torn down
On 06.08.2025 23:24, Jason Andryuk wrote:
> On 2025-08-05 03:59, Jan Beulich wrote:
>> While it is correct that in shim-exclusive mode log-dirty handling is
>> all unreachable code, the present conditional still isn't correct: In a
>> HVM=n and SHADOW_PAGING=n configuration log-dirty code also is all
>> unreachable (and hence violating Misra rule 2.1).
>>
>> As we're aiming at moving away from special casing PV_SHIM_EXCLUSIVE=y,
>> don't retain that part of the conditional.
>>
>> Because of hypercall-defs.c we need to carry out the dependency by
>> introducing a new auxiliary PAGING control.
>>
>> Since compiling out mm/paging.c altogether would entail further changes,
>> merely conditionalize the one function in there (paging_enable()) which
>> would otherwise remain unreachable (Misra rule 2.1 again) when PAGING=n.
>>
>> Fixes: 23d4e0d17b76 ("x86/shim: fix build with PV_SHIM_EXCLUSIVE and SHADOW_PAGING")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of course PAGING is at risk of being confused with MEM_PAGING. It not
>> having a prompt, I hope that's tolerable, as I can't really think of a
>> better name.
>>
>> Other PG_log_dirty pre-processor conditionals then likely also want
>> replacing.
Isn't this remark of mine ...
>> mm/paging.c and mm/p2m-basic.c could also be compiled out
>> altogether when PAGING=n, at the expense of introducing a few more
>> stubs.
>>
>> FTAOD, the Fixes: tag being referenced does not mean this patch corrects
>> the far more recently introduced build issue with the combination of the
>> two features. That's still work that I expect Penny to carry out (with
>> there still being the option of reverting the final part of the earlier
>> series).
>>
>
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -864,6 +864,7 @@ void paging_final_teardown(struct domain
>> p2m_final_teardown(d);
>> }
>>
>> +#ifdef CONFIG_PAGING
>
> The file already has a lot of uses of #if PG_log_dirty with similar
> meaning, if I am not mistaken, so using that would make it more
> consistent. But CONFIG_PAGING is directly tied to the Kconfig, so maybe
> it is better? Just something I noticed.
... precisely matching your observation? If we want to accept the extra churn,
we certainly can go this route in a follow-on patch.
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Thanks.
Jan
© 2016 - 2025 Red Hat, Inc.