On 18.02.2026 12:30, Andrew Cooper wrote:
> On 18/02/2026 9:03 am, Jan Beulich wrote:
>> As per the standard this is UB, i.e. we're building on a defacto extension
>> in the compilers we use.
>
> Is it a real extension, or just something that happens to work?
I was hoping I would not need to go through that large swath of gcc doc to
actually figure, because ...
>> Misra C:2012 rule 20.6 disallows this altogether,
>> though.
... this I assumed was reason enough. Still, now that you forced me to: In
The C Preprocessor the behavior is described as intentional, but not as an
extension (section "Directives Within Macro Arguments"). Now you get to
judge whether that's a "real" extension or a "de-facto" one.
>> Use helper always-inline functions instead.
>>
>> In sh_audit_l1_table(), along with reducing the scope of "gfn", which now
>> isn't used anymore by the if() side of the conditional, also reduce the
>> scope of two other adjacent variables.
>>
>> For audit_magic() note that both which parameters are needed and what
>> their types are is attributed to AUDIT_FAIL() accessing variables which
>> aren't passed as arguments to it.
>
> This is grammatically awkward. IMO it would be clearer to say "For
> audit_magic() note that there are more parameters than might seem
> necessary, caused by the expectations of AUDIT_FAIL()."
I've switched to using that, but one aspect is lost this way: I would have
preferred both gl1e and sl1e to be plain entries, not pointers to ones.
>> ---
>> Leaving even the fetching of current to the helper in
>> sh_rm_write_access_from_l1() looks tidier to me overall, albeit this means
>> the fetch will now occur once per present L1E.
>
> This will not make a dent in the performance of the shadow code.
>
>> Converting the #if to if() and #ifdef to if(IS_ENABLED()) wouldn't work
>> here, as identifiers are used which aren't available when the respective
>> conditions are false.
>
> Personally, I'd have put this in the main commit message, because it's
> the justification for why out-of-line static inline's need to be used.
I was wondering, so I've moved this up.
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -395,7 +395,7 @@ static inline mfn_t cf_check sh_next_pag
>> shadow_set_l2e(d, sl2e, new_sl2e, sl2mfn, SH_type_fl1_shadow,
>> sh_next_page)
>>
>> static inline u32
>> -guest_index(void *ptr)
>> +guest_index(const void *ptr)
>> {
>> return (u32)((unsigned long)ptr & ~PAGE_MASK) / sizeof(guest_l1e_t);
>> }
>
> While fine per say, this doesn't appear to be related to the patch?
It does, the compiler told me to: type_from_gl3e() uses it, and I really
want to keep the const-s on both of its parameters.
Jan
