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