2022-06-13 19:14:28

by Anirudh Rayabharam

[permalink] [raw]
Subject: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

VM entry into an L2 guest on KVM on Hyper-V fails with the following
splat (stripped for brevity) when running cloud-hypervisor tests.

[ 1481.600386] WARNING: CPU: 4 PID: 7641 at arch/x86/kvm/vmx/nested.c:4563 nested_vmx_vmexit+0x70d/0x790 [kvm_intel]
[ 1481.600427] CPU: 4 PID: 7641 Comm: vcpu2 Not tainted 5.15.0-1008-azure #9-Ubuntu
[ 1481.600429] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 07/22/2021
[ 1481.600430] RIP: 0010:nested_vmx_vmexit+0x70d/0x790 [kvm_intel]
[ 1481.600447] Call Trace:
[ 1481.600449] <TASK>
[ 1481.600451] nested_vmx_reflect_vmexit+0x10b/0x440 [kvm_intel]
[ 1481.600457] __vmx_handle_exit+0xef/0x670 [kvm_intel]
[ 1481.600467] vmx_handle_exit+0x12/0x50 [kvm_intel]
[ 1481.600472] vcpu_enter_guest+0x83a/0xfd0 [kvm]
[ 1481.600524] vcpu_run+0x5e/0x240 [kvm]
[ 1481.600560] kvm_arch_vcpu_ioctl_run+0xd7/0x550 [kvm]
[ 1481.600597] kvm_vcpu_ioctl+0x29a/0x6d0 [kvm]
[ 1481.600634] __x64_sys_ioctl+0x91/0xc0
[ 1481.600637] do_syscall_64+0x5c/0xc0
[ 1481.600667] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 1481.600670] RIP: 0033:0x7f688becdaff
[ 1481.600686] </TASK>

As per the comments in arch/x86/kvm/vmx/evmcs.h, TSC multiplier field is
currently not supported in EVMCS. As a result, there is no TSC scaling
support when KVM is running on Hyper-V i.e. kvm_has_tsc_control is
false.

However, in nested_vmx_setup_ctls_msrs(), TSC scaling is exposed to L1.
When L1 tries to launch an L2 guest, vmcs12 has TSC scaling enabled.
This propagates to vmcs02. But KVM doesn't set the TSC multiplier value
because kvm_has_tsc_control is false. Due to this, VM entry for L2 guest
fails. (VM entry fails if "use TSC scaling" is 1 and TSC multiplier is 0.)

To fix, expose TSC scaling to L1 only if kvm_has_tsc_control.

Fixes: d041b5ea93352 ("KVM: nVMX: Enable nested TSC scaling")
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
arch/x86/kvm/vmx/nested.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f5cb18e00e78..d773ddc6422b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6656,6 +6656,9 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
msrs->secondary_ctls_low,
msrs->secondary_ctls_high);

+ if (!kvm_has_tsc_control)
+ msrs->secondary_ctls_high &= ~SECONDARY_EXEC_TSC_SCALING;
+
msrs->secondary_ctls_low = 0;
msrs->secondary_ctls_high &=
SECONDARY_EXEC_DESC |
@@ -6667,8 +6670,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
SECONDARY_EXEC_RDRAND_EXITING |
SECONDARY_EXEC_ENABLE_INVPCID |
SECONDARY_EXEC_RDSEED_EXITING |
- SECONDARY_EXEC_XSAVES |
- SECONDARY_EXEC_TSC_SCALING;
+ SECONDARY_EXEC_XSAVES;

