On 22/03/2023 9:31 am, Jan Beulich wrote:
> SHADOW_FOREACH_L<N>E() already invokes the "body" only when the present
> bit is set; no need to re-do the check.
>
> While there also
> - stop open-coding mfn_to_maddr() in code being touched (re-indented)
> anyway,
> - stop open-coding mfn_eq() in code being touched or adjacent code,
> - drop local variables when they're no longer used at least twice.
>
> Signed-off-by: Jan Beulich <[email protected]>
While I agree this is the current behaviour, it's another spiketrap
waiting to happen.
There needs to be a rename of these macros to something like
FOREACH_PRESENT_SL?E(...). The SHADOW prefix is a bit verbose and can
be ineligibly encoded with the L?E acronym.
After which this patch be trivially correct, and the resulting code will
read safely in context.
>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3647,13 +3632,10 @@ int cf_check sh_remove_l1_shadow(struct
> {
> shadow_l2e_t *sl2e;
> int done = 0;
> - int flags;
>
> SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, done, d,
> {
> - flags = shadow_l2e_get_flags(*sl2e);
> - if ( (flags & _PAGE_PRESENT)
> - && (mfn_x(shadow_l2e_get_mfn(*sl2e)) == mfn_x(sl1mfn)) )
> + if ( mfn_x(shadow_l2e_get_mfn(*sl2e)) == mfn_x(sl1mfn) )
You've done mfn_eq() conversions thus far, but the final 3 hunks of the
patch could do with a conversion too.
With that, and subject to a suitable rename of the macros ahead of this
change, Reviewed-by: Andrew Cooper <[email protected]>
