[PATCH 0/3] improve -overcommit cpu-pm=on|off

[PATCH 0/3] improve -overcommit cpu-pm=on|off

[Zide Chen]

Currently, if running "-overcommit cpu-pm=on" on hosts that don't
have MWAIT support, the MWAIT/MONITOR feature is advertised to the
guest and executing MWAIT/MONITOR on the guest triggers #UD.

Zide Chen (3):
  vl: Allow multiple -overcommit commands
  target/i386: call cpu_exec_realizefn before x86_cpu_filter_features
  target/i386: Move host_cpu_enable_cpu_pm into kvm_cpu_realizefn()

 system/vl.c               |  8 ++++++--
 target/i386/cpu.c         | 24 ++++++++++++------------
 target/i386/host-cpu.c    | 12 ------------
 target/i386/kvm/kvm-cpu.c | 12 +++++++++---
 4 files changed, 27 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH 1/3] vl: Allow multiple -overcommit commands

[Zide Chen]

Both cpu-pm and mem-lock are related to system resource overcommit, but
they are separate from each other, in terms of how they are realized,
and of course, they are applied to different system resources.

It's tempting to use separate command lines to specify their behavior.
e.g., in the following example, the cpu-pm command is quietly
overwritten, and it's not easy to notice it without careful inspection.

  --overcommit mem-lock=on
  --overcommit cpu-pm=on

Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
Signed-off-by: Zide Chen <zide.chen@intel.com>
---
 system/vl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5b8..ed682643805b 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
                 if (!opts) {
                     exit(1);
                 }