/*
* We can emulate "VMCS shadowing," even if the hardware
--
2.34.1


2022-06-14 12:32:13

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

Anirudh Rayabharam <[email protected]> writes:

...

>
> As per the comments in arch/x86/kvm/vmx/evmcs.h, TSC multiplier field is
> currently not supported in EVMCS.

The latest version:
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs

has it, actually. It was missing before (compare with e.g. 6.0b version
here:
https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf)

but AFAIR TSC scaling wasn't advertised by genuine Hyper-V either.
Interestingly enough, eVMCS version didn't change when these fields were
added, it is still '1'.

I even have a patch in my stash (attached). I didn't send it out because
it wasn't properly tested with different Hyper-V versions.

--
Vitaly


Attachments:
0001-KVM-x86-Allow-some-previously-forbidden-controls-whe.patch (5.54 kB)

2022-06-14 15:12:47

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

Vitaly Kuznetsov <[email protected]> writes:

> Anirudh Rayabharam <[email protected]> writes:
>
> ...
>
>>
>> As per the comments in arch/x86/kvm/vmx/evmcs.h, TSC multiplier field is
>> currently not supported in EVMCS.
>
> The latest version:
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs
>
> has it, actually. It was missing before (compare with e.g. 6.0b version
> here:
> https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf)
>
> but AFAIR TSC scaling wasn't advertised by genuine Hyper-V either.
> Interestingly enough, eVMCS version didn't change when these fields were
> added, it is still '1'.
>
> I even have a patch in my stash (attached). I didn't send it out because
> it wasn't properly tested with different Hyper-V versions.

And of course I forgot a pre-requisite patch which updates 'struct
hv_enlightened_vmcs' to the latest:

diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 0a9407dc0859..038e5ef9b4a6 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -559,9 +559,20 @@ struct hv_enlightened_vmcs {
u64 partition_assist_page;
u64 padding64_4[4];
u64 guest_bndcfgs;
- u64 padding64_5[7];
+ u64 guest_ia32_perf_global_ctrl;
+ u64 guest_ia32_s_cet;
+ u64 guest_ssp;
+ u64 guest_ia32_int_ssp_table_addr;
+ u64 guest_ia32_lbr_ctl;
+ u64 padding64_5[2];
u64 xss_exit_bitmap;
- u64 padding64_6[7];
+ u64 host_ia32_perf_global_ctrl;
+ u64 encls_exiting_bitmap;
+ u64 tsc_multiplier;
+ u64 host_ia32_s_cet;
+ u64 host_ssp;
+ u64 host_ia32_int_ssp_table_addr;
+ u64 padding64_6;
} __packed;

#define HV_VMX_ENLIGHTENED_CLEAN_FIELD_NONE 0

--
Vitaly

2022-06-14 17:38:20

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

On 6/14/22 14:19, Vitaly Kuznetsov wrote:
> The latest version:
> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs
>
> has it, actually. It was missing before (compare with e.g. 6.0b version
> here:
> https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf)
>
> but AFAIR TSC scaling wasn't advertised by genuine Hyper-V either.
> Interestingly enough, eVMCS version didn't change when these fields were
> added, it is still '1'.
>
> I even have a patch in my stash (attached). I didn't send it out because
> it wasn't properly tested with different Hyper-V versions.
>
> -- Vitaly

Anirudh, can you check if Vitaly's patches work for you?

Paolo

2022-06-15 09:49:34

by Anirudh Rayabharam

[permalink] [raw]
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

On Tue, Jun 14, 2022 at 07:20:34PM +0200, Paolo Bonzini wrote:
> On 6/14/22 14:19, Vitaly Kuznetsov wrote:
> > The latest version:
> > https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs
> >
> > has it, actually. It was missing before (compare with e.g. 6.0b version
> > here:
> > https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf)
> >
> > but AFAIR TSC scaling wasn't advertised by genuine Hyper-V either.
> > Interestingly enough, eVMCS version didn't change when these fields were
> > added, it is still '1'.
> >
> > I even have a patch in my stash (attached). I didn't send it out because
> > it wasn't properly tested with different Hyper-V versions.
> >
> > -- Vitaly
>
> Anirudh, can you check if Vitaly's patches work for you?

I will check it. But I wonder if they fit the criteria for inclusion in
stable trees...

It is important for the fix to land in the stable trees since this issue
is a regression that was introduced _after_ 5.13. (I probably should've
mentioned this in the changelog.)

Thanks,

Anirudh.

>
> Paolo

2022-06-15 09:51:06

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

Anirudh Rayabharam <[email protected]> writes:

> On Tue, Jun 14, 2022 at 07:20:34PM +0200, Paolo Bonzini wrote:
>> On 6/14/22 14:19, Vitaly Kuznetsov wrote:
>> > The latest version:
>> > https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs
>> >
>> > has it, actually. It was missing before (compare with e.g. 6.0b version
>> > here:
>> > https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf)
>> >
>> > but AFAIR TSC scaling wasn't advertised by genuine Hyper-V either.
>> > Interestingly enough, eVMCS version didn't change when these fields were
>> > added, it is still '1'.
>> >
>> > I even have a patch in my stash (attached). I didn't send it out because
>> > it wasn't properly tested with different Hyper-V versions.
>> >
>> > -- Vitaly
>>
>> Anirudh, can you check if Vitaly's patches work for you?
>
> I will check it. But I wonder if they fit the criteria for inclusion in
> stable trees...
>
> It is important for the fix to land in the stable trees since this issue
> is a regression that was introduced _after_ 5.13. (I probably should've
> mentioned this in the changelog.)
>

Personally, I see no problem with splitting off TscMultiplier part from
my patch and marking it for stable@ fixing d041b5ea93352. I'm going to
run some tests too.

--
Vitaly

2022-06-15 11:51:52

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] KVM: nVMX: Don't expose TSC scaling to L1 when on Hyper-V

Vitaly Kuznetsov <[email protected]> writes:

> Vitaly Kuznetsov <[email protected]> writes:
>
>> Anirudh Rayabharam <[email protected]> writes:
>>
>> ...
>>
>>>
>>> As per the comments in arch/x86/kvm/vmx/evmcs.h, TSC multiplier field is
>>> currently not supported in EVMCS.
>>
>> The latest version:
>> https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/datatypes/hv_vmx_enlightened_vmcs
>>
>> has it, actually. It was missing before (compare with e.g. 6.0b version
>> here:
>> https://github.com/MicrosoftDocs/Virtualization-Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v6.0b.pdf)
>>
>> but AFAIR TSC scaling wasn't advertised by genuine Hyper-V either.
>> Interestingly enough, eVMCS version didn't change when these fields were
>> added, it is still '1'.
>>
>> I even have a patch in my stash (attached). I didn't send it out because
>> it wasn't properly tested with different Hyper-V versions.
>
> And of course I forgot a pre-requisite patch which updates 'struct
> hv_enlightened_vmcs' to the latest:
>

The good news is that TscMultiplies seems to work fine for me, at least
with an Azure Dv5 instance where I can see Tsc scaling exposed. The bad
news is that a few more patches are needed:

1) Fix 'struct hv_enlightened_vmcs' definition:
https://lore.kernel.org/kvm/[email protected]/

2) Define VMCS-to-EVMCS conversion for the new fields :

diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 6a61b1ae7942..707a8de11802 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -28,6 +28,8 @@ const struct evmcs_field vmcs_field_to_evmcs_1[] = {
HV_VMX_ENLIGHTENED_CLEAN_FIELD_HOST_GRP1),
EVMCS1_FIELD(HOST_IA32_EFER, host_ia32_efer,
HV_VMX_ENLIGHTENED_CLEAN_FIELD_HOST_GRP1),
+ EVMCS1_FIELD(HOST_IA32_PERF_GLOBAL_CTRL, host_ia32_perf_global_ctrl,
+ HV_VMX_ENLIGHTENED_CLEAN_FIELD_HOST_GRP1),
EVMCS1_FIELD(HOST_CR0, host_cr0,
HV_VMX_ENLIGHTENED_CLEAN_FIELD_HOST_GRP1),
EVMCS1_FIELD(HOST_CR3, host_cr3,
@@ -78,6 +80,8 @@ const struct evmcs_field vmcs_field_to_evmcs_1[] = {
HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1),
EVMCS1_FIELD(GUEST_IA32_EFER, guest_ia32_efer,
HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1),
+ EVMCS1_FIELD(GUEST_IA32_PERF_GLOBAL_CTRL, guest_ia32_perf_global_ctrl,
+ HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1),
EVMCS1_FIELD(GUEST_PDPTR0, guest_pdptr0,
HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1),
EVMCS1_FIELD(GUEST_PDPTR1, guest_pdptr1,
@@ -126,24 +130,47 @@ const struct evmcs_field vmcs_field_to_evmcs_1[] = {
HV_VMX_ENLIGHTENED_CLEAN_FIELD_GUEST_GRP1),
EVMCS1_FIELD(XSS_EXIT_BITMAP, xss_exit_bitmap,
HV_VMX_ENLIGHTENED_CLEAN_FIELD_CONTROL_GRP2),
+ EVMCS1_FIELD(ENCLS_EXITING_BITMAP, encls_exiting_bitmap,
+ HV_VMX_ENLIGHTENED_CLEAN_FIELD_CONTROL_GRP2),
+ EVMCS1_FIELD(TSC_MULTIPLIER, tsc_multiplier,
+ HV_VMX_ENLIGHTENED_CLEAN_FIELD_CONTROL_GRP2),

...

so it is becoming more and more complicated to assemble for testing. Let
me finish my testing and I'll put a series together and send it out to
simplify the process. Stay tuned!

--
Vitaly