On 23.03.2023 19:28, Andrew Cooper wrote: > On 22/03/2023 9:35 am, Jan Beulich wrote: >> While benign at present, it is still a little fragile to operate on a >> wrong "old_mode" value in sh_update_paging_modes(). This can happen when >> no monitor table was present initially - we'd create one for the new >> mode without updating old_mode. Correct this two ways, each of which > > I think you mean "Correct this in two ways" ? > >> would be sufficient on its own: Once by adding "else" to the second of >> the involved if()s in the function, and then by setting the correct >> initial mode for HVM domains in shadow_vcpu_init(). >> >> Further use the same predicate (paging_mode_external()) consistently >> when dealing with shadow mode init/update/cleanup, rather than a mix of >> is_hvm_vcpu() (init), is_hvm_domain() (update), and >> paging_mode_external() (cleanup). >> >> Finally drop a redundant is_hvm_domain() from inside the bigger if() >> (which is being converted to paging_mode_external()) in >> sh_update_paging_modes(). >> >> Signed-off-by: Jan Beulich <jbeul...@suse.com> >> >> --- a/xen/arch/x86/mm/shadow/common.c >> +++ b/xen/arch/x86/mm/shadow/common.c >> @@ -129,8 +129,8 @@ void shadow_vcpu_init(struct vcpu *v) >> } >> #endif >> >> - v->arch.paging.mode = is_hvm_vcpu(v) ? >> - &SHADOW_INTERNAL_NAME(sh_paging_mode, 3) : >> + v->arch.paging.mode = paging_mode_external(v->domain) ? >> + &SHADOW_INTERNAL_NAME(sh_paging_mode, 2) : >> &SHADOW_INTERNAL_NAME(sh_paging_mode, 4); > > As you're changing this, reposition the ? and : to the start of the > following lines?
Sure. > But, is 2-level mode actually right? It's better than 3 certainly, and > is what sh_update_paging_modes() selects, but isn't that only right for > the IDENT_PT case? Which is how HVM vCPU-s start, isn't it? For PVH there clearly needs to be a separate (later) paging mode update anyway (or else what - as you say - sh_update_paging_modes() selects would also be wrong), which is going to be whenever we see CR0.PG becoming set by the toolstack. Jan
