Re: [PATCH 2/2] x86/mm: correct PG_log_dirty definition

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>> ---
>> 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@xxxxxxx>

Thanks.

Jan