On Mon, Aug 21, 2017 at 1:32 PM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote:
> 2017-08-21 12:26-0700, Jim Mattson:
>> 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. Also clear the "allowed-1" bit for RDSEED exiting
>> in the IA32_VMX_PROCBASED_CTLS2 MSR.
>
> (RDRAND looks the same.)
Agreed. A general paradigm would be nicer.
>
>> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
>> ---
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> @@ -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;
>> +
>> return exec_control;
>> }
>>
>> @@ -9665,6 +9682,24 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>> }
>> }
>>
>> + if (vmx_rdseed_supported()) {
>> + bool rdseed_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDSEED);
>> +
>> + if (rdseed_enabled)
>> + secondary_exec_ctl &= ~SECONDARY_EXEC_RDSEED;
>
> All other CPUID-controlled features use vmx_cpuid_update(), but I would
> actually prefer to have it in vmx_secondary_exec_control. In any case,
> combining those two is weird.
Yes, it is weird. The reasons I adopted this two-pronged approach are:
1. vmx_secondary_exec_control() is only used at VCPU creation time to
set the default controls for the vmcs01.
2. Changes due to KVM_SET_CPUID2 are pushed into the vmcs01 by
vmx_cpuid_update() [assuming that the vcpu is not in virtualized vmx
non-root mode when the ioctl is called].
3. vmx_secondary_exec_control() is used at every emulated VM-entry
from L1 to L2 to determine the controls that L0 needs to set in the
vmcs02.
Ideally, vmx_secondary_exec_control() would always return the
secondary processor-based execution controls for vmcs01, but too many
changes are already being made behind its back.
>
>> + else
>> + secondary_exec_ctl |= SECONDARY_EXEC_RDSEED;
>
> The feature can never be unset here, so we can have just the first
> branch,
I believe that CPUID.07H:EBX.RDSEED can be unset by
KVM_SET_CPUID/KVM_SET_CPUID2 (and both ioctls pass through here). How
else would you defeature a VCPU?
> thanks.
>
>> +
>> + if (nested) {
>> + if (rdseed_enabled)
>> + vmx->nested.nested_vmx_secondary_ctls_high |=
>> + SECONDARY_EXEC_RDSEED;
Arguably, this isn't necessary. A sane platform could have support for
the RDSEED instruction without support for "RDSEED exiting." The same
applies to INVPCID, in the code above.
>> + else
>> + vmx->nested.nested_vmx_secondary_ctls_high &=
>> + ~SECONDARY_EXEC_RDSEED;
>> + }
>> + }
>
> I think it would be nicer to generalize that pattern:
In general, "<opcode> exiting" or enable <opcode>" VMX capabilities
should be forced off if the CPUID feature bit for the opcode is
cleared (except, perhaps, for the MONITOR/MWAIT oddity).
> (We can call it after updating the MSRs too.)
After updating the MSRs, perhaps the CPUID feature bit for the opcode
should be forced on if the corresponding "allowed-1" VMX capability
bit is set. Or, perhaps it should be an error to set the VMX
capability bit via KVM_SET_MSRS after having already cleared the
corresponding CPUID feature bit via KVM_SET_CPUID2.
>
> ---8<---
> Subject: [PATCH] KVM: nVMX: refactor secondary_ctls_high updates
>
> We should not enable a VMX feature if its instruction is not in guest
> CPUID or not provided by hardware. The change allows us to easily add
> more features.
>
> RDTSCP will always get configured with CPUID, so there is no need to set
> it from the beginning, just like INVPCID.
Isn't RDTSCP automatically set in the default CPUID if there is
hardware support for "enable RDTSCP"? When the default CPUID is used,
there is no call to kvm_x86_ops->cpuid_update. Shouldn't the default
VMX capability MSRs match the default CPUID settings?
>
> Signed-off-by: Radim Krčmář <rkrcmar@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 46 +++++++++++++++++++++-------------------------
> 1 file changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 2b92c2de2b3a..7e2b33e0948d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2810,7 +2810,6 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
> vmx->nested.nested_vmx_secondary_ctls_high &=
> SECONDARY_EXEC_RDRAND | SECONDARY_EXEC_RDSEED |
> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> - SECONDARY_EXEC_RDTSCP |
> SECONDARY_EXEC_DESC |
> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
> SECONDARY_EXEC_APIC_REGISTER_VIRT |
> @@ -9623,25 +9622,28 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
> #undef cr4_fixed1_update
> }
>
> +/*
> + * Update MSR_IA32_VMX_PROCBASED_CTLS2 according to CPUID. Selected features
> + * are enabled iff they are enabled in CPUID and supported by the host.
> + */
> +static void nested_vmx_secondary_ctls_high_update(struct kvm_vcpu *vcpu,
> + u32 host_secondary_exec_ctl)
> +{
> + u32 mask = SECONDARY_EXEC_RDTSCP | SECONDARY_EXEC_ENABLE_INVPCID;
> +
> + vcpu->vmx.nested.nested_vmx_secondary_ctls_high &= ~mask
> + vcpu->vmx.nested.nested_vmx_secondary_ctls_high |=
> + host_secondary_exec_ctl & mask;
> +}
> +
> static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
>
> - if (vmx_rdtscp_supported()) {
> - bool rdtscp_enabled = guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP);
> - if (!rdtscp_enabled)
> - secondary_exec_ctl &= ~SECONDARY_EXEC_RDTSCP;
> -
> - if (nested) {
> - if (rdtscp_enabled)
> - vmx->nested.nested_vmx_secondary_ctls_high |=
> - SECONDARY_EXEC_RDTSCP;
> - else
> - vmx->nested.nested_vmx_secondary_ctls_high &=
> - ~SECONDARY_EXEC_RDTSCP;
> - }
> - }
> + if (vmx_rdtscp_supported() &&
> + !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> + secondary_exec_ctl &= ~SECONDARY_EXEC_RDTSCP;
>
> if (vmx_invpcid_supported()) {
> /* Exposing INVPCID only when PCID is exposed */
> @@ -9653,15 +9655,6 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID);
> }
> -
> - if (nested) {
> - if (invpcid_enabled)
> - vmx->nested.nested_vmx_secondary_ctls_high |=
> - SECONDARY_EXEC_ENABLE_INVPCID;
> - else
> - vmx->nested.nested_vmx_secondary_ctls_high &=
> - ~SECONDARY_EXEC_ENABLE_INVPCID;
> - }
> }
>
> if (cpu_has_secondary_exec_ctrls())
> @@ -9674,8 +9667,11 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
> ~FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
>
> - if (nested_vmx_allowed(vcpu))
> + if (nested_vmx_allowed(vcpu)) {
> nested_vmx_cr_fixed1_bits_update(vcpu);
> + nested_vmx_secondary_ctls_high_update(vcpu, secondary_exec_ctl);
> + }
> +
> }
>
> static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> --
> 2.13.3
>
