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? 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? ~Andrew