-                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
-                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
+
+                /* Don't override the -overcommit option if set */
+                enable_mlock = enable_mlock ||
+                    qemu_opt_get_bool(opts, "mem-lock", false);
+                enable_cpu_pm = enable_cpu_pm ||
+                    qemu_opt_get_bool(opts, "cpu-pm", false);
                 break;
             case QEMU_OPTION_compat:
                 {
-- 
2.34.1

Re: [PATCH 1/3] vl: Allow multiple -overcommit commands

[Thomas Huth]

On 20/05/2024 19.47, Zide Chen wrote:
> Both cpu-pm and mem-lock are related to system resource overcommit, but
> they are separate from each other, in terms of how they are realized,
> and of course, they are applied to different system resources.
> 
> It's tempting to use separate command lines to specify their behavior.
> e.g., in the following example, the cpu-pm command is quietly
> overwritten, and it's not easy to notice it without careful inspection.
> 
>    --overcommit mem-lock=on
>    --overcommit cpu-pm=on
> 
> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> ---
>   system/vl.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/system/vl.c b/system/vl.c
> index a3eede5fa5b8..ed682643805b 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>                   if (!opts) {
>                       exit(1);
>                   }
> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
> +
> +                /* Don't override the -overcommit option if set */
> +                enable_mlock = enable_mlock ||
> +                    qemu_opt_get_bool(opts, "mem-lock", false);
> +                enable_cpu_pm = enable_cpu_pm ||
> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>                   break;
>               case QEMU_OPTION_compat:
>                   {

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/3] vl: Allow multiple -overcommit commands

[Thomas Huth]

On 21/05/2024 07.08, Thomas Huth wrote:
> On 20/05/2024 19.47, Zide Chen wrote:
>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>> they are separate from each other, in terms of how they are realized,
>> and of course, they are applied to different system resources.
>>
>> It's tempting to use separate command lines to specify their behavior.
>> e.g., in the following example, the cpu-pm command is quietly
>> overwritten, and it's not easy to notice it without careful inspection.
>>
>>    --overcommit mem-lock=on
>>    --overcommit cpu-pm=on
>>
>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>> ---
>>   system/vl.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/system/vl.c b/system/vl.c
>> index a3eede5fa5b8..ed682643805b 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>                   if (!opts) {
>>                       exit(1);
>>                   }
>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
>> +
>> +                /* Don't override the -overcommit option if set */
>> +                enable_mlock = enable_mlock ||
>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>> +                enable_cpu_pm = enable_cpu_pm ||
>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>                   break;
>>               case QEMU_OPTION_compat:
>>                   {
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Ah, wait, actually, this is a bad idea, too, since now you cannot disable an 
enabled option anymore. Imagine that you have for example enabled the option 
in the config file, and now you'd like to disable it on the command line 
again - you're stuck with the enabled setting in that case.

I think the better solution is to replace the "false" default value at the end:

         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);

What do you think about this?

  Thomas

Re: [PATCH 1/3] vl: Allow multiple -overcommit commands

[Chen, Zide]

On 5/20/2024 10:16 PM, Thomas Huth wrote:
> On 21/05/2024 07.08, Thomas Huth wrote:
>> On 20/05/2024 19.47, Zide Chen wrote:
>>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>>> they are separate from each other, in terms of how they are realized,
>>> and of course, they are applied to different system resources.
>>>
>>> It's tempting to use separate command lines to specify their behavior.
>>> e.g., in the following example, the cpu-pm command is quietly
>>> overwritten, and it's not easy to notice it without careful inspection.
>>>
>>>    --overcommit mem-lock=on
>>>    --overcommit cpu-pm=on
>>>
>>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>>   system/vl.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index a3eede5fa5b8..ed682643805b 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>>                   if (!opts) {
>>>                       exit(1);
>>>                   }
>>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
>>> false);
>>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
>>> false);
>>> +
>>> +                /* Don't override the -overcommit option if set */
>>> +                enable_mlock = enable_mlock ||
>>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>>> +                enable_cpu_pm = enable_cpu_pm ||
>>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>>                   break;
>>>               case QEMU_OPTION_compat:
>>>                   {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Ah, wait, actually, this is a bad idea, too, since now you cannot
> disable an enabled option anymore. Imagine that you have for example
> enabled the option in the config file, and now you'd like to disable it
> on the command line again - you're stuck with the enabled setting in
> that case.
> 
> I think the better solution is to replace the "false" default value at
> the end:
> 
>         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
>         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
> 

> What do you think about this?

Thank you very much! This is a very good idea, and I can update it in V2.

> 
>  Thomas
> 

Re: [PATCH 1/3] vl: Allow multiple -overcommit commands

[Chen, Zide]

On 5/20/2024 10:16 PM, Thomas Huth wrote:
> On 21/05/2024 07.08, Thomas Huth wrote:
>> On 20/05/2024 19.47, Zide Chen wrote:
>>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>>> they are separate from each other, in terms of how they are realized,
>>> and of course, they are applied to different system resources.
>>>
>>> It's tempting to use separate command lines to specify their behavior.
>>> e.g., in the following example, the cpu-pm command is quietly
>>> overwritten, and it's not easy to notice it without careful inspection.
>>>
>>>    --overcommit mem-lock=on
>>>    --overcommit cpu-pm=on
>>>
>>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>>   system/vl.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index a3eede5fa5b8..ed682643805b 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>>                   if (!opts) {
>>>                       exit(1);
>>>                   }
>>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
>>> false);
>>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
>>> false);
>>> +
>>> +                /* Don't override the -overcommit option if set */
>>> +                enable_mlock = enable_mlock ||
>>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>>> +                enable_cpu_pm = enable_cpu_pm ||
>>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>>                   break;
>>>               case QEMU_OPTION_compat:
>>>                   {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Ah, wait, actually, this is a bad idea, too, since now you cannot
> disable an enabled option anymore. Imagine that you have for example
> enabled the option in the config file, and now you'd like to disable it
> on the command line again - you're stuck with the enabled setting in
> that case.
> 
> I think the better solution is to replace the "false" default value at
> the end:
> 
>         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
>         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
> 
> What do you think about this?

Thank you very much! This is a very good idea! I can update it in V2.

>  Thomas
> 

Re: [PATCH 1/3] vl: Allow multiple -overcommit commands

[Chen, Zide]

On 5/20/2024 10:16 PM, Thomas Huth wrote:
> On 21/05/2024 07.08, Thomas Huth wrote:
>> On 20/05/2024 19.47, Zide Chen wrote:
>>> Both cpu-pm and mem-lock are related to system resource overcommit, but
>>> they are separate from each other, in terms of how they are realized,
>>> and of course, they are applied to different system resources.
>>>
>>> It's tempting to use separate command lines to specify their behavior.
>>> e.g., in the following example, the cpu-pm command is quietly
>>> overwritten, and it's not easy to notice it without careful inspection.
>>>
>>>    --overcommit mem-lock=on
>>>    --overcommit cpu-pm=on
>>>
>>> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
>>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>>> ---
>>>   system/vl.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/system/vl.c b/system/vl.c
>>> index a3eede5fa5b8..ed682643805b 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -3545,8 +3545,12 @@ void qemu_init(int argc, char **argv)
>>>                   if (!opts) {
>>>                       exit(1);
>>>                   }
>>> -                enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
>>> false);
>>> -                enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
>>> false);
>>> +
>>> +                /* Don't override the -overcommit option if set */
>>> +                enable_mlock = enable_mlock ||
>>> +                    qemu_opt_get_bool(opts, "mem-lock", false);
>>> +                enable_cpu_pm = enable_cpu_pm ||
>>> +                    qemu_opt_get_bool(opts, "cpu-pm", false);
>>>                   break;
>>>               case QEMU_OPTION_compat:
>>>                   {
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Ah, wait, actually, this is a bad idea, too, since now you cannot
> disable an enabled option anymore. Imagine that you have for example
> enabled the option in the config file, and now you'd like to disable it
> on the command line again - you're stuck with the enabled setting in
> that case.
> 
> I think the better solution is to replace the "false" default value at
> the end:
> 
>         enable_mlock = qemu_opt_get_bool(opts, "mem-lock", enable_mlock);
>         enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", enable_cpu_pm);
> 
> What do you think about this?

This is great! Thank you very much! I will update it in V2.

> 
>  Thomas
> 

[PATCH 2/3] target/i386: call cpu_exec_realizefn before x86_cpu_filter_features

[Zide Chen]

cpu_exec_realizefn which calls the accel-specific realizefn may expand
features.  e.g., some accel-specific options may require extra features
to be enabled, and it's appropriate to expand these features in accel-
specific realizefn.

One such example is the cpu-pm option, which may add CPUID_EXT_MONITOR.

Thus, call cpu_exec_realizefn before x86_cpu_filter_features to ensure
that it won't expose features not supported by the host.

Fixes: 662175b91ff2 ("i386: reorder call to cpu_exec_realizefn")
Suggested-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/cpu.c         | 24 ++++++++++++------------
 target/i386/kvm/kvm-cpu.c |  1 -
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index cfe7c92d6bc6..da1ab7892d26 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7438,6 +7438,18 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    /*
+     * note: the call to the framework needs to happen after feature expansion,
+     * but before the checks/modifications to ucode_rev, mwait, phys_bits.
+     * These may be set by the accel-specific code,
+     * and the results are subsequently checked / assumed in this function.
+     */
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);
 
     if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
