Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32
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
Erm, why?
The safety justification for the original patch was that this is HVM
only code. And it really is HVM only code - it's genuinely compiled out
for !HVM builds.
So if putting this entry back in fixes the regression OSSTest
identified, then either SH_type_l2h_64_shadow isn't PV32-only, or we
have PV guests entering HVM-only logic. Either way, the precondition
for correctness of the original patch is violated, and it needs
reverting on those grounds alone.
~Andrew
Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32
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
>
> Erm, why?
>
> The safety justification for the original patch was that this is HVM
> only code. 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. Hence
the PV32-special type needs to have a non-zero entry when, besides HVM=y,
PV32=y as well.
> So if putting this entry back in fixes the regression OSSTest
> identified, then either SH_type_l2h_64_shadow isn't PV32-only, or we
> have PV guests entering HVM-only logic. Either way, the precondition
> for correctness of the original patch is violated, and it needs
> reverting on those grounds alone.
I disagree - the table isn't needed when !HVM, and as such can be
considered HVM-only. It merely needs to deal with all cases correctly
when HVM=y.
Jan
Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32
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
>> Erm, why?
>>
>> The safety justification for the original patch was that this is HVM
>> only code. 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.
~Andrew
Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32
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
>>> Erm, why?
>>>
>>> The safety justification for the original patch was that this is HVM
>>> only code. 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) ...
Jan
Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32
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
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
Re: [PATCH] x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32
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
>
