| Message ID | 20211217153003.1719189-24-jing2.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMX Support in KVM | expand |
On 12/17/21 16:30, Jing Liu wrote: > Also disable read emulation of IA32_XFD_ERR MSR at the same point > where r/w emulation of IA32_XFD MSR is disabled. This saves one > unnecessary VM-exit in guest #NM handler, given that the MSR is > already restored with the guest value before the guest is resumed. > > Signed-off-by: Jing Liu <jing2.liu@intel.com> Why not do this unconditionally (i.e. in patch 13, with the call to vmx_disable_intercept_for_msr in function vmx_create_vcpu)? Thanks, Paolo > --- > arch/x86/kvm/vmx/vmx.c | 2 ++ > arch/x86/kvm/vmx/vmx.h | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 97a823a3f23f..b66a005f076b 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -163,6 +163,7 @@ static u32 vmx_possible_passthrough_msrs[MAX_POSSIBLE_PASSTHROUGH_MSRS] = { > MSR_GS_BASE, > MSR_KERNEL_GS_BASE, > MSR_IA32_XFD, > + MSR_IA32_XFD_ERR, > #endif > MSR_IA32_SYSENTER_CS, > MSR_IA32_SYSENTER_ESP, > @@ -1934,6 +1935,7 @@ static u64 vcpu_supported_debugctl(struct kvm_vcpu *vcpu) > static void vmx_set_xfd_passthrough(struct kvm_vcpu *vcpu) > { > vmx_disable_intercept_for_msr(vcpu, MSR_IA32_XFD, MSR_TYPE_RW); > + vmx_disable_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, MSR_TYPE_R); > vcpu->arch.xfd_out_of_sync = true; > } > #endif > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index bf9d3051cd6c..0a00242a91e7 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -340,7 +340,7 @@ struct vcpu_vmx { > struct lbr_desc lbr_desc; > > /* Save desired MSR intercept (read: pass-through) state */ > -#define MAX_POSSIBLE_PASSTHROUGH_MSRS 14 > +#define MAX_POSSIBLE_PASSTHROUGH_MSRS 15 > struct { > DECLARE_BITMAP(read, MAX_POSSIBLE_PASSTHROUGH_MSRS); > DECLARE_BITMAP(write, MAX_POSSIBLE_PASSTHROUGH_MSRS); >
Hi Paolo, On 12/20/2021 5:08 PM, Paolo Bonzini wrote: > > On 12/17/21 16:30, Jing Liu wrote: > > Also disable read emulation of IA32_XFD_ERR MSR at the same point > > where r/w emulation of IA32_XFD MSR is disabled. This saves one > > unnecessary VM-exit in guest #NM handler, given that the MSR is > > already restored with the guest value before the guest is resumed. > > > > Signed-off-by: Jing Liu <jing2.liu@intel.com> > > Why not do this unconditionally (i.e. in patch 13, with the call to > vmx_disable_intercept_for_msr in function vmx_create_vcpu)? > Thanks for reviewing the patches. If disable unconditionally in vmx_create_vcpu, it means even guest has no cpuid, the msr read is passthrough to guest and it can read a value, which seems strange, though spec doesn't mention the read behaviour w/o cpuid. How about disabling read interception at vmx_vcpu_after_set_cpuid? if (boot_cpu_has(X86_FEATURE_XFD) && guest_cpuid_has(vcpu, X86_FEATURE_XFD)) vmx_set_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, MSR_TYPE_R, false); Thanks, Jing > Thanks, > > Paolo > > > --- > > arch/x86/kvm/vmx/vmx.c | 2 ++ > > arch/x86/kvm/vmx/vmx.h | 2 +- > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index > > 97a823a3f23f..b66a005f076b 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -163,6 +163,7 @@ static u32 > vmx_possible_passthrough_msrs[MAX_POSSIBLE_PASSTHROUGH_MSRS] = { > > MSR_GS_BASE, > > MSR_KERNEL_GS_BASE, > > MSR_IA32_XFD, > > + MSR_IA32_XFD_ERR, > > #endif > > MSR_IA32_SYSENTER_CS, > > MSR_IA32_SYSENTER_ESP, > > @@ -1934,6 +1935,7 @@ static u64 vcpu_supported_debugctl(struct > kvm_vcpu *vcpu) > > static void vmx_set_xfd_passthrough(struct kvm_vcpu *vcpu) > > { > > vmx_disable_intercept_for_msr(vcpu, MSR_IA32_XFD, > MSR_TYPE_RW); > > + vmx_disable_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, > MSR_TYPE_R); > > vcpu->arch.xfd_out_of_sync = true; > > } > > #endif > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index > > bf9d3051cd6c..0a00242a91e7 100644 > > --- a/arch/x86/kvm/vmx/vmx.h > > +++ b/arch/x86/kvm/vmx/vmx.h > > @@ -340,7 +340,7 @@ struct vcpu_vmx { > > struct lbr_desc lbr_desc; > > > > /* Save desired MSR intercept (read: pass-through) state */ > > -#define MAX_POSSIBLE_PASSTHROUGH_MSRS 14 > > +#define MAX_POSSIBLE_PASSTHROUGH_MSRS 15 > > struct { > > DECLARE_BITMAP(read, > MAX_POSSIBLE_PASSTHROUGH_MSRS); > > DECLARE_BITMAP(write, > MAX_POSSIBLE_PASSTHROUGH_MSRS); > >
On 12/21/21 07:29, Liu, Jing2 wrote: >> > Thanks for reviewing the patches. > > If disable unconditionally in vmx_create_vcpu, it means even guest has > no cpuid, the msr read is passthrough to guest and it can read a value, which > seems strange, though spec doesn't mention the read behaviour w/o cpuid. > > How about disabling read interception at vmx_vcpu_after_set_cpuid? > > if (boot_cpu_has(X86_FEATURE_XFD) && guest_cpuid_has(vcpu, X86_FEATURE_XFD)) > vmx_set_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, MSR_TYPE_R, false); Even better: if (boot_cpu_has(X86_FEATURE_XFD)) vmx_set_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, MSR_TYPE_R, !guest_cpuid_has(vcpu, X86_FEATURE_XFD)); Thanks, Paolo
On 12/21/2021 4:48 PM, Paolo Bonzini wrote: > > On 12/21/21 07:29, Liu, Jing2 wrote: > >> > > Thanks for reviewing the patches. > > > > If disable unconditionally in vmx_create_vcpu, it means even guest has > > no cpuid, the msr read is passthrough to guest and it can read a > > value, which seems strange, though spec doesn't mention the read > behaviour w/o cpuid. > > > > How about disabling read interception at vmx_vcpu_after_set_cpuid? > > > > if (boot_cpu_has(X86_FEATURE_XFD) && guest_cpuid_has(vcpu, > X86_FEATURE_XFD)) > > vmx_set_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, MSR_TYPE_R, > > false); > > Even better: > > if (boot_cpu_has(X86_FEATURE_XFD)) > vmx_set_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, > MSR_TYPE_R, > !guest_cpuid_has(vcpu, > X86_FEATURE_XFD)); Thanks Paolo. BTW do we want to put this together into patch 13 or 14, I guess you were saying patch 14
On 12/21/21 10:00, Liu, Jing2 wrote: > Thanks Paolo. > > BTW do we want to put this together into patch 13 or 14, I guess you were saying patch 14
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 97a823a3f23f..b66a005f076b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -163,6 +163,7 @@ static u32 vmx_possible_passthrough_msrs[MAX_POSSIBLE_PASSTHROUGH_MSRS] = { MSR_GS_BASE, MSR_KERNEL_GS_BASE, MSR_IA32_XFD, + MSR_IA32_XFD_ERR, #endif MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, @@ -1934,6 +1935,7 @@ static u64 vcpu_supported_debugctl(struct kvm_vcpu *vcpu) static void vmx_set_xfd_passthrough(struct kvm_vcpu *vcpu) { vmx_disable_intercept_for_msr(vcpu, MSR_IA32_XFD, MSR_TYPE_RW); + vmx_disable_intercept_for_msr(vcpu, MSR_IA32_XFD_ERR, MSR_TYPE_R); vcpu->arch.xfd_out_of_sync = true; } #endif diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index bf9d3051cd6c..0a00242a91e7 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -340,7 +340,7 @@ struct vcpu_vmx { struct lbr_desc lbr_desc; /* Save desired MSR intercept (read: pass-through) state */ -#define MAX_POSSIBLE_PASSTHROUGH_MSRS 14 +#define MAX_POSSIBLE_PASSTHROUGH_MSRS 15 struct { DECLARE_BITMAP(read, MAX_POSSIBLE_PASSTHROUGH_MSRS); DECLARE_BITMAP(write, MAX_POSSIBLE_PASSTHROUGH_MSRS);
Also disable read emulation of IA32_XFD_ERR MSR at the same point where r/w emulation of IA32_XFD MSR is disabled. This saves one unnecessary VM-exit in guest #NM handler, given that the MSR is already restored with the guest value before the guest is resumed. Signed-off-by: Jing Liu <jing2.liu@intel.com> --- arch/x86/kvm/vmx/vmx.c | 2 ++ arch/x86/kvm/vmx/vmx.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)