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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2023 13:49, Jan Beulich wrote:
> On 23.01.2023 13:30, Andrew Cooper wrote:
>> On 23/01/2023 10:47 am, Jan Beulich wrote:
>>> On 23.01.2023 11:43, Andrew Cooper wrote:
>>>> On 23/01/2023 8:12 am, Jan Beulich wrote:
>>>>> While the table is used only when HVM=y, the table entry of course needs
>>>>> to be properly populated when also PV32=y. Fully removing the table
>>>>> entry we therefore wrong.
>>>>>
>>>>> Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only")
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Erm, why?
>>>>
>>>> The safety justification for the original patch was that this is HVM
>>>> only code.

Coming back to this: There was no such claim. There was a claim about
the type in question being PV32-only, and there was a comparison with
other types which are HVM-only.

>>>>  And it really is HVM only code - it's genuinely compiled out
>>>> for !HVM builds.
>>> Right, and we have logic taking care of the !HVM case. But that same
>>> logic uses this "HVM-only" table when HVM=y also for all PV types.
>>
>> Ok - this is what needs fixing then.
>>
>> 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);
#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