Re: [PATCH 07/11] x86/shadow: L2H shadow type is PV32-only

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 05/01/2023 4:05 pm, Jan Beulich wrote:
> Like for the various HVM-only types, save a little bit of code by suitably
> "masking" this type out when !PV32.

add/remove: 0/1 grow/shrink: 2/4 up/down: 544/-922 (-378)
Function                                     old     new   delta
sh_map_and_validate_gl2e__guest_4            136     666    +530
sh_destroy_shadow                            289     303     +14
sh_clear_shadow_entry__guest_4               178     176      -2
sh_validate_guest_entry                      521     472     -49
sh_map_and_validate_gl2he__guest_4           136       2    -134
sh_remove_shadows                           4757    4545    -212
validate_gl2e                                525       -    -525
Total: Before=3914702, After=3914324, chg -0.01%

Marginal...

>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I wasn't really sure whether it would be worthwhile to also update the
> "#else" part of shadow_size(). Doing so would be a little tricky, as the
> type to return 0 for has no name right now; I'd need to move down the
> #undef to allow for that. Thoughts?

This refers to the straight deletion from sh_type_to_size[] ?

I was confused by that at first.  The shadow does have a size of 1.  Might

/*   [SH_type_l2h_64_shadow]  = 1,  PV32 only */

work better?  That leaves it clearly in there as a 1, but not needing
any further ifdefary.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -859,13 +866,12 @@ do {
>      int _i;                                                                 \
>      int _xen = !shadow_mode_external(_dom);                                 \
>      shadow_l2e_t *_sp = map_domain_page((_sl2mfn));                         \
> -    ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow ||\
> -           mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2h_64_shadow);\
> +    ASSERT_VALID_L2(mfn_to_page(_sl2mfn)->u.sh.type);                       \
>      for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i++ )                  \
>      {                                                                       \
>          if ( (!(_xen))                                                      \
>               || !is_pv_32bit_domain(_dom)                                   \
> -             || mfn_to_page(_sl2mfn)->u.sh.type != SH_type_l2h_64_shadow    \
> +             || mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_64_shadow     \

Isn't this redundant with the ASSERT_VALID_L2() now?

Per your other question, yes this desperately wants rearranging, but I
would agree with it being another patch.

I did previously play at trying to simplify the PV pagetable loops in a
similar way.  Code-gen wise, I think the L2 loops what to calculate an
upper bound which is either 512, or compat_first_slot, while the L4
loops what an "if(i == 256) i += 7; continue;" rather than having
LFENCE-ing predicates on each iteration.

~Andrew