| Message ID | 20170818184310.117806-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.08.2017 20:43, Jim Mattson wrote: > A guest may not be configured to support RDSEED, even when the host > does. If the guest does not support RDSEED, intercept the instruction > and synthesize #UD. Would the same also hold for nVMX guests? I think if our L1 CPU does not have RSEED, then also the L2 CPU should not be allowed to use it. @@ -10371,6 +10371,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, SECONDARY_EXEC_RDTSCP | SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | SECONDARY_EXEC_APIC_REGISTER_VIRT | + SECONDARY_EXEC_RDSEED_EXITING | SECONDARY_EXEC_ENABLE_VMFUNC); if (nested_cpu_has(vmcs12, CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) { and maybe also +++ b/arch/x86/kvm/vmx.c @@ -2811,6 +2811,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) SECONDARY_EXEC_RDRAND | SECONDARY_EXEC_RDSEED | SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES | SECONDARY_EXEC_RDTSCP | + SECONDARY_EXEC_RDSEED_EXITING | SECONDARY_EXEC_DESC | SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE | SECONDARY_EXEC_APIC_REGISTER_VIRT | (but I always get confused about the level of filtering) > --- > arch/x86/kvm/vmx.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index ed1074e98b8e..8b9015f081b7 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3662,6 +3662,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | > SECONDARY_EXEC_SHADOW_VMCS | > SECONDARY_EXEC_XSAVES | > + SECONDARY_EXEC_RDSEED_EXITING | > SECONDARY_EXEC_ENABLE_PML | > SECONDARY_EXEC_TSC_SCALING | > SECONDARY_EXEC_ENABLE_VMFUNC; > @@ -5298,6 +5299,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx) > if (!enable_pml) > exec_control &= ~SECONDARY_EXEC_ENABLE_PML; > > + if (guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDSEED)) > + exec_control &= ~SECONDARY_EXEC_RDSEED_EXITING; > + > return exec_control; > } > > @@ -6806,6 +6810,12 @@ static int handle_mwait(struct kvm_vcpu *vcpu) > return handle_nop(vcpu); > } > > +static int handle_invalid_op(struct kvm_vcpu *vcpu) > +{ > + kvm_queue_exception(vcpu, UD_VECTOR); > + return 1; > +} > + (unrelated to this patch) just wondering if we should now replace most code fragments kvm_queue_exception(vcpu, UD_VECTOR); return 1; by return handle_invalid_op(vcpu); > static int handle_monitor_trap(struct kvm_vcpu *vcpu) > { > return 1; > @@ -8050,6 +8060,7 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = { > [EXIT_REASON_MONITOR_INSTRUCTION] = handle_monitor, > [EXIT_REASON_INVEPT] = handle_invept, > [EXIT_REASON_INVVPID] = handle_invvpid, > + [EXIT_REASON_RDSEED] = handle_invalid_op, > [EXIT_REASON_XSAVES] = handle_xsaves, > [EXIT_REASON_XRSTORS] = handle_xrstors, > [EXIT_REASON_PML_FULL] = handle_pml_full, >
Right. If L1 doesn't support RDSEED, then the corresponding "allowed-1" bit in the IA32_VMX_PROCBASED_CTLS2 MSR should be cleared. I think vmx_cpuid_update is the right place for this. Note, however, that prepare_vmcs02() should still respect L0's setting of this bit. On Mon, Aug 21, 2017 at 6:00 AM, David Hildenbrand <david@redhat.com> wrote: > On 18.08.2017 20:43, Jim Mattson wrote: >> A guest may not be configured to support RDSEED, even when the host >> does. If the guest does not support RDSEED, intercept the instruction >> and synthesize #UD. > > Would the same also hold for nVMX guests? I think if our L1 CPU does not > have RSEED, then also the L2 CPU should not be allowed to use it. > > @@ -10371,6 +10371,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, > struct vmcs12 *vmcs12, > SECONDARY_EXEC_RDTSCP | > SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | > SECONDARY_EXEC_APIC_REGISTER_VIRT | > + SECONDARY_EXEC_RDSEED_EXITING | > SECONDARY_EXEC_ENABLE_VMFUNC); > if (nested_cpu_has(vmcs12, > CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)) { > > > and maybe also > > > +++ b/arch/x86/kvm/vmx.c > @@ -2811,6 +2811,7 @@ static void nested_vmx_setup_ctls_msrs(struct > vcpu_vmx *vmx) > SECONDARY_EXEC_RDRAND | SECONDARY_EXEC_RDSEED | > SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES | > SECONDARY_EXEC_RDTSCP | > + SECONDARY_EXEC_RDSEED_EXITING | > SECONDARY_EXEC_DESC | > SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE | > SECONDARY_EXEC_APIC_REGISTER_VIRT | > > (but I always get confused about the level of filtering) > >> --- >> arch/x86/kvm/vmx.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index ed1074e98b8e..8b9015f081b7 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -3662,6 +3662,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) >> SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | >> SECONDARY_EXEC_SHADOW_VMCS | >> SECONDARY_EXEC_XSAVES | >> + SECONDARY_EXEC_RDSEED_EXITING | >> SECONDARY_EXEC_ENABLE_PML | >> SECONDARY_EXEC_TSC_SCALING | >> SECONDARY_EXEC_ENABLE_VMFUNC; >> @@ -5298,6 +5299,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx) >> if (!enable_pml) >> exec_control &= ~SECONDARY_EXEC_ENABLE_PML; >> >> + if (guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDSEED)) >> + exec_control &= ~SECONDARY_EXEC_RDSEED_EXITING; >> + >> return exec_control; >> } >> >> @@ -6806,6 +6810,12 @@ static int handle_mwait(struct kvm_vcpu *vcpu) >> return handle_nop(vcpu); >> } >> >> +static int handle_invalid_op(struct kvm_vcpu *vcpu) >> +{ >> + kvm_queue_exception(vcpu, UD_VECTOR); >> + return 1; >> +} >> + > > (unrelated to this patch) > just wondering if we should now replace most code fragments > > kvm_queue_exception(vcpu, UD_VECTOR); > return 1; > > by > > return handle_invalid_op(vcpu); > > >> static int handle_monitor_trap(struct kvm_vcpu *vcpu) >> { >> return 1; >> @@ -8050,6 +8060,7 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = { >> [EXIT_REASON_MONITOR_INSTRUCTION] = handle_monitor, >> [EXIT_REASON_INVEPT] = handle_invept, >> [EXIT_REASON_INVVPID] = handle_invvpid, >> + [EXIT_REASON_RDSEED] = handle_invalid_op, >> [EXIT_REASON_XSAVES] = handle_xsaves, >> [EXIT_REASON_XRSTORS] = handle_xrstors, >> [EXIT_REASON_PML_FULL] = handle_pml_full, >> > > > -- > > Thanks, > > David
On 21.08.2017 18:37, Jim Mattson wrote: > Right. If L1 doesn't support RDSEED, then the corresponding > "allowed-1" bit in the IA32_VMX_PROCBASED_CTLS2 MSR should be cleared. > I think vmx_cpuid_update is the right place for this. Note, however, > that prepare_vmcs02() should still respect L0's setting of this bit. > Right, now I also realize why you sent v2 (EXITING vs. !EXITING) :)
Perhaps this change to vmx_cpuid_update is too simplistic, and it should follow the pattern established for RDTSCP/INVPCID instead? On Mon, Aug 21, 2017 at 9:50 AM, David Hildenbrand <david@redhat.com> wrote: > On 21.08.2017 18:37, Jim Mattson wrote: >> Right. If L1 doesn't support RDSEED, then the corresponding >> "allowed-1" bit in the IA32_VMX_PROCBASED_CTLS2 MSR should be cleared. >> I think vmx_cpuid_update is the right place for this. Note, however, >> that prepare_vmcs02() should still respect L0's setting of this bit. >> > > Right, now I also realize why you sent v2 (EXITING vs. !EXITING) :) > > > -- > > Thanks, > > David
There is a potential conflict here between KVM_SET_CPUID and KVM_SET_MSRS, if the two don't agree about the setting of the RDSEED-exiting "allowed-1" bit in the IA32_VMX_PROCBASED_CTLS2 MSR. But we already have that issue with the "allowed-1" bit for "enable RDTSCP," so this is nothing new. Surprisingly, we don't have that issue with the "allowed-1" setting for "enable INVPCID," because vmx_cpuid_update() doesn't make any attempt to update that bit. On Mon, Aug 21, 2017 at 10:01 AM, Jim Mattson <jmattson@google.com> wrote: > Perhaps this change to vmx_cpuid_update is too simplistic, and it > should follow the pattern established for RDTSCP/INVPCID instead? > > On Mon, Aug 21, 2017 at 9:50 AM, David Hildenbrand <david@redhat.com> wrote: >> On 21.08.2017 18:37, Jim Mattson wrote: >>> Right. If L1 doesn't support RDSEED, then the corresponding >>> "allowed-1" bit in the IA32_VMX_PROCBASED_CTLS2 MSR should be cleared. >>> I think vmx_cpuid_update is the right place for this. Note, however, >>> that prepare_vmcs02() should still respect L0's setting of this bit. >>> >> >> Right, now I also realize why you sent v2 (EXITING vs. !EXITING) :) >> >> >> -- >> >> Thanks, >> >> David
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ed1074e98b8e..8b9015f081b7 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -3662,6 +3662,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | SECONDARY_EXEC_SHADOW_VMCS | SECONDARY_EXEC_XSAVES | + SECONDARY_EXEC_RDSEED_EXITING | SECONDARY_EXEC_ENABLE_PML | SECONDARY_EXEC_TSC_SCALING | SECONDARY_EXEC_ENABLE_VMFUNC; @@ -5298,6 +5299,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx) if (!enable_pml) exec_control &= ~SECONDARY_EXEC_ENABLE_PML; + if (guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDSEED)) + exec_control &= ~SECONDARY_EXEC_RDSEED_EXITING; + return exec_control; } @@ -6806,6 +6810,12 @@ static int handle_mwait(struct kvm_vcpu *vcpu) return handle_nop(vcpu); } +static int handle_invalid_op(struct kvm_vcpu *vcpu) +{ + kvm_queue_exception(vcpu, UD_VECTOR); + return 1; +} + static int handle_monitor_trap(struct kvm_vcpu *vcpu) { return 1; @@ -8050,6 +8060,7 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = { [EXIT_REASON_MONITOR_INSTRUCTION] = handle_monitor, [EXIT_REASON_INVEPT] = handle_invept, [EXIT_REASON_INVVPID] = handle_invvpid, + [EXIT_REASON_RDSEED] = handle_invalid_op, [EXIT_REASON_XSAVES] = handle_xsaves, [EXIT_REASON_XRSTORS] = handle_xrstors, [EXIT_REASON_PML_FULL] = handle_pml_full,