Re: [PATCH 15/17] x86/shadow: drop SH_type_l2h_pae_shadow

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2021 21:02, Tim Deegan wrote:
> At 17:31 +0100 on 22 Jan (1611336662), Jan Beulich wrote:
>> On 22.01.2021 14:11, Tim Deegan wrote:
>>> At 16:10 +0100 on 14 Jan (1610640627), Jan Beulich wrote:
>>>> hash_{domain,vcpu}_foreach() have a use each of literal 15. It's not
>>>> clear to me what the proper replacement constant would be, as it
>>>> doesn't look as if SH_type_monitor_table was meant.
>>>
>>> Good spot.  I think the '<= 15' should be replaced with '< SH_type_unused'.
>>> It originally matched the callback arrays all being coded as
>>> "static hash_callback_t callbacks[16]".
>>
>> So are you saying this was off by one then prior to this patch
>> (when SH_type_unused was still 17), albeit in apparently a
>> benign way?
> 
> Yes - this assertion is just to catch overruns of the callback table,
> and so it was OK for its limit to be too low.  The new types that were
> added since then are never put in the hash table, so don't trigger
> this assertion.
> 
>> And the comments in sh_remove_write_access(),
>> sh_remove_all_mappings(), sh_remove_shadows(), and
>> sh_reset_l3_up_pointers() are then wrong as well, and would
>> instead better be like in shadow_audit_tables()?
> 
> Yes, it looks like those comments are also out of date where they
> mention 'unused'.

For this, which likely will end up being part of ...

>> Because of this having been benign (due to none of the callback
>> tables specifying non-NULL entries there), wouldn't it make
>> sense to dimension the tables by SH_type_max_shadow + 1 only?
>> Or would you consider this too risky?
> 
> Yes, I think that would be fine, also changing '<= 15' to
> '<= SH_type_max_shadow'.  Maybe add a matching
> ASSERT(t <= SH_type_max_shadow) in shadow_hash_insert as well?

... this, I'll send a patch for 4.16 going beyond the more
immediate one sent, which I'll ask Ian to consider for 4.15
(assuming of course you consider it okay in the first place).

Jan