On 18/02/2026 12:42 pm, Jan Beulich wrote:
> 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.
Sorry - all I was trying to do was judge whether it was fair to call it
UB like this or not.
The MISRA complaint alone is fine justification for the patch.
Given this, I'd suggest dropping "defacto" as the easiest way of making
this a little more precise.
>
>>> 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.
Oh of course. I'd gone looking explicitly, and failed to spot it.
Reviewed-by: Andrew Cooper <[email protected]>
