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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.01.2023 02:31, Andrew Cooper wrote:
> 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...

Talk wasn't of only size, but also of what actually is being executed
for no gain e.g. in sh_remove_shadows(). I think "a little bit" is a
fair statement.

>> 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[] ?

Not really, no, but ...

> 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.

... I've made this adjustment anyway. I'd like to note though that such
commenting is being disliked by Misra.

The remark is rather about shadow_size() itself which already doesn't
return the "correct" size for all of the types when !HVM, utilizing that
the returned size is of no real interest for types which aren't used in
that case (and which then also don't really exist anymore as
"distinguishable" types). Plus the connection to assertions like this

    ASSERT(pages);

in shadow_alloc(): "pages" is the return value from shadow_size(), and
I think it wouldn't be bad to trigger for "dead" types, requiring the
function to return zero for this type despite it not becoming aliased to
SH_type_unused (i.e. unlike the types unavailable when !HVM).

Which makes me notice that with HVM=y shadow_size() would probably
better use array_access_nospec(). I'll add another patch for this.

>> --- 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?

How would that be? ASSERT_VALID_L2(), when PV32=y allows for both l2 and
l2h, whereas here we strictly mean only l2. The sole reason for the change
is that the l2h constant simply doesn't exist anymore when !PV32.

> 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.

I'll see what I can do without getting distracted too much. (I also guess
you mean "i += 15".)

Jan