| Message ID | 20230125165308.22897-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/shadow: Fix PV32 shadowing in !HVM builds | expand |
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@citrix.com> 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
On Wed, Jan 25, 2023 at 4:53 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> > I don't have super-strong opinions here; I'd be OK with any of the patches. But I tend to sympathize with Andrew's arguments re communicating what's going on. Acked-by: George Dunlap <george.dunlap@cloud.com>
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@citrix.com> > 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
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index 26901b8b3bcf..a74b15e3e75b 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -39,6 +39,44 @@ #include <public/sched.h> #include "private.h" +/* + * This table shows the allocation behaviour of the different modes: + * + * Xen paging 64b 64b 64b + * Guest paging 32b pae 64b + * PV or HVM HVM HVM * + * Shadow paging pae pae 64b + * + * sl1 size 8k 4k 4k + * sl2 size 16k 4k 4k + * sl3 size - - 4k + * sl4 size - - 4k + * + * Note: our accessor, shadow_size(), can optimise out this table in PV-only + * builds. + */ +#ifdef CONFIG_HVM +const uint8_t sh_type_to_size[] = { + [SH_type_l1_32_shadow] = 2, + [SH_type_fl1_32_shadow] = 2, + [SH_type_l2_32_shadow] = 4, + [SH_type_l1_pae_shadow] = 1, + [SH_type_fl1_pae_shadow] = 1, + [SH_type_l2_pae_shadow] = 1, + [SH_type_l1_64_shadow] = 1, + [SH_type_fl1_64_shadow] = 1, + [SH_type_l2_64_shadow] = 1, +#ifdef CONFIG_PV32 + [SH_type_l2h_64_shadow] = 1, +#endif + [SH_type_l3_64_shadow] = 1, + [SH_type_l4_64_shadow] = 1, + [SH_type_p2m_table] = 1, + [SH_type_monitor_table] = 1, + [SH_type_oos_snapshot] = 1, +}; +#endif /* CONFIG_HVM */ + DEFINE_PER_CPU(uint32_t,trace_shadow_path_flags); static int cf_check sh_enable_log_dirty(struct domain *, bool log_global); diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c index 918865cf1b6a..88c3c16322f2 100644 --- a/xen/arch/x86/mm/shadow/hvm.c +++ b/xen/arch/x86/mm/shadow/hvm.c @@ -33,37 +33,6 @@ #include "private.h" -/* - * This table shows the allocation behaviour of the different modes: - * - * Xen paging 64b 64b 64b - * Guest paging 32b pae 64b - * PV or HVM HVM HVM * - * Shadow paging pae pae 64b - * - * sl1 size 8k 4k 4k - * sl2 size 16k 4k 4k - * sl3 size - - 4k - * sl4 size - - 4k - */ -const uint8_t sh_type_to_size[] = { - [SH_type_l1_32_shadow] = 2, - [SH_type_fl1_32_shadow] = 2, - [SH_type_l2_32_shadow] = 4, - [SH_type_l1_pae_shadow] = 1, - [SH_type_fl1_pae_shadow] = 1, - [SH_type_l2_pae_shadow] = 1, - [SH_type_l1_64_shadow] = 1, - [SH_type_fl1_64_shadow] = 1, - [SH_type_l2_64_shadow] = 1, -/* [SH_type_l2h_64_shadow] = 1, PV32-only */ - [SH_type_l3_64_shadow] = 1, - [SH_type_l4_64_shadow] = 1, - [SH_type_p2m_table] = 1, - [SH_type_monitor_table] = 1, - [SH_type_oos_snapshot] = 1, -}; - /**************************************************************************/ /* x86 emulator support for the shadow code */ diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h index 7d6c846c8037..79d82364fc92 100644 --- a/xen/arch/x86/mm/shadow/private.h +++ b/xen/arch/x86/mm/shadow/private.h @@ -362,7 +362,7 @@ static inline int mfn_oos_may_write(mfn_t gmfn) #endif /* (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC) */ /* Figure out the size (in pages) of a given shadow type */ -extern const u8 sh_type_to_size[SH_type_unused]; +extern const uint8_t sh_type_to_size[SH_type_unused]; static inline unsigned int shadow_size(unsigned int shadow_type) {
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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Tim Deegan <tim@xen.org> I was unsure whether it was reasonable to move the table back into its old position but it can live pretty much anywhere in common.c as far as I'm concerned. --- xen/arch/x86/mm/shadow/common.c | 38 ++++++++++++++++++++++++++++++++++++++ xen/arch/x86/mm/shadow/hvm.c | 31 ------------------------------- xen/arch/x86/mm/shadow/private.h | 2 +- 3 files changed, 39 insertions(+), 32 deletions(-)