2019-01-24 17:15:57

by Vitaly Kuznetsov

[permalink] [raw]
Subject: [PATCH] x86/kvm/hyper-v: tweak HYPERV_CPUID_ENLIGHTMENT_INFO

We shouldn't probably be suggesting using Enlightened VMCS when it's not
enabled (not supported from guest's point of view). System reset through
synthetic MSR is not recommended neither by genuine Hyper-V nor my QEMU.

Windows seems to be fine either way but let's be consistent.

Fixes: 2bc39970e932 ("x86/kvm/hyper-v: Introduce KVM_GET_SUPPORTED_HV_CPUID")
Signed-off-by: Vitaly Kuznetsov <[email protected]>
---
arch/x86/kvm/hyperv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index ac44a681f065..4730fcaa70cf 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1847,11 +1847,11 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
case HYPERV_CPUID_ENLIGHTMENT_INFO:
ent->eax |= HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED;
ent->eax |= HV_X64_APIC_ACCESS_RECOMMENDED;
- ent->eax |= HV_X64_SYSTEM_RESET_RECOMMENDED;
ent->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
ent->eax |= HV_X64_CLUSTER_IPI_RECOMMENDED;
ent->eax |= HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED;
- ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
+ if (evmcs_ver)
+ ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;

/*
* Default number of spinlock retry attempts, matches
--
2.20.1



2019-01-24 17:29:06

by Liran Alon

[permalink] [raw]
Subject: Re: [PATCH] x86/kvm/hyper-v: tweak HYPERV_CPUID_ENLIGHTMENT_INFO



> On 24 Jan 2019, at 19:15, Vitaly Kuznetsov <[email protected]> wrote:
>
> We shouldn't probably be suggesting using Enlightened VMCS when it's not
> enabled (not supported from guest's point of view). System reset through
> synthetic MSR is not recommended neither by genuine Hyper-V nor my QEMU.
>
> Windows seems to be fine either way but let's be consistent.
>
> Fixes: 2bc39970e932 ("x86/kvm/hyper-v: Introduce KVM_GET_SUPPORTED_HV_CPUID")
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> ---
> arch/x86/kvm/hyperv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index ac44a681f065..4730fcaa70cf 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1847,11 +1847,11 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> case HYPERV_CPUID_ENLIGHTMENT_INFO:
> ent->eax |= HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED;
> ent->eax |= HV_X64_APIC_ACCESS_RECOMMENDED;
> - ent->eax |= HV_X64_SYSTEM_RESET_RECOMMENDED;
> ent->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
> ent->eax |= HV_X64_CLUSTER_IPI_RECOMMENDED;
> ent->eax |= HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED;
> - ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
> + if (evmcs_ver)
> + ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>
> /*
> * Default number of spinlock retry attempts, matches
> --
> 2.20.1
>

Seems to me that there are 2 unrelated separated patches here. Why not split them?
For content itself: Reviewed-by: Liran Alon <[email protected]>


2019-01-24 17:41:29

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] x86/kvm/hyper-v: tweak HYPERV_CPUID_ENLIGHTMENT_INFO

Liran Alon <[email protected]> writes:

>> On 24 Jan 2019, at 19:15, Vitaly Kuznetsov <[email protected]> wrote:
>>
>> We shouldn't probably be suggesting using Enlightened VMCS when it's not
>> enabled (not supported from guest's point of view). System reset through
>> synthetic MSR is not recommended neither by genuine Hyper-V nor my QEMU.
>>
>> Windows seems to be fine either way but let's be consistent.
>>
>> Fixes: 2bc39970e932 ("x86/kvm/hyper-v: Introduce KVM_GET_SUPPORTED_HV_CPUID")
>> Signed-off-by: Vitaly Kuznetsov <[email protected]>
>> ---
>> arch/x86/kvm/hyperv.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>> index ac44a681f065..4730fcaa70cf 100644
>> --- a/arch/x86/kvm/hyperv.c
>> +++ b/arch/x86/kvm/hyperv.c
>> @@ -1847,11 +1847,11 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>> case HYPERV_CPUID_ENLIGHTMENT_INFO:
>> ent->eax |= HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED;
>> ent->eax |= HV_X64_APIC_ACCESS_RECOMMENDED;
>> - ent->eax |= HV_X64_SYSTEM_RESET_RECOMMENDED;
>> ent->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
>> ent->eax |= HV_X64_CLUSTER_IPI_RECOMMENDED;
>> ent->eax |= HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED;
>> - ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>> + if (evmcs_ver)
>> + ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>>
>> /*
>> * Default number of spinlock retry attempts, matches
>> --
>> 2.20.1
>>
>
> Seems to me that there are 2 unrelated separated patches here. Why not
> split them?

They seem to be too small :-) No problem, I'll split them up in v2.

> For content itself: Reviewed-by: Liran Alon <[email protected]>
>

Thanks!

--
Vitaly

2019-01-24 17:42:17

by Liran Alon

[permalink] [raw]
Subject: Re: [PATCH] x86/kvm/hyper-v: tweak HYPERV_CPUID_ENLIGHTMENT_INFO



> On 24 Jan 2019, at 19:39, Vitaly Kuznetsov <[email protected]> wrote:
>
> Liran Alon <[email protected]> writes:
>
>>> On 24 Jan 2019, at 19:15, Vitaly Kuznetsov <[email protected]> wrote:
>>>
>>> We shouldn't probably be suggesting using Enlightened VMCS when it's not
>>> enabled (not supported from guest's point of view). System reset through
>>> synthetic MSR is not recommended neither by genuine Hyper-V nor my QEMU.
>>>
>>> Windows seems to be fine either way but let's be consistent.
>>>
>>> Fixes: 2bc39970e932 ("x86/kvm/hyper-v: Introduce KVM_GET_SUPPORTED_HV_CPUID")
>>> Signed-off-by: Vitaly Kuznetsov <[email protected]>
>>> ---
>>> arch/x86/kvm/hyperv.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>>> index ac44a681f065..4730fcaa70cf 100644
>>> --- a/arch/x86/kvm/hyperv.c
>>> +++ b/arch/x86/kvm/hyperv.c
>>> @@ -1847,11 +1847,11 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>>> case HYPERV_CPUID_ENLIGHTMENT_INFO:
>>> ent->eax |= HV_X64_REMOTE_TLB_FLUSH_RECOMMENDED;
>>> ent->eax |= HV_X64_APIC_ACCESS_RECOMMENDED;
>>> - ent->eax |= HV_X64_SYSTEM_RESET_RECOMMENDED;
>>> ent->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
>>> ent->eax |= HV_X64_CLUSTER_IPI_RECOMMENDED;
>>> ent->eax |= HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED;
>>> - ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>>> + if (evmcs_ver)
>>> + ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>>>
>>> /*
>>> * Default number of spinlock retry attempts, matches
>>> --
>>> 2.20.1
>>>
>>
>> Seems to me that there are 2 unrelated separated patches here. Why not
>> split them?
>
> They seem to be too small :-) No problem, I'll split them up in v2.

I don’t think in general that it matters how small they are.
Separating to small logical patches allows better bisect, easier review and better revert resolution. So better overall. :)

>
>> For content itself: Reviewed-by: Liran Alon <[email protected]>
>>
>
> Thanks!
>
> --
> Vitaly