@@ -7459,18 +7471,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     x86_cpu_set_sgxlepubkeyhash(env);
 
-    /*
-     * note: the call to the framework needs to happen after feature expansion,
-     * but before the checks/modifications to ucode_rev, mwait, phys_bits.
-     * These may be set by the accel-specific code,
-     * and the results are subsequently checked / assumed in this function.
-     */
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
         g_autofree char *name = x86_cpu_class_get_model_name(xcc);
         error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index f76972e47e61..3adcedf0dbc3 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -50,7 +50,6 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      * nothing else has been set by the user (or by accelerators) in
      * cpu->ucode_rev and cpu->phys_bits, and updates the CPUID results in
      * mwait.ecx.
-     * This accel realization code also assumes cpu features are already expanded.
      *
      * realize order:
      *
-- 
2.34.1

[PATCH 3/3] target/i386: Move host_cpu_enable_cpu_pm into kvm_cpu_realizefn()

[Zide Chen]

It seems not a good idea to expand features in host_cpu_realizefn,
which is for host CPU only.  Additionally, cpu-pm option is KVM
specific, and it's cleaner to put it in kvm_cpu_realizefn(), together
with the WAITPKG code.

Fixes: f5cc5a5c1686 ("i386: split cpu accelerators from cpu.c, using AccelCPUClass")
Signed-off-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/host-cpu.c    | 12 ------------
 target/i386/kvm/kvm-cpu.c | 11 +++++++++--
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index 280e427c017c..8b8bf5afeccf 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -42,15 +42,6 @@ static uint32_t host_cpu_phys_bits(void)
     return host_phys_bits;
 }
 
-static void host_cpu_enable_cpu_pm(X86CPU *cpu)
-{
-    CPUX86State *env = &cpu->env;
-
-    host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
-               &cpu->mwait.ecx, &cpu->mwait.edx);
-    env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
-}
-
 static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
 {
     uint32_t host_phys_bits = host_cpu_phys_bits();
@@ -83,9 +74,6 @@ bool host_cpu_realizefn(CPUState *cs, Error **errp)
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
 
-    if (cpu->max_features && enable_cpu_pm) {
-        host_cpu_enable_cpu_pm(cpu);
-    }
     if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
         uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu);
 
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 3adcedf0dbc3..197c892da89a 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -64,9 +64,16 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      *   cpu_common_realizefn() (via xcc->parent_realize)
      */
     if (cpu->max_features) {
-        if (enable_cpu_pm && kvm_has_waitpkg()) {
-            env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
+        if (enable_cpu_pm) {
+            if (kvm_has_waitpkg()) {
+                env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
+            }
+
+            host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
+                       &cpu->mwait.ecx, &cpu->mwait.edx);
+            env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
         }
+
         if (cpu->ucode_rev == 0) {
             cpu->ucode_rev =
                 kvm_arch_get_supported_msr_feature(kvm_state,
-- 
2.34.1