Re: [PATCH 6/8] x86/shadow: adjust and move sh_type_to_size[]

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2022 19:58, Andrew Cooper wrote:
> On 21/12/2022 1:27 pm, Jan Beulich wrote:
>> Drop the SH_type_none entry - there are no allocation attempts with
>> this type, and there also shouldn't be any. Adjust the shadow_size()
>> alternative path to match that change. Also generalize two related
>> assertions.
>>
>> While there move the entire table and the respective part of the comment
>> there to hvm.c, resulting in one less #ifdef. In the course of the
>> movement switch to using designated initializers.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, although ...

Thanks.

>> --- a/xen/arch/x86/mm/shadow/hvm.c
>> +++ b/xen/arch/x86/mm/shadow/hvm.c
>> @@ -33,6 +33,37 @@
>>  
>>  #include "private.h"
>>  
>> +/*
>> + * This table shows the allocation behaviour of the different modes:
>> + *
>> + * Xen paging      64b  64b  64b
>> + * Guest paging    32b  pae  64b
>> + * PV or HVM       HVM  HVM   *
>> + * Shadow paging   pae  pae  64b
>> + *
>> + * sl1 size         8k   4k   4k
>> + * sl2 size        16k   4k   4k
>> + * sl3 size         -    -    4k
>> + * sl4 size         -    -    4k
>> + */
>> +const uint8_t sh_type_to_size[] = {
>> +    [SH_type_l1_32_shadow]   = 2,
>> +    [SH_type_fl1_32_shadow]  = 2,
>> +    [SH_type_l2_32_shadow]   = 4,
>> +    [SH_type_l1_pae_shadow]  = 1,
>> +    [SH_type_fl1_pae_shadow] = 1,
>> +    [SH_type_l2_pae_shadow]  = 1,
>> +    [SH_type_l1_64_shadow]   = 1,
>> +    [SH_type_fl1_64_shadow]  = 1,
>> +    [SH_type_l2_64_shadow]   = 1,
>> +    [SH_type_l2h_64_shadow]  = 1,
>> +    [SH_type_l3_64_shadow]   = 1,
>> +    [SH_type_l4_64_shadow]   = 1,
>> +    [SH_type_p2m_table]      = 1,
>> +    [SH_type_monitor_table]  = 1,
>> +    [SH_type_oos_snapshot]   = 1,
> 
> ... this feels like it's wanting to turn into a (1 + ...) expression.  I
> can't see anything that prevents us from reordering the SH_type
> constants, but
> 
> 1 + (idx == 1 /* l1 */ || idx == /* fl1 */) + 2 * (idx == 3 /* l2 */);
> 
> doesn't obviously simplify further.

But that would then undermine the cases where the function returns 0,
which the two (generalized in this change) assertions actually check
for not having got back. This would also become relevant for the l2h
slot, which - if not right here (see the respective remark) - will
become zero in a subsequent patch when !PV32.

Jan