| Message ID | 20200909095920.25495-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pv: Minor perf improvements in segment handling | expand |
On 09.09.2020 11:59, Andrew Cooper wrote: > is_pv_32bit_domain() is an expensive predicate, but isn't used for speculative > safety in this case. Swap to checking the Long Mode bit in the CPUID policy, > which is the architecturally correct behaviour. > > is_canonical_address() isn't a trivial predicate, but it will become more > complicated when 5-level support is added. Rearrange write_msr() to collapse > the common checks. Did you mean "is" instead of "isn't" or "and" instead of "but"? The way it is it doesn't look very logical to me. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one more remark: > @@ -991,22 +993,22 @@ static int write_msr(unsigned int reg, uint64_t val, > uint64_t temp; > > case MSR_FS_BASE: > - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) > - break; > - write_fs_base(val); > - return X86EMUL_OKAY; > - > case MSR_GS_BASE: > - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) > - break; > - write_gs_base(val); > - return X86EMUL_OKAY; > - > case MSR_SHADOW_GS_BASE: > - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) > + if ( !cp->extd.lm || !is_canonical_address(val) ) > break; > - write_gs_shadow(val); > - curr->arch.pv.gs_base_user = val; > + > + if ( reg == MSR_FS_BASE ) > + write_fs_base(val); > + else if ( reg == MSR_GS_BASE ) > + write_gs_base(val); > + else if ( reg == MSR_SHADOW_GS_BASE ) With the three case labels just above, I think this "else if" and ... > + { > + write_gs_shadow(val); > + curr->arch.pv.gs_base_user = val; > + } > + else > + ASSERT_UNREACHABLE(); ... this assertion are at least close to being superfluous. Their dropping would then also make me less inclined to ask for an inner switch(). Jan
On 11/09/2020 11:01, Jan Beulich wrote: > On 09.09.2020 11:59, Andrew Cooper wrote: >> is_pv_32bit_domain() is an expensive predicate, but isn't used for speculative >> safety in this case. Swap to checking the Long Mode bit in the CPUID policy, >> which is the architecturally correct behaviour. >> >> is_canonical_address() isn't a trivial predicate, but it will become more >> complicated when 5-level support is added. Rearrange write_msr() to collapse >> the common checks. > Did you mean "is" instead of "isn't" or "and" instead of "but"? The > way it is it doesn't look very logical to me. I guess the meaning got lost somewhere. is_canonical_address() is currently not completely trivial, but also not massively complicated either. It will become much more complicated with LA57. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one more remark: > >> @@ -991,22 +993,22 @@ static int write_msr(unsigned int reg, uint64_t val, >> uint64_t temp; >> >> case MSR_FS_BASE: >> - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) >> - break; >> - write_fs_base(val); >> - return X86EMUL_OKAY; >> - >> case MSR_GS_BASE: >> - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) >> - break; >> - write_gs_base(val); >> - return X86EMUL_OKAY; >> - >> case MSR_SHADOW_GS_BASE: >> - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) >> + if ( !cp->extd.lm || !is_canonical_address(val) ) >> break; >> - write_gs_shadow(val); >> - curr->arch.pv.gs_base_user = val; >> + >> + if ( reg == MSR_FS_BASE ) >> + write_fs_base(val); >> + else if ( reg == MSR_GS_BASE ) >> + write_gs_base(val); >> + else if ( reg == MSR_SHADOW_GS_BASE ) > With the three case labels just above, I think this "else if" and ... > >> + { >> + write_gs_shadow(val); >> + curr->arch.pv.gs_base_user = val; >> + } >> + else >> + ASSERT_UNREACHABLE(); > ... this assertion are at least close to being superfluous. Their > dropping would then also make me less inclined to ask for an > inner switch(). I'm not overly fussed, as this example is fairly trivial, but I was attempting to go for something which ends up safe even in the case of a bad edit to the outer switch statement. I'd expect the compiler to be drop the both aspects you talk about. ~Andrew
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index 9dd1d59423..0fd95fe9fa 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -848,6 +848,7 @@ static int read_msr(unsigned int reg, uint64_t *val, { struct vcpu *curr = current; const struct domain *currd = curr->domain; + const struct cpuid_policy *cp = currd->arch.cpuid; bool vpmu_msr = false; int ret; @@ -869,19 +870,19 @@ static int read_msr(unsigned int reg, uint64_t *val, return X86EMUL_OKAY; case MSR_FS_BASE: - if ( is_pv_32bit_domain(currd) ) + if ( !cp->extd.lm ) break; *val = read_fs_base(); return X86EMUL_OKAY; case MSR_GS_BASE: - if ( is_pv_32bit_domain(currd) ) + if ( !cp->extd.lm ) break; *val = read_gs_base(); return X86EMUL_OKAY; case MSR_SHADOW_GS_BASE: - if ( is_pv_32bit_domain(currd) ) + if ( !cp->extd.lm ) break; *val = curr->arch.pv.gs_base_user; return X86EMUL_OKAY; @@ -975,6 +976,7 @@ static int write_msr(unsigned int reg, uint64_t val, { struct vcpu *curr = current; const struct domain *currd = curr->domain; + const struct cpuid_policy *cp = currd->arch.cpuid; bool vpmu_msr = false; int ret; @@ -991,22 +993,22 @@ static int write_msr(unsigned int reg, uint64_t val, uint64_t temp; case MSR_FS_BASE: - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) - break; - write_fs_base(val); - return X86EMUL_OKAY; - case MSR_GS_BASE: - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) - break; - write_gs_base(val); - return X86EMUL_OKAY; - case MSR_SHADOW_GS_BASE: - if ( is_pv_32bit_domain(currd) || !is_canonical_address(val) ) + if ( !cp->extd.lm || !is_canonical_address(val) ) break; - write_gs_shadow(val); - curr->arch.pv.gs_base_user = val; + + if ( reg == MSR_FS_BASE ) + write_fs_base(val); + else if ( reg == MSR_GS_BASE ) + write_gs_base(val); + else if ( reg == MSR_SHADOW_GS_BASE ) + { + write_gs_shadow(val); + curr->arch.pv.gs_base_user = val; + } + else + ASSERT_UNREACHABLE(); return X86EMUL_OKAY; case MSR_EFER:
is_pv_32bit_domain() is an expensive predicate, but isn't used for speculative safety in this case. Swap to checking the Long Mode bit in the CPUID policy, which is the architecturally correct behaviour. is_canonical_address() isn't a trivial predicate, but it will become more complicated when 5-level support is added. Rearrange write_msr() to collapse the common checks. 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> For reference, the diff is: add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-152 (-152) Function old new delta read_msr 1075 1030 -45 write_msr 1537 1430 -107 but this isn't the point of the change. --- xen/arch/x86/pv/emul-priv-op.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)