[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/shadow: Fix PV32 shadowing in !HVM builds
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |