| Message ID | d6ef377e-731e-faa4-aa43-ef40ea34be07@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: assorted (mostly) shadow mode adjustments | expand |
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 <jbeulich@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
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 <jbeulich@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
--- 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); } @@ -1811,7 +1811,7 @@ static void sh_update_paging_modes(struc sh_detach_old_tables(v); #ifdef CONFIG_HVM - if ( is_hvm_domain(d) ) + if ( paging_mode_external(d) ) { const struct paging_mode *old_mode = v->arch.paging.mode; @@ -1864,13 +1864,12 @@ static void sh_update_paging_modes(struc make_cr3(v, mmfn); hvm_update_host_cr3(v); } - - if ( v->arch.paging.mode != old_mode ) + else if ( v->arch.paging.mode != old_mode ) { SHADOW_PRINTK("new paging mode: %pv pe=%d gl=%u " "sl=%u (was g=%u s=%u)\n", v, - is_hvm_domain(d) ? hvm_paging_enabled(v) : 1, + hvm_paging_enabled(v), v->arch.paging.mode->guest_levels, v->arch.paging.mode->shadow.shadow_levels, old_mode ? old_mode->guest_levels : 0,
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 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 <jbeulich@suse.com>