On 20.02.2026 16:29, 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. Misra C:2012 rule 20.6 disallows this altogether,
>> though. 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.
>>
>> No functional change intended. Of course codegen does change with this,
>> first and foremost in register allocation.
>>
>> Reported-by: Andrew Cooper <[email protected]>
>> Signed-off-by: Jan Beulich <[email protected]>
>
> I included this patch on an interim branch of other MISRA fixes of mine
> to get a run.
>
> https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/13198988953
>
> There's one more violation still to fix:
>
> if ( unlikely((level == 1)
> && sh_mfn_is_a_page_table(target_mfn)
> #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC )
> /* Unless the page is out of sync and the guest is
> writing to it. */
> && !(mfn_oos_may_write(target_mfn)
> && (ft == ft_demand_write))
> #endif /* OOS */
> ) )
> sflags &= ~_PAGE_RW;
>
>
>
> I also looked at this one previously. Making mfn_oos_may_write()
> visible outside of SHOPT_OUT_OF_SYNC is quite invasive.
We could simply add a stub returning constant true for the !OOS case.
> Here, I suggest dropping the unlikely() as the easiest fix. It's almost
> certainly useless anyway.
Especially when used around an && expression.
I may want to go a little further there, if already we need to touch this,
combining the two adjacent "level == 1" checks:
if ( level == 1 )
{
/* Protect guest page tables. */
if ( unlikely(sh_mfn_is_a_page_table(target_mfn))
#if SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC
/*
* Unless the page is out of sync and the guest is writing to it.
*/
&& (ft != ft_demand_write || !mfn_oos_may_write(target_mfn))
#endif /* OOS */
)
sflags &= ~_PAGE_RW;
/*
* paging_mode_log_dirty support
*
* Only allow the guest write access to a page a) on a demand fault,
* or b) if the page is already marked as dirty.
*
* (We handle log-dirty entirely inside the shadow code, without using
* the p2m_ram_logdirty p2m type: only HAP uses that.)
*/
if ( unlikely(paging_mode_log_dirty(d)) && !mmio_mfn )
{
if ( ft & FETCH_TYPE_WRITE )
paging_mark_dirty(d, target_mfn);
else if ( (sflags & _PAGE_RW) &&
!paging_mfn_is_dirty(d, target_mfn) )
sflags &= ~_PAGE_RW;
}
}
Thoughts?
The two different ways of checking for "guest is writing" also look somewhat
unhelpful. But there's yet another "ft & FETCH_TYPE_WRITE" elsewhere, so it
may want to be a separate patch to switch to uniformly comparing against
ft_demand_write.
Jan
