[PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Liran Alon]

Since commits 668fffa3f838 ("kvm: better MWAIT emulation for guests”)
and 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts”),
KVM was modified to allow an admin to configure certain guests to execute
MONITOR/MWAIT inside guest without being intercepted by host.

This is useful in case admin wishes to allocate a dedicated logical
processor for each vCPU thread. Thus, making it safe for guest to
completely control the power-state of the logical processor.

The ability to use this new KVM capability was introduced to QEMU by
commits 6f131f13e68d ("kvm: support -overcommit cpu-pm=on|off”) and
2266d4431132 ("i386/cpu: make -cpu host support monitor/mwait”).

However, exposing MONITOR/MWAIT to a Linux guest may cause it's intel_idle
kernel module to execute c1e_promotion_disable() which will attempt to
RDMSR/WRMSR from/to MSR_IA32_POWER_CTL to manipulate the "C1E Enable"
bit. This behaviour was introduced by commit
32e9518005c8 ("intel_idle: export both C1 and C1E”).

Becuase KVM doesn't emulate this MSR, running KVM with ignore_msrs=0
will cause the above guest behaviour to raise a #GP which will cause
guest to kernel panic.

Therefore, add support for nop emulation of MSR_IA32_POWER_CTL to
avoid #GP in guest in this scenario.

Future commits can optimise emulation further by reflecting guest
MSR changes to host MSR to provide guest with the ability to
fine-tune the dedicated logical processor power-state.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 arch/x86/kvm/vmx/vmx.c | 6 ++++++
 arch/x86/kvm/vmx/vmx.h | 2 ++
 arch/x86/kvm/x86.c     | 1 +
 3 files changed, 9 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 2634ee8c9dc8..6246d782b746 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1696,6 +1696,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_SYSENTER_ESP:
 		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
+	case MSR_IA32_POWER_CTL:
+		msr_info->data = vmx->msr_ia32_power_ctl;
+		break;
 	case MSR_IA32_BNDCFGS:
 		if (!kvm_mpx_supported() ||
 		    (!msr_info->host_initiated &&
@@ -1826,6 +1829,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_SYSENTER_ESP:
 		vmcs_writel(GUEST_SYSENTER_ESP, data);
 		break;
+	case MSR_IA32_POWER_CTL:
+		vmx->msr_ia32_power_ctl = data;
+		break;
 	case MSR_IA32_BNDCFGS:
 		if (!kvm_mpx_supported() ||
 		    (!msr_info->host_initiated &&
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 99328954c2fc..e9772850a2a1 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -259,6 +259,8 @@ struct vcpu_vmx {
 
 	unsigned long host_debugctlmsr;
 
+	u64 msr_ia32_power_ctl;
+
 	/*
 	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
 	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 02c8e095a239..39ee4087f954 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1167,6 +1167,7 @@ static u32 emulated_msrs[] = {
 	MSR_PLATFORM_INFO,
 	MSR_MISC_FEATURES_ENABLES,
 	MSR_AMD64_VIRT_SPEC_CTRL,
+	MSR_IA32_POWER_CTL,
 };
 
 static unsigned num_emulated_msrs;
-- 
2.20.1

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Sean Christopherson]

On Mon, Apr 15, 2019 at 06:45:26PM +0300, Liran Alon wrote:
> Since commits 668fffa3f838 ("kvm: better MWAIT emulation for guests”)
> and 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts”),
> KVM was modified to allow an admin to configure certain guests to execute
> MONITOR/MWAIT inside guest without being intercepted by host.
> 
> This is useful in case admin wishes to allocate a dedicated logical
> processor for each vCPU thread. Thus, making it safe for guest to
> completely control the power-state of the logical processor.
> 
> The ability to use this new KVM capability was introduced to QEMU by
> commits 6f131f13e68d ("kvm: support -overcommit cpu-pm=on|off”) and
> 2266d4431132 ("i386/cpu: make -cpu host support monitor/mwait”).
> 
> However, exposing MONITOR/MWAIT to a Linux guest may cause it's intel_idle
                                                             ^^^^
                                                             its

English is a wonderful language...

> kernel module to execute c1e_promotion_disable() which will attempt to
> RDMSR/WRMSR from/to MSR_IA32_POWER_CTL to manipulate the "C1E Enable"
> bit. This behaviour was introduced by commit
> 32e9518005c8 ("intel_idle: export both C1 and C1E”).

Technically, I think this is a Qemu bug.  KVM reports all zeros for
CPUID_MWAIT_LEAF when userspace queries KVM_GET_SUPPORTED_CPUID and
KVM_GET_EMULATED_CPUID.  And I think that's correct/desired, supporting
MONITOR/MWAIT sub-features should be a separate enabling patch set.

Note, there is a virtualization hole regarding MWAIT as KVM can't
intercept MWAIT when executed with unsupported hints/features, but
I don't think that absolves Qemu of wrongdoing.

> Becuase KVM doesn't emulate this MSR, running KVM with ignore_msrs=0
> will cause the above guest behaviour to raise a #GP which will cause
> guest to kernel panic.
> 
> Therefore, add support for nop emulation of MSR_IA32_POWER_CTL to
> avoid #GP in guest in this scenario.
> 
> Future commits can optimise emulation further by reflecting guest
> MSR changes to host MSR to provide guest with the ability to
> fine-tune the dedicated logical processor power-state.
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 6 ++++++
>  arch/x86/kvm/vmx/vmx.h | 2 ++
>  arch/x86/kvm/x86.c     | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 2634ee8c9dc8..6246d782b746 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1696,6 +1696,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_SYSENTER_ESP:
>  		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
>  		break;
> +	case MSR_IA32_POWER_CTL:
> +		msr_info->data = vmx->msr_ia32_power_ctl;
> +		break;
>  	case MSR_IA32_BNDCFGS:
>  		if (!kvm_mpx_supported() ||
>  		    (!msr_info->host_initiated &&
> @@ -1826,6 +1829,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_SYSENTER_ESP:
>  		vmcs_writel(GUEST_SYSENTER_ESP, data);
>  		break;
> +	case MSR_IA32_POWER_CTL:
> +		vmx->msr_ia32_power_ctl = data;
> +		break;
>  	case MSR_IA32_BNDCFGS:
>  		if (!kvm_mpx_supported() ||
>  		    (!msr_info->host_initiated &&

If KVM does go the route of advertising MWAIT/MONITOR sub-features, then I
think the MSR needs to be emulated on both Intel and AMD.  Glancing through
drivers/idle/intel_idle.c, I don't see anything that prevents it from
successfully probing an "Intel" vCPU that is being emulated on AMD hardware.

> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 99328954c2fc..e9772850a2a1 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -259,6 +259,8 @@ struct vcpu_vmx {
>  
>  	unsigned long host_debugctlmsr;
>  
> +	u64 msr_ia32_power_ctl;
> +
>  	/*
>  	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
>  	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 02c8e095a239..39ee4087f954 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1167,6 +1167,7 @@ static u32 emulated_msrs[] = {
>  	MSR_PLATFORM_INFO,
>  	MSR_MISC_FEATURES_ENABLES,
>  	MSR_AMD64_VIRT_SPEC_CTRL,
> +	MSR_IA32_POWER_CTL,
>  };
>  
>  static unsigned num_emulated_msrs;
> -- 
> 2.20.1
> 

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Liran Alon]

> On 15 Apr 2019, at 21:17, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Mon, Apr 15, 2019 at 06:45:26PM +0300, Liran Alon wrote:
>> Since commits 668fffa3f838 ("kvm: better MWAIT emulation for guests”)
>> and 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts”),
>> KVM was modified to allow an admin to configure certain guests to execute
>> MONITOR/MWAIT inside guest without being intercepted by host.
>> 
>> This is useful in case admin wishes to allocate a dedicated logical
>> processor for each vCPU thread. Thus, making it safe for guest to
>> completely control the power-state of the logical processor.
>> 
>> The ability to use this new KVM capability was introduced to QEMU by
>> commits 6f131f13e68d ("kvm: support -overcommit cpu-pm=on|off”) and
>> 2266d4431132 ("i386/cpu: make -cpu host support monitor/mwait”).
>> 
>> However, exposing MONITOR/MWAIT to a Linux guest may cause it's intel_idle
>                                                             ^^^^
>                                                             its
> 
> English is a wonderful language…

Yeah OK… :\

> 
>> kernel module to execute c1e_promotion_disable() which will attempt to
>> RDMSR/WRMSR from/to MSR_IA32_POWER_CTL to manipulate the "C1E Enable"
>> bit. This behaviour was introduced by commit
>> 32e9518005c8 ("intel_idle: export both C1 and C1E”).
> 
> Technically, I think this is a Qemu bug.  KVM reports all zeros for
> CPUID_MWAIT_LEAF when userspace queries KVM_GET_SUPPORTED_CPUID and
> KVM_GET_EMULATED_CPUID.  And I think that's correct/desired, supporting
> MONITOR/MWAIT sub-features should be a separate enabling patch set.

At some point in time Jim added commit df9cb9cc5bcd ("kvm: x86: Amend the KVM_GET_SUPPORTED_CPUID API documentation”)
which added the following paragraph to documentation:
"Note that certain capabilities, such as KVM_CAP_X86_DISABLE_EXITS, may
expose cpuid features (e.g. MONITOR) which are not supported by kvm in
its default configuration. If userspace enables such capabilities, it
is responsible for modifying the results of this ioctl appropriately.”

It’s indeed not clear what it means to “modify the results of this ioctl *appropriately*” right?
It can either mean you just expose in CPUID[EAX=1].ECX support for MONITOR/MWAIT
or that you also expose CPUID_MWAIT_LEAF (CPUID[EAX=5]).
Both regardless of the value returned from KVM_GET_SUPPORTED_CPUID ioctl.

Having said that, I tend to agree with you.
Instead of emulating this MSR in KVM, I think it it preferred to change QEMU to expose MONITOR/MWAIT support in CPUID[EAX=1].ECX
but in CPUID[EAX=5] init everything as in host besides ECX[0] which will be set to 0 to report we don’t support extensions.
(We still want to support range of monitor line size, whether we can treat interrupts as break-events for MWAIT and the supported C substates).
I will create this patch for QEMU.

But in addition, this MSR emulation I think is still required. As guests may still legitimately use this MSR to enable C1E
even when MWAIT extensions is not supported. This is because just executing HLT/MWAIT on all cores in package allows
auto transition to C1E.

So to conclude I think this KVM patch is still required and that we need a QEMU patch in addition.
Do you agree?

> 
> Note, there is a virtualization hole regarding MWAIT as KVM can't
> intercept MWAIT when executed with unsupported hints/features, but
> I don't think that absolves Qemu of wrongdoing.

I think that’s OK as this will just cause the logical processor to ignore MWAIT extension register
Specifying Sub C-state and target C-state. The reason this is OK is because in SDM on MWAIT it says:
"Implementation-specific conditions may cause a processor to ignore the hint and enter a different optimized state”.
So our implementation-specific condition is to always ignore the hint & extension :D

> 
>> Becuase KVM doesn't emulate this MSR, running KVM with ignore_msrs=0
>> will cause the above guest behaviour to raise a #GP which will cause
>> guest to kernel panic.
>> 
>> Therefore, add support for nop emulation of MSR_IA32_POWER_CTL to
>> avoid #GP in guest in this scenario.
>> 
>> Future commits can optimise emulation further by reflecting guest
>> MSR changes to host MSR to provide guest with the ability to
>> fine-tune the dedicated logical processor power-state.
>> 
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>> arch/x86/kvm/vmx/vmx.c | 6 ++++++
>> arch/x86/kvm/vmx/vmx.h | 2 ++
>> arch/x86/kvm/x86.c     | 1 +
>> 3 files changed, 9 insertions(+)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 2634ee8c9dc8..6246d782b746 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -1696,6 +1696,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> 	case MSR_IA32_SYSENTER_ESP:
>> 		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
>> 		break;
>> +	case MSR_IA32_POWER_CTL:
>> +		msr_info->data = vmx->msr_ia32_power_ctl;
>> +		break;
>> 	case MSR_IA32_BNDCFGS:
>> 		if (!kvm_mpx_supported() ||
>> 		    (!msr_info->host_initiated &&
>> @@ -1826,6 +1829,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>> 	case MSR_IA32_SYSENTER_ESP:
>> 		vmcs_writel(GUEST_SYSENTER_ESP, data);
>> 		break;
>> +	case MSR_IA32_POWER_CTL:
>> +		vmx->msr_ia32_power_ctl = data;
>> +		break;
>> 	case MSR_IA32_BNDCFGS:
>> 		if (!kvm_mpx_supported() ||
>> 		    (!msr_info->host_initiated &&
> 
> If KVM does go the route of advertising MWAIT/MONITOR sub-features, then I
> think the MSR needs to be emulated on both Intel and AMD.  Glancing through
> drivers/idle/intel_idle.c, I don't see anything that prevents it from
> successfully probing an "Intel" vCPU that is being emulated on AMD hardware.

Do we even support exposing “Intel” vCPU which is emulated using AMD SVM?
In that case, I agree and I will just move code to kvm_set_msr_common() & kvm_get_msr_common().

Thanks for the insightful review,
-Liran

> 
>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> index 99328954c2fc..e9772850a2a1 100644
>> --- a/arch/x86/kvm/vmx/vmx.h
>> +++ b/arch/x86/kvm/vmx/vmx.h
>> @@ -259,6 +259,8 @@ struct vcpu_vmx {
>> 
>> 	unsigned long host_debugctlmsr;
>> 
>> +	u64 msr_ia32_power_ctl;
>> +
>> 	/*
>> 	 * Only bits masked by msr_ia32_feature_control_valid_bits can be set in
>> 	 * msr_ia32_feature_control. FEATURE_CONTROL_LOCKED is always included
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 02c8e095a239..39ee4087f954 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1167,6 +1167,7 @@ static u32 emulated_msrs[] = {
>> 	MSR_PLATFORM_INFO,
>> 	MSR_MISC_FEATURES_ENABLES,
>> 	MSR_AMD64_VIRT_SPEC_CTRL,
>> +	MSR_IA32_POWER_CTL,
>> };
>> 
>> static unsigned num_emulated_msrs;
>> -- 
>> 2.20.1
>> 

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Paolo Bonzini]

On 16/04/19 17:21, Liran Alon wrote:
> 
> But in addition, this MSR emulation I think is still required. As
> guests may still legitimately use this MSR to enable C1E even when
> MWAIT extensions is not supported. This is because just executing
> HLT/MWAIT on all cores in package allows auto transition to C1E.
> 
> So to conclude I think this KVM patch is still required and that we
> need a QEMU patch in addition. Do you agree?
> 

Yes, I agree and I have queued the patch.

Paolo

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Liran Alon]

> On 16 Apr 2019, at 18:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 16/04/19 17:21, Liran Alon wrote:
>> 
>> But in addition, this MSR emulation I think is still required. As
>> guests may still legitimately use this MSR to enable C1E even when
>> MWAIT extensions is not supported. This is because just executing
>> HLT/MWAIT on all cores in package allows auto transition to C1E.
>> 
>> So to conclude I think this KVM patch is still required and that we
>> need a QEMU patch in addition. Do you agree?
>> 
> 
> Yes, I agree and I have queued the patch.
> 
> Paolo

Thanks Paolo! :)
Have you also moved the MSR emulation to the more generic x86 kvm_set_msr_common() & kvm_get_msr_common() when queueing?
(To address Sean concern that one could emulate an Intel vCPU on top of AMD SVM…)

-Liran

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Liran Alon]

> On 16 Apr 2019, at 18:21, Liran Alon <liran.alon@oracle.com> wrote:
> 
> 
> 
>> On 15 Apr 2019, at 21:17, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>> 
>> On Mon, Apr 15, 2019 at 06:45:26PM +0300, Liran Alon wrote:
>> 
>> Technically, I think this is a Qemu bug.  KVM reports all zeros for
>> CPUID_MWAIT_LEAF when userspace queries KVM_GET_SUPPORTED_CPUID and
>> KVM_GET_EMULATED_CPUID.  And I think that's correct/desired, supporting
>> MONITOR/MWAIT sub-features should be a separate enabling patch set.
> 
> At some point in time Jim added commit df9cb9cc5bcd ("kvm: x86: Amend the KVM_GET_SUPPORTED_CPUID API documentation”)
> which added the following paragraph to documentation:
> "Note that certain capabilities, such as KVM_CAP_X86_DISABLE_EXITS, may
> expose cpuid features (e.g. MONITOR) which are not supported by kvm in
> its default configuration. If userspace enables such capabilities, it
> is responsible for modifying the results of this ioctl appropriately.”
> 
> It’s indeed not clear what it means to “modify the results of this ioctl *appropriately*” right?
> It can either mean you just expose in CPUID[EAX=1].ECX support for MONITOR/MWAIT
> or that you also expose CPUID_MWAIT_LEAF (CPUID[EAX=5]).
> Both regardless of the value returned from KVM_GET_SUPPORTED_CPUID ioctl.
> 
> Having said that, I tend to agree with you.
> Instead of emulating this MSR in KVM, I think it it preferred to change QEMU to expose MONITOR/MWAIT support in CPUID[EAX=1].ECX
> but in CPUID[EAX=5] init everything as in host besides ECX[0] which will be set to 0 to report we don’t support extensions.
> (We still want to support range of monitor line size, whether we can treat interrupts as break-events for MWAIT and the supported C substates).
> I will create this patch for QEMU.

Actually on second thought, I will just remain with the KVM patch (that Paolo was nice enough to already queue).
and not do this QEMU patch. This is because why will we want to prevent guest from specifying target C-State if he is exposed with MWAIT?
I don’t see a reason we should prevent that. Do you?

The only thing which does looks a bit weird to me is that it seems QEMU always expose extensions support even if host don’t support it.
That I don’t quite understand why. Maybe Paolo knows.

-Liran

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Joao Martins]

On 4/16/19 4:40 PM, Liran Alon wrote:
>> On 16 Apr 2019, at 18:21, Liran Alon <liran.alon@oracle.com> wrote:
>>> On 15 Apr 2019, at 21:17, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>> On Mon, Apr 15, 2019 at 06:45:26PM +0300, Liran Alon wrote:
>>>
>>> Technically, I think this is a Qemu bug.  KVM reports all zeros for
>>> CPUID_MWAIT_LEAF when userspace queries KVM_GET_SUPPORTED_CPUID and
>>> KVM_GET_EMULATED_CPUID.  And I think that's correct/desired, supporting
>>> MONITOR/MWAIT sub-features should be a separate enabling patch set.
>>
>> At some point in time Jim added commit df9cb9cc5bcd ("kvm: x86: Amend the KVM_GET_SUPPORTED_CPUID API documentation”)
>> which added the following paragraph to documentation:
>> "Note that certain capabilities, such as KVM_CAP_X86_DISABLE_EXITS, may
>> expose cpuid features (e.g. MONITOR) which are not supported by kvm in
>> its default configuration. If userspace enables such capabilities, it
>> is responsible for modifying the results of this ioctl appropriately.”
>>
>> It’s indeed not clear what it means to “modify the results of this ioctl *appropriately*” right?
>> It can either mean you just expose in CPUID[EAX=1].ECX support for MONITOR/MWAIT
>> or that you also expose CPUID_MWAIT_LEAF (CPUID[EAX=5]).
>> Both regardless of the value returned from KVM_GET_SUPPORTED_CPUID ioctl.
>>
>> Having said that, I tend to agree with you.
>> Instead of emulating this MSR in KVM, I think it it preferred to change QEMU to expose MONITOR/MWAIT support in CPUID[EAX=1].ECX
>> but in CPUID[EAX=5] init everything as in host besides ECX[0] which will be set to 0 to report we don’t support extensions.
>> (We still want to support range of monitor line size, whether we can treat interrupts as break-events for MWAIT and the supported C substates).
>> I will create this patch for QEMU.
> 
> Actually on second thought, I will just remain with the KVM patch (that Paolo was nice enough to already queue).
> and not do this QEMU patch. This is because why will we want to prevent guest from specifying target C-State if he is exposed with MWAIT?
> I don’t see a reason we should prevent that. Do you?
> 
One reason it is a good idea to prevent the guest from entering deeper
C-states (e.g. deeper than C2) is to allow preemption timer to be used again
when guests are exposed with MWAIT (currently we can't do that).

Not sure if this would be towards not exposing MWAIT leaf (CPUID[EAX=5]) at all.
But at least it's one other reason I see about limiting MWAIT exposed
featureset. Userspace would zero out CPUID[EAX=5].EDX or mask the exposed host
EDX value to the first 2 C-states of host values. Even if we zero out
CPUID[EAX=5].EDX (and consequently not intel_idle does get used) guest can still
use mwait (but is limited to C1 IIUC).

See the KVM patch which handles that just below the scissors mark. But I am
waiting on a specific workload test (other than my own tests) before I formally
submit this (and by this I mean its counterpart on tests and qemu).

	Joao

------- >8 --------

From 29d69cd1e6d0f4e301dda53a1cef1b2c68d4a2d7 Mon Sep 17 00:00:00 2001
From: Joao Martins <joao.m.martins@oracle.com>
Date: Thu, Feb 21 12:48:07 2019 -0500
Subject: [PATCH] KVM: VMX: use preemption timer for C-states <= C2

According to section "25.5.1 VMX-Preemption Timer" in Volume 3C of Intel
SDM, the VMX preemption timer stops counting on C-states deeper than C2.

When mwait is exposed to the guest and MWAIT/MONITOR intercepts
are disabled, and only if the advertised MWAIT substates are
limited up to C2 (or zeroed out), we use preemption timer.

A guest which happens to ignore CPUID is going against spec,
so they can only shot themselves in the foot.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 arch/x86/kvm/vmx/vmx.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/vmx/vmx.h |  3 +++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 3fe2020e3bc4..153957bdfa73 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -45,6 +45,7 @@
 #include <asm/mmu_context.h>
 #include <asm/mshyperv.h>
 #include <asm/spec-ctrl.h>
+#include <asm/mwait.h>
 #include <asm/virtext.h>
 #include <asm/vmx.h>

@@ -6962,6 +6963,16 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
 		vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4));
 }

+static u32 vmx_get_mwait_substates(struct kvm_vcpu *vcpu)
+{
+	u32 eax, ebx, ecx, mwait_substates;
+
+	eax = CPUID_MWAIT_LEAF;
+	if (kvm_cpuid(vcpu, &eax, &ebx, &ecx, &mwait_substates, false))
+		return mwait_substates;
+	return 0;
+}
+
 static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -6987,6 +6998,36 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
 			guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT))
 		update_intel_pt_cfg(vcpu);
+
+	if (kvm_mwait_in_guest(vcpu->kvm))
+		to_vmx(vcpu)->mwait_substates = vmx_get_mwait_substates(vcpu);
+}
+
+static bool vmx_mwait_hv_timer_reliable(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * Per Intel SDM:
+	 *
+	 * Bits 03-00: Number of C0* sub C-states supported using MWAIT
+	 * Bits 07-04: Number of C1* sub C-states supported using MWAIT
+	 * Bits 11-08: Number of C2* sub C-states supported using MWAIT
+	 * Bits 15-12: Number of C3* sub C-states supported using MWAIT
+	 * Bits 19-16: Number of C4* sub C-states supported using MWAIT
+	 * Bits 23-20: Number of C5* sub C-states supported using MWAIT
+	 * Bits 27-24: Number of C6* sub C-states supported using MWAIT
+	 * Bits 31-28: Number of C7* sub C-states supported using MWAIT
+	 *
+	 * NOTE:
+	 * The definition of C0 through C7 states for MWAIT extension are
+	 * processor-specific C-states, not ACPI C-states.
+	 *
+	 * The VMX preemption timer stops counting in C-states higher than C2.
+	 * Most processors define MWAIT 0x0/0x1 as C1/C1E. MWAIT 0x10
+	 * corresponds to C3 except that on Atom processors which is C2.
+	 * So deem the first two as safe and allow preemption timer to be used.
+	 */
+#define VMX_MWAIT_CSTATES_UNSAFE 0xffffff00
+	return !(to_vmx(vcpu)->mwait_substates & VMX_MWAIT_CSTATES_UNSAFE);
 }

 static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
@@ -7046,7 +7087,7 @@ static int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64
guest_deadline_tsc)
 	struct vcpu_vmx *vmx;
 	u64 tscl, guest_tscl, delta_tsc, lapic_timer_advance_cycles;

-	if (kvm_mwait_in_guest(vcpu->kvm))
+	if (kvm_mwait_in_guest(vcpu->kvm) && !vmx_mwait_hv_timer_reliable(vcpu))
 		return -EOPNOTSUPP;

 	vmx = to_vmx(vcpu);
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 1e42f983e0f1..6024ae52f90c 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -257,6 +257,9 @@ struct vcpu_vmx {

 	unsigned long host_debugctlmsr;

+	/* maximum mwait sub-states */
+	u32 mwait_substates;
+
 	u64 msr_ia32_power_ctl;

 	/*
-- 
2.11.0

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Wanpeng Li]

Hi all,
On Wed, 17 Apr 2019 at 03:18, Joao Martins <joao.m.martins@oracle.com> wrote:
>
> On 4/16/19 4:40 PM, Liran Alon wrote:
> >> On 16 Apr 2019, at 18:21, Liran Alon <liran.alon@oracle.com> wrote:
> >>> On 15 Apr 2019, at 21:17, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >>> On Mon, Apr 15, 2019 at 06:45:26PM +0300, Liran Alon wrote:
> >>>
> >>> Technically, I think this is a Qemu bug.  KVM reports all zeros for
> >>> CPUID_MWAIT_LEAF when userspace queries KVM_GET_SUPPORTED_CPUID and
> >>> KVM_GET_EMULATED_CPUID.  And I think that's correct/desired, supporting
> >>> MONITOR/MWAIT sub-features should be a separate enabling patch set.
> >>
> >> At some point in time Jim added commit df9cb9cc5bcd ("kvm: x86: Amend the KVM_GET_SUPPORTED_CPUID API documentation”)
> >> which added the following paragraph to documentation:
> >> "Note that certain capabilities, such as KVM_CAP_X86_DISABLE_EXITS, may
> >> expose cpuid features (e.g. MONITOR) which are not supported by kvm in
> >> its default configuration. If userspace enables such capabilities, it
> >> is responsible for modifying the results of this ioctl appropriately.”
> >>
> >> It’s indeed not clear what it means to “modify the results of this ioctl *appropriately*” right?
> >> It can either mean you just expose in CPUID[EAX=1].ECX support for MONITOR/MWAIT
> >> or that you also expose CPUID_MWAIT_LEAF (CPUID[EAX=5]).
> >> Both regardless of the value returned from KVM_GET_SUPPORTED_CPUID ioctl.
> >>
> >> Having said that, I tend to agree with you.
> >> Instead of emulating this MSR in KVM, I think it it preferred to change QEMU to expose MONITOR/MWAIT support in CPUID[EAX=1].ECX
> >> but in CPUID[EAX=5] init everything as in host besides ECX[0] which will be set to 0 to report we don’t support extensions.
> >> (We still want to support range of monitor line size, whether we can treat interrupts as break-events for MWAIT and the supported C substates).
> >> I will create this patch for QEMU.
> >
> > Actually on second thought, I will just remain with the KVM patch (that Paolo was nice enough to already queue).
> > and not do this QEMU patch. This is because why will we want to prevent guest from specifying target C-State if he is exposed with MWAIT?
> > I don’t see a reason we should prevent that. Do you?
> >
> One reason it is a good idea to prevent the guest from entering deeper
> C-states (e.g. deeper than C2) is to allow preemption timer to be used again
> when guests are exposed with MWAIT (currently we can't do that).

It is weird that we can observe intel_idle driver in the guest
executes mwait eax=0x20, and the corresponding pCPU enters C3 on HSW
server, however, we can't observe this on SKX/CLX server, it just
enters maximal C1. All the setups between HSW and SKX/CLX are the
same. Sean and Liran, any opinions?

Regards,
Wanpeng Li

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Joao Martins]

On 5/10/19 10:54 AM, Wanpeng Li wrote:
> Hi all,
> On Wed, 17 Apr 2019 at 03:18, Joao Martins <joao.m.martins@oracle.com> wrote:
>>
>> On 4/16/19 4:40 PM, Liran Alon wrote:
>>>> On 16 Apr 2019, at 18:21, Liran Alon <liran.alon@oracle.com> wrote:
>>>>> On 15 Apr 2019, at 21:17, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>>>> On Mon, Apr 15, 2019 at 06:45:26PM +0300, Liran Alon wrote:
>>>>>
>>>>> Technically, I think this is a Qemu bug.  KVM reports all zeros for
>>>>> CPUID_MWAIT_LEAF when userspace queries KVM_GET_SUPPORTED_CPUID and
>>>>> KVM_GET_EMULATED_CPUID.  And I think that's correct/desired, supporting
>>>>> MONITOR/MWAIT sub-features should be a separate enabling patch set.
>>>>
>>>> At some point in time Jim added commit df9cb9cc5bcd ("kvm: x86: Amend the KVM_GET_SUPPORTED_CPUID API documentation”)
>>>> which added the following paragraph to documentation:
>>>> "Note that certain capabilities, such as KVM_CAP_X86_DISABLE_EXITS, may
>>>> expose cpuid features (e.g. MONITOR) which are not supported by kvm in
>>>> its default configuration. If userspace enables such capabilities, it
>>>> is responsible for modifying the results of this ioctl appropriately.”
>>>>
>>>> It’s indeed not clear what it means to “modify the results of this ioctl *appropriately*” right?
>>>> It can either mean you just expose in CPUID[EAX=1].ECX support for MONITOR/MWAIT
>>>> or that you also expose CPUID_MWAIT_LEAF (CPUID[EAX=5]).
>>>> Both regardless of the value returned from KVM_GET_SUPPORTED_CPUID ioctl.
>>>>
>>>> Having said that, I tend to agree with you.
>>>> Instead of emulating this MSR in KVM, I think it it preferred to change QEMU to expose MONITOR/MWAIT support in CPUID[EAX=1].ECX
>>>> but in CPUID[EAX=5] init everything as in host besides ECX[0] which will be set to 0 to report we don’t support extensions.
>>>> (We still want to support range of monitor line size, whether we can treat interrupts as break-events for MWAIT and the supported C substates).
>>>> I will create this patch for QEMU.
>>>
>>> Actually on second thought, I will just remain with the KVM patch (that Paolo was nice enough to already queue).
>>> and not do this QEMU patch. This is because why will we want to prevent guest from specifying target C-State if he is exposed with MWAIT?
>>> I don’t see a reason we should prevent that. Do you?
>>>
>> One reason it is a good idea to prevent the guest from entering deeper
>> C-states (e.g. deeper than C2) is to allow preemption timer to be used again
>> when guests are exposed with MWAIT (currently we can't do that).
> 
> It is weird that we can observe intel_idle driver in the guest
> executes mwait eax=0x20, and the corresponding pCPU enters C3 on HSW
> server, however, we can't observe this on SKX/CLX server, it just
> enters maximal C1. 

I assume you refer to the case where you pass the host mwait substates to the
guests as is, right? Or are you zeroing/filtering out the mwait cpuid leaf EDX
like my patch (attached in the previous message) suggests?

Interestingly, hints set to 0x20 actually corresponds to C6 on HSW (based on
intel_idle driver). IIUC From the SDM (see Vol 2B, "MWAIT for Power Management"
in instruction set reference M-U) the hints register, doesn't necessarily
guarantee the specified C-state depicted in the hints will be used. The manual
makes it sound like it is tentative, and implementation-specific condition may
either ignore it or enter a different one. It appears to be only guaranteed that
it won't enter a C-{sub,}state deeper than the one depicted.

> All the setups between HSW and SKX/CLX are the
> same. Sean and Liran, any opinions?
> 
> Regards,
> Wanpeng Li
> 

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Sean Christopherson]

On Fri, May 10, 2019 at 11:34:41AM +0100, Joao Martins wrote:
> On 5/10/19 10:54 AM, Wanpeng Li wrote:
> > It is weird that we can observe intel_idle driver in the guest
> > executes mwait eax=0x20, and the corresponding pCPU enters C3 on HSW
> > server, however, we can't observe this on SKX/CLX server, it just
> > enters maximal C1. 
> 
> I assume you refer to the case where you pass the host mwait substates to the
> guests as is, right? Or are you zeroing/filtering out the mwait cpuid leaf EDX
> like my patch (attached in the previous message) suggests?
> 
> Interestingly, hints set to 0x20 actually corresponds to C6 on HSW (based on
> intel_idle driver). IIUC From the SDM (see Vol 2B, "MWAIT for Power Management"
> in instruction set reference M-U) the hints register, doesn't necessarily
> guarantee the specified C-state depicted in the hints will be used. The manual
> makes it sound like it is tentative, and implementation-specific condition may
> either ignore it or enter a different one. It appears to be only guaranteed that
> it won't enter a C-{sub,}state deeper than the one depicted.

Yep, section "MWAIT EXTENSIONS FOR ADVANCED POWER MANAGEMENT" is more
explicit on this point:

  At CPL=0, system software can specify desired C-state and sub C-state by
  using the MWAIT hints register (EAX).  Processors will not go to C-state
  and sub C-state deeper than what is specified by the hint register.

As for why SKX/CLX only enters C1, AFAICT SKX isn't configured to support
C3, e.g. skx_cstates in drivers/idle/intel_idle.c shows C1, C1E and C6.
A quick search brings up a variety of docs that confirm this.  My guess is
that C1E provides better power/performance than C3 for the majority of
server workloads, e.g. C3 doesn't provide enough power savings to justify
its higher latency and TLB flush.

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Wanpeng Li]

On Sat, 11 May 2019 at 01:17, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, May 10, 2019 at 11:34:41AM +0100, Joao Martins wrote:
> > On 5/10/19 10:54 AM, Wanpeng Li wrote:
> > > It is weird that we can observe intel_idle driver in the guest
> > > executes mwait eax=0x20, and the corresponding pCPU enters C3 on HSW
> > > server, however, we can't observe this on SKX/CLX server, it just
> > > enters maximal C1.
> >
> > I assume you refer to the case where you pass the host mwait substates to the
> > guests as is, right? Or are you zeroing/filtering out the mwait cpuid leaf EDX
> > like my patch (attached in the previous message) suggests?
> >
> > Interestingly, hints set to 0x20 actually corresponds to C6 on HSW (based on
> > intel_idle driver). IIUC From the SDM (see Vol 2B, "MWAIT for Power Management"
> > in instruction set reference M-U) the hints register, doesn't necessarily
> > guarantee the specified C-state depicted in the hints will be used. The manual
> > makes it sound like it is tentative, and implementation-specific condition may
> > either ignore it or enter a different one. It appears to be only guaranteed that
> > it won't enter a C-{sub,}state deeper than the one depicted.
>
> Yep, section "MWAIT EXTENSIONS FOR ADVANCED POWER MANAGEMENT" is more
> explicit on this point:
>
>   At CPL=0, system software can specify desired C-state and sub C-state by
>   using the MWAIT hints register (EAX).  Processors will not go to C-state
>   and sub C-state deeper than what is specified by the hint register.
>
> As for why SKX/CLX only enters C1, AFAICT SKX isn't configured to support
> C3, e.g. skx_cstates in drivers/idle/intel_idle.c shows C1, C1E and C6.
> A quick search brings up a variety of docs that confirm this.  My guess is
> that C1E provides better power/performance than C3 for the majority of
> server workloads, e.g. C3 doesn't provide enough power savings to justify
> its higher latency and TLB flush.

You are right, I figure this out by referring to the SKX/CLX EDS, the
Core C-States of these two generations just support CC0/CC1/CC1E/CC6.
The issue here is after exposing mwait to the guest, SKX/CLX guest
can't enter CC6, however, HSW guest can enter CC3/CC6. Both HSW and
SKX/CLX hosts can enter CC6. We observe SKX/CLX guests execute mwait
eax 0x20, however, we can't observe the corresponding pCPU enter CC6
by turbostat or reading MSR_CORE_C6_RESIDENCY directly.

Regards,
Wanpeng Li

Re: [PATCH] KVM: VMX: Nop emulation of MSR_IA32_POWER_CTL

[Sean Christopherson]

On Mon, May 13, 2019 at 05:13:29PM +0800, Wanpeng Li wrote:
> On Sat, 11 May 2019 at 01:17, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Fri, May 10, 2019 at 11:34:41AM +0100, Joao Martins wrote:
> > > On 5/10/19 10:54 AM, Wanpeng Li wrote:
> > > > It is weird that we can observe intel_idle driver in the guest
> > > > executes mwait eax=0x20, and the corresponding pCPU enters C3 on HSW
> > > > server, however, we can't observe this on SKX/CLX server, it just
> > > > enters maximal C1.
> > >
> > > I assume you refer to the case where you pass the host mwait substates to the
> > > guests as is, right? Or are you zeroing/filtering out the mwait cpuid leaf EDX
> > > like my patch (attached in the previous message) suggests?
> > >
> > > Interestingly, hints set to 0x20 actually corresponds to C6 on HSW (based on
> > > intel_idle driver). IIUC From the SDM (see Vol 2B, "MWAIT for Power Management"
> > > in instruction set reference M-U) the hints register, doesn't necessarily
> > > guarantee the specified C-state depicted in the hints will be used. The manual
> > > makes it sound like it is tentative, and implementation-specific condition may
> > > either ignore it or enter a different one. It appears to be only guaranteed that
> > > it won't enter a C-{sub,}state deeper than the one depicted.
> >
> > Yep, section "MWAIT EXTENSIONS FOR ADVANCED POWER MANAGEMENT" is more
> > explicit on this point:
> >
> >   At CPL=0, system software can specify desired C-state and sub C-state by
> >   using the MWAIT hints register (EAX).  Processors will not go to C-state
> >   and sub C-state deeper than what is specified by the hint register.
> >
> > As for why SKX/CLX only enters C1, AFAICT SKX isn't configured to support
> > C3, e.g. skx_cstates in drivers/idle/intel_idle.c shows C1, C1E and C6.
> > A quick search brings up a variety of docs that confirm this.  My guess is
> > that C1E provides better power/performance than C3 for the majority of
> > server workloads, e.g. C3 doesn't provide enough power savings to justify
> > its higher latency and TLB flush.
> 
> You are right, I figure this out by referring to the SKX/CLX EDS, the
> Core C-States of these two generations just support CC0/CC1/CC1E/CC6.
> The issue here is after exposing mwait to the guest, SKX/CLX guest
> can't enter CC6, however, HSW guest can enter CC3/CC6. Both HSW and
> SKX/CLX hosts can enter CC6. We observe SKX/CLX guests execute mwait
> eax 0x20, however, we can't observe the corresponding pCPU enter CC6
> by turbostat or reading MSR_CORE_C6_RESIDENCY directly.

It's likely that the CPU is operating as expected and isn't dropping into
the deeper sleep state because of some heuristic or wake event.  It might
be something as simple as the combination of periodic tick interrupts
between host and guest occuring too frequently (to get to C6), or it could
be a much more complex scenario.  It's been several years since I've done
anything close to hands on debug with C-states, I have no idea what
capabilities are available to help debug this sort of thing.  Sorry I
can't be more helpful.