| Message ID | 942e1164-5ed0-bdda-424f-90134b0e22c5@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/shadow: sh_type_to_size[] needs L2H entry when HVM+PV32 | expand |
On 23/01/2023 8:12 am, Jan Beulich wrote: > While the table is used only when HVM=y, the table entry of course needs > to be properly populated when also PV32=y. Fully removing the table > entry we therefore wrong. > > Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Erm, why? The safety justification for the original patch was that this is HVM only code. And it really is HVM only code - it's genuinely compiled out for !HVM builds. So if putting this entry back in fixes the regression OSSTest identified, then either SH_type_l2h_64_shadow isn't PV32-only, or we have PV guests entering HVM-only logic. Either way, the precondition for correctness of the original patch is violated, and it needs reverting on those grounds alone. ~Andrew
On 23.01.2023 11:43, Andrew Cooper wrote: > On 23/01/2023 8:12 am, Jan Beulich wrote: >> While the table is used only when HVM=y, the table entry of course needs >> to be properly populated when also PV32=y. Fully removing the table >> entry we therefore wrong. >> >> Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Erm, why? > > The safety justification for the original patch was that this is HVM > only code. And it really is HVM only code - it's genuinely compiled out > for !HVM builds. Right, and we have logic taking care of the !HVM case. But that same logic uses this "HVM-only" table when HVM=y also for all PV types. Hence the PV32-special type needs to have a non-zero entry when, besides HVM=y, PV32=y as well. > So if putting this entry back in fixes the regression OSSTest > identified, then either SH_type_l2h_64_shadow isn't PV32-only, or we > have PV guests entering HVM-only logic. Either way, the precondition > for correctness of the original patch is violated, and it needs > reverting on those grounds alone. I disagree - the table isn't needed when !HVM, and as such can be considered HVM-only. It merely needs to deal with all cases correctly when HVM=y. Jan
On 23/01/2023 10:47 am, Jan Beulich wrote: > On 23.01.2023 11:43, Andrew Cooper wrote: >> On 23/01/2023 8:12 am, Jan Beulich wrote: >>> While the table is used only when HVM=y, the table entry of course needs >>> to be properly populated when also PV32=y. Fully removing the table >>> entry we therefore wrong. >>> >>> Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Erm, why? >> >> The safety justification for the original patch was that this is HVM >> only code. And it really is HVM only code - it's genuinely compiled out >> for !HVM builds. > Right, and we have logic taking care of the !HVM case. But that same > logic uses this "HVM-only" table when HVM=y also for all PV types. Ok - this is what needs fixing then. This is a layering violation which has successfully tricked you into making a buggy patch. I'm unwilling to bet this will be the final time either... "this file is HVM-only, therefore no PV paths enter it" is a reasonable expectation, and should be true. ~Andrew
On 23.01.2023 13:30, Andrew Cooper wrote: > On 23/01/2023 10:47 am, Jan Beulich wrote: >> On 23.01.2023 11:43, Andrew Cooper wrote: >>> On 23/01/2023 8:12 am, Jan Beulich wrote: >>>> While the table is used only when HVM=y, the table entry of course needs >>>> to be properly populated when also PV32=y. Fully removing the table >>>> entry we therefore wrong. >>>> >>>> Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only") >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> Erm, why? >>> >>> The safety justification for the original patch was that this is HVM >>> only code. And it really is HVM only code - it's genuinely compiled out >>> for !HVM builds. >> Right, and we have logic taking care of the !HVM case. But that same >> logic uses this "HVM-only" table when HVM=y also for all PV types. > > Ok - this is what needs fixing then. > > This is a layering violation which has successfully tricked you into > making a buggy patch. > > I'm unwilling to bet this will be the final time either... "this file > is HVM-only, therefore no PV paths enter it" is a reasonable > expectation, and should be true. Nice abstract consideration, but would mind pointing out how you envision shadow_size() to look like meeting your constraints _and_ meeting my demand of no excess #ifdef-ary? The way I'm reading your reply is that you ask to special case L2H _right in_ shadow_size(). Then again see also my remark in the original (now known faulty) patch regarding such special casing. I could of course follow that route, regardless of HVM (i.e. unlike said there not just for the #else part) ... Jan
On 23.01.2023 13:49, Jan Beulich wrote: > On 23.01.2023 13:30, Andrew Cooper wrote: >> On 23/01/2023 10:47 am, Jan Beulich wrote: >>> On 23.01.2023 11:43, Andrew Cooper wrote: >>>> On 23/01/2023 8:12 am, Jan Beulich wrote: >>>>> While the table is used only when HVM=y, the table entry of course needs >>>>> to be properly populated when also PV32=y. Fully removing the table >>>>> entry we therefore wrong. >>>>> >>>>> Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only") >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> Erm, why? >>>> >>>> The safety justification for the original patch was that this is HVM >>>> only code. Coming back to this: There was no such claim. There was a claim about the type in question being PV32-only, and there was a comparison with other types which are HVM-only. >>>> And it really is HVM only code - it's genuinely compiled out >>>> for !HVM builds. >>> Right, and we have logic taking care of the !HVM case. But that same >>> logic uses this "HVM-only" table when HVM=y also for all PV types. >> >> Ok - this is what needs fixing then. >> >> This is a layering violation which has successfully tricked you into >> making a buggy patch. >> >> I'm unwilling to bet this will be the final time either... "this file >> is HVM-only, therefore no PV paths enter it" is a reasonable >> expectation, and should be true. > > Nice abstract consideration, but would mind pointing out how you envision > shadow_size() to look like meeting your constraints _and_ meeting my > demand of no excess #ifdef-ary? The way I'm reading your reply is that > you ask to special case L2H _right in_ shadow_size(). Then again see also > my remark in the original (now known faulty) patch regarding such special > casing. I could of course follow that route, regardless of HVM (i.e. > unlike said there not just for the #else part) ... Actually no, that remark was about the opposite (!PV32) case, so if I took both together, this would result: static inline unsigned int shadow_size(unsigned int shadow_type) { #ifdef CONFIG_HVM #ifdef CONFIG_PV32 if ( shadow_type == SH_type_l2h_64_shadow ) return 1; #endif ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size)); return sh_type_to_size[shadow_type]; #else #ifndef CONFIG_PV32 if ( shadow_type == SH_type_l2h_64_shadow ) return 0; #endif ASSERT(shadow_type < SH_type_unused); return shadow_type != SH_type_none; #endif } I think that's quite a bit worse than using sh_type_to_size[] for all kinds of guest uniformly when HVM=y. This static inline unsigned int shadow_size(unsigned int shadow_type) { if ( shadow_type == SH_type_l2h_64_shadow ) return IS_ENABLED(CONFIG_PV32); #ifdef CONFIG_HVM ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size)); return sh_type_to_size[shadow_type]; #else ASSERT(shadow_type < SH_type_unused); return shadow_type != SH_type_none; #endif } is also only marginally better, as we really would better avoid any such open-coding. Jan
On 23.01.2023 17:56, Jan Beulich wrote: > On 23.01.2023 13:49, Jan Beulich wrote: >> On 23.01.2023 13:30, Andrew Cooper wrote: >>> This is a layering violation which has successfully tricked you into >>> making a buggy patch. >>> >>> I'm unwilling to bet this will be the final time either... "this file >>> is HVM-only, therefore no PV paths enter it" is a reasonable >>> expectation, and should be true. >> >> Nice abstract consideration, but would mind pointing out how you envision >> shadow_size() to look like meeting your constraints _and_ meeting my >> demand of no excess #ifdef-ary? The way I'm reading your reply is that >> you ask to special case L2H _right in_ shadow_size(). Then again see also >> my remark in the original (now known faulty) patch regarding such special >> casing. I could of course follow that route, regardless of HVM (i.e. >> unlike said there not just for the #else part) ... > > Actually no, that remark was about the opposite (!PV32) case, so if I > took both together, this would result: > > static inline unsigned int > shadow_size(unsigned int shadow_type) > { > #ifdef CONFIG_HVM > #ifdef CONFIG_PV32 > if ( shadow_type == SH_type_l2h_64_shadow ) > return 1; > #endif > ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size)); > return sh_type_to_size[shadow_type]; > #else > #ifndef CONFIG_PV32 > if ( shadow_type == SH_type_l2h_64_shadow ) > return 0; > #endif > ASSERT(shadow_type < SH_type_unused); > return shadow_type != SH_type_none; > #endif > } > > I think that's quite a bit worse than using sh_type_to_size[] for all > kinds of guest uniformly when HVM=y. This > > static inline unsigned int > shadow_size(unsigned int shadow_type) > { > if ( shadow_type == SH_type_l2h_64_shadow ) > return IS_ENABLED(CONFIG_PV32); Which might better use opt_pv32 instead, if we really were to go this route. Jan > #ifdef CONFIG_HVM > ASSERT(shadow_type < ARRAY_SIZE(sh_type_to_size)); > return sh_type_to_size[shadow_type]; > #else > ASSERT(shadow_type < SH_type_unused); > return shadow_type != SH_type_none; > #endif > } > > is also only marginally better, as we really would better avoid any > such open-coding. > > Jan >
--- a/xen/arch/x86/mm/shadow/hvm.c +++ b/xen/arch/x86/mm/shadow/hvm.c @@ -56,7 +56,9 @@ const uint8_t sh_type_to_size[] = { [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 */ +#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,
While the table is used only when HVM=y, the table entry of course needs to be properly populated when also PV32=y. Fully removing the table entry we therefore wrong. Fixes: 1894049fa283 ("x86/shadow: L2H shadow type is PV32-only") Signed-off-by: Jan Beulich <jbeulich@suse.com>