On Wed, Jul 22, 2020 at 11:18:09AM +0100, Andrew Cooper wrote: > ... rather than from the default clauses of the PV and HVM MSR handlers. > > This means that we no longer take the vmce lock for any unknown MSR, and > accesses to architectural MCE banks outside of the subset implemented for the > guest no longer fall further through the unknown MSR path. > > With the vmce calls removed, the hvm alternative_call()'s expression can be > simplified substantially. > > Signed-off-by: Andrew Cooper <[email protected]>
LGTM, I just have one question below regarding the ranges. Reviewed-by: Roger Pau Monné <[email protected]> > --- > CC: Jan Beulich <[email protected]> > CC: Wei Liu <[email protected]> > CC: Roger Pau Monné <[email protected]> > --- > xen/arch/x86/hvm/hvm.c | 16 ++-------------- > xen/arch/x86/msr.c | 16 ++++++++++++++++ > xen/arch/x86/pv/emul-priv-op.c | 15 --------------- > 3 files changed, 18 insertions(+), 29 deletions(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 5bb47583b3..a9d1685549 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3560,13 +3560,7 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t > *msr_content) > break; > > default: > - if ( (ret = vmce_rdmsr(msr, msr_content)) < 0 ) > - goto gp_fault; > - /* If ret == 0 then this is not an MCE MSR, see other MSRs. */ > - ret = ((ret == 0) > - ? alternative_call(hvm_funcs.msr_read_intercept, > - msr, msr_content) > - : X86EMUL_OKAY); > + ret = alternative_call(hvm_funcs.msr_read_intercept, msr, > msr_content); > break; > } > > @@ -3696,13 +3690,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t > msr_content, > break; > > default: > - if ( (ret = vmce_wrmsr(msr, msr_content)) < 0 ) > - goto gp_fault; > - /* If ret == 0 then this is not an MCE MSR, see other MSRs. */ > - ret = ((ret == 0) > - ? alternative_call(hvm_funcs.msr_write_intercept, > - msr, msr_content) > - : X86EMUL_OKAY); > + ret = alternative_call(hvm_funcs.msr_write_intercept, msr, > msr_content); > break; > } > > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c > index 22f921cc71..ca4307e19f 100644 > --- a/xen/arch/x86/msr.c > +++ b/xen/arch/x86/msr.c > @@ -227,6 +227,14 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t > *val) > *val = msrs->misc_features_enables.raw; > break; > > + case MSR_IA32_MCG_CAP ... MSR_IA32_MCG_CTL: /* 0x179 -> 0x17b */ > + case MSR_IA32_MCx_CTL2(0) ... MSR_IA32_MCx_CTL2(31): /* 0x280 -> 0x29f */ > + case MSR_IA32_MCx_CTL(0) ... MSR_IA32_MCx_MISC(31): /* 0x400 -> 0x47f */ Where do you get the ranges from 0 to 31? It seems like the count field in the CAP register is 8 bits, which could allow for up to 256 banks? I'm quite sure this would then overlap with other MSRs? > + case MSR_IA32_MCG_EXT_CTL: /* 0x4d0 */ > + if ( vmce_rdmsr(msr, val) < 0 ) > + goto gp_fault; > + break; > + > case MSR_X2APIC_FIRST ... MSR_X2APIC_LAST: > if ( !is_hvm_domain(d) || v != curr ) > goto gp_fault; > @@ -436,6 +444,14 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t > val) > break; > } > > + case MSR_IA32_MCG_CAP ... MSR_IA32_MCG_CTL: /* 0x179 -> 0x17b */ > + case MSR_IA32_MCx_CTL2(0) ... MSR_IA32_MCx_CTL2(31): /* 0x280 -> 0x29f */ > + case MSR_IA32_MCx_CTL(0) ... MSR_IA32_MCx_MISC(31): /* 0x400 -> 0x47f */ > + case MSR_IA32_MCG_EXT_CTL: /* 0x4d0 */ > + if ( vmce_wrmsr(msr, val) < 0 ) > + goto gp_fault; > + break; > + > case MSR_X2APIC_FIRST ... MSR_X2APIC_LAST: > if ( !is_hvm_domain(d) || v != curr ) > goto gp_fault; > diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c > index 254da2b849..f14552cb4b 100644 > --- a/xen/arch/x86/pv/emul-priv-op.c > +++ b/xen/arch/x86/pv/emul-priv-op.c > @@ -855,8 +855,6 @@ static int read_msr(unsigned int reg, uint64_t *val, > > switch ( reg ) > { > - int rc; > - > case MSR_FS_BASE: > if ( is_pv_32bit_domain(currd) ) > break; > @@ -955,12 +953,6 @@ static int read_msr(unsigned int reg, uint64_t *val, > } > /* fall through */ > default: > - rc = vmce_rdmsr(reg, val); > - if ( rc < 0 ) > - break; > - if ( rc ) > - return X86EMUL_OKAY; > - /* fall through */ > normal: We could remove the 'normal' label and just use the default one instead. Thanks, Roger.
