Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2023 17:56, Jan Beulich wrote:
> On 23.01.2023 13:49, Jan Beulich wrote:
>> On 23.01.2023 13:30, Andrew Cooper wrote:
>>> This is a layering violation which has successfully tricked you into
>>> making a buggy patch.
>>>
>>> I'm unwilling to bet this will be the final time either...  "this file
>>> is HVM-only, therefore no PV paths enter it" is a reasonable
>>> expectation, and should be true.
>>
>> Nice abstract consideration, but would mind pointing out how you envision
>> shadow_size() to look like meeting your constraints _and_ meeting my
>> demand of no excess #ifdef-ary? The way I'm reading your reply is that
>> you ask to special case L2H _right in_ shadow_size(). Then again see also
>> my remark in the original (now known faulty) patch regarding such special
>> casing. I could of course follow that route, regardless of HVM (i.e.
>> unlike said there not just for the #else part) ...
> 
> Actually no, that remark was about the opposite (!PV32) case, so if I
> took both together, this would result:
> 
> static inline unsigned int
> shadow_size(unsigned int shadow_type)
> {
> #ifdef CONFIG_HVM
> #ifdef CONFIG_PV32
>     if ( shadow_type == SH_type_l2h_64_shadow )
>         return 1;
> #endif
>     ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size));
>     return sh_type_to_size[shadow_type];
> #else
> #ifndef CONFIG_PV32
>     if ( shadow_type == SH_type_l2h_64_shadow )
>         return 0;
> #endif
>     ASSERT(shadow_type < SH_type_unused);
>     return shadow_type != SH_type_none;
> #endif
> }
> 
> I think that's quite a bit worse than using sh_type_to_size[] for all
> kinds of guest uniformly when HVM=y. This
> 
> static inline unsigned int
> shadow_size(unsigned int shadow_type)
> {
>     if ( shadow_type == SH_type_l2h_64_shadow )
>         return IS_ENABLED(CONFIG_PV32);

Which might better use opt_pv32 instead, if we really were to go this route.

Jan

> #ifdef CONFIG_HVM
>     ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size));
>     return sh_type_to_size[shadow_type];
> #else
>     ASSERT(shadow_type < SH_type_unused);
>     return shadow_type != SH_type_none;
> #endif
> }
> 
> is also only marginally better, as we really would better avoid any
> such open-coding.
> 
> Jan
>