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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.  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