Re: [PATCH] x86/shadow: Fix PV32 shadowing in !HVM builds

[Andrew Cooper <amc96@xxxxxxxx>]

On 26/01/2023 8:50 am, Jan Beulich wrote:
> On 25.01.2023 17:53, Andrew Cooper wrote:
>> The OSSTest bisector identified an issue with c/s 1894049fa283 ("x86/shadow:
>> L2H shadow type is PV32-only") in !HVM builds.
>>
>> The bug is ultimately caused by sh_type_to_size[] not actually being specific
>> to HVM guests, and it's position in shadow/hvm.c mislead the reasoning.
>>
>> To fix the issue that OSSTest identified, SH_type_l2h_64_shadow must still
>> have the value 1 in any CONFIG_PV32 build.  But simply adjusting this leaves
>> us with misleading logic, and a reasonable chance of making a related error
>> again in the future.
>>
>> In hindsight, moving sh_type_to_size[] out of common.c in the first place a
>> mistake.  Therefore, move sh_type_to_size[] back to living in common.c,
>> leaving a comment explaining why it happens to be inside an HVM conditional.
>>
>> This effectively reverts the second half of 4fec945409fc ("x86/shadow: adjust
>> and move sh_type_to_size[]") while retaining the other improvements from the
>> same changeset.
>>
>> While making this change, also adjust the sh_type_to_size[] declaration to
>> match its definition.
>>
>> Fixes: 4fec945409fc ("x86/shadow: adjust and move sh_type_to_size[]")
>> Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Now it's time for me to ask: But why? Your interpretation of "HVM-only"
> is simply too restricted.

I appreciate that we have different opinions on the matter.

But the sequence of events speaks for itself.  Inadvertently, you
created this trap, and then fell straight into it.

> As said, "HVM-only" can have two meanings -
> build-time HVM-only and run time HVM-only.

So

obj-$(CONFIG_HVM) += hvm.c

mean "this file, if and only if it is compiled in, contains logic
critical to the runtime functioning of PV guests" does it.

>  Code in hvm.c is also
> expecting to be entered for PV guests when HVM=y - see the several
> is_hvm_...(). They're all bogus though, and I have a patch pending to
> remove them. But that doesn't alter the principle. See e.g. audit_p2m(),
> which simply bails first thing when !paging_mode_translate(), or
> p2m_pod_active(), which bails first thing when !is_hvm_domain().

The fact that similar layering violations exist elsewhere doesn't mean
that this isn't one.  The fact that all experts in this area of code got
it wrong *is* the problem.

You in writing the patch and me in reviewing it made the assumption that
PV guests don't enter code in hvm.c *because* it is an entirely
reasonable assumption to make.

> Content of hvm.c (and other files which are built only when HVM=y, or
> more generally any other files which have a Kconfig build time
> dependency in their respective Makefile) simply has to be aware of this
> fact, and hence the data (array) in question is quite fine to live where
> it does.

There are two ways to stop this from happening.

Either make the assumption actually true, or stop people making the
assumption.

One is these can be done, and one cannot.


> I continue to view my 1st patch as the better approach. And in no case
> do I view the 1st Fixes: tag as appropriate.
>
> I guess we really need George or Tim to break ties here.

I have committed this with George's Ack.

~Andrew