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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. As said, "HVM-only" can have two meanings -
build-time HVM-only and run time HVM-only. 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().

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.

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.

Jan