[Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management

Michael S. Tsirkin posted 2 patches 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180612184616.90838-1-mst@redhat.com
Test checkpatch failed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
include/sysemu/sysemu.h |  1 +
target/i386/cpu.h       |  9 +++++++++
target/i386/cpu.c       | 19 ++++++++++++++-----
target/i386/kvm.c       | 30 ++++++++++++++++++++++++++++++
vl.c                    |  6 ++++++
qemu-options.hx         |  9 +++++++--
6 files changed, 67 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
This adds ability to expose host CPU power management capabilities to
guests. For intel guests, this is sufficient for guest to enable
low power CPU power management. For AMD guests it isn't sufficient,
deeper C-states are entered using System-IO.

mwait based power management is tied closely to specifics of CPUID,
making migration challenging. At this point only the non-migrateable
-cpu host is supported.

With this patch applied, VM latency is within the noise of
baremetal for some benchmarks.

perf bench sched pipe results:
Before:
    6.452 sec
After:
    4.382 sec
Baremetal:
    4.136 sec

Michael S. Tsirkin (2):
  kvm: support -realtime cpu-pm=on|off
  i386/cpu: make -cpu host support monitor/mwait

 include/sysemu/sysemu.h |  1 +
 target/i386/cpu.h       |  9 +++++++++
 target/i386/cpu.c       | 19 ++++++++++++++-----
 target/i386/kvm.c       | 30 ++++++++++++++++++++++++++++++
 vl.c                    |  6 ++++++
 qemu-options.hx         |  9 +++++++--
 6 files changed, 67 insertions(+), 7 deletions(-)

-- 
MST


Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by no-reply@patchew.org 5 years, 9 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180612184616.90838-1-mst@redhat.com
Subject: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20180612184616.90838-1-mst@redhat.com -> patchew/20180612184616.90838-1-mst@redhat.com
Switched to a new branch 'test'
b92893898a i386/cpu: make -cpu host support monitor/mwait
26a0f862a2 kvm: support -realtime cpu-pm=on|off

=== OUTPUT BEGIN ===
Checking PATCH 1/2: kvm: support -realtime cpu-pm=on|off...
WARNING: line over 80 characters
#85: FILE: target/i386/kvm.c:1401:
+            error_report("kvm: guest stopping CPU not supported: %s", strerror(-ret));

ERROR: do not initialise globals to 0 or NULL
#100: FILE: vl.c:145:
+bool enable_cpu_pm = false;

total: 1 errors, 1 warnings, 82 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/2: i386/cpu: make -cpu host support monitor/mwait...
WARNING: line over 80 characters
#82: FILE: target/i386/kvm.c:371:
+            int disable_exits = kvm_check_extension(s, KVM_CAP_X86_DISABLE_EXITS);

total: 0 errors, 1 warnings, 60 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
On Tue, Jun 12, 2018 at 12:18:03PM -0700, no-reply@patchew.org wrote:
> Checking PATCH 1/2: kvm: support -realtime cpu-pm=on|off...
> WARNING: line over 80 characters
> #85: FILE: target/i386/kvm.c:1401:
> +            error_report("kvm: guest stopping CPU not supported: %s", strerror(-ret));
> 
> ERROR: do not initialise globals to 0 or NULL
> #100: FILE: vl.c:145:
> +bool enable_cpu_pm = false;
> 
> total: 1 errors, 1 warnings, 82 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.

We seem to violate this left and right. Let's drop this?

-- 
MST

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Marcelo Tosatti 5 years, 9 months ago
On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote:
> This adds ability to expose host CPU power management capabilities to
> guests. For intel guests, this is sufficient for guest to enable
> low power CPU power management. For AMD guests it isn't sufficient,
> deeper C-states are entered using System-IO.
> 
> mwait based power management is tied closely to specifics of CPUID,
> making migration challenging. At this point only the non-migrateable
> -cpu host is supported.
> 
> With this patch applied, VM latency is within the noise of
> baremetal for some benchmarks.
> 
> perf bench sched pipe results:
> Before:
>     6.452 sec
> After:
>     4.382 sec
> Baremetal:
>     4.136 sec
> 
> Michael S. Tsirkin (2):
>   kvm: support -realtime cpu-pm=on|off
>   i386/cpu: make -cpu host support monitor/mwait
> 
>  include/sysemu/sysemu.h |  1 +
>  target/i386/cpu.h       |  9 +++++++++
>  target/i386/cpu.c       | 19 ++++++++++++++-----
>  target/i386/kvm.c       | 30 ++++++++++++++++++++++++++++++
>  vl.c                    |  6 ++++++
>  qemu-options.hx         |  9 +++++++--
>  6 files changed, 67 insertions(+), 7 deletions(-)
> 
> -- 
> MST

Hi Michael,

1) Command line option interface

Why is this not an optional cpu feature such as the other features? 


-cpu CPU,+mwait  

rather than a separate, architecture independent "-realtime cpu-pm=on|off" 
command line option?

2) Migration

Isnt it sufficient to check that both CPUID leafs are the same, 
to allow migration ?

1. Check that the processor supports MONITOR and MWAIT. If
CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at
ring 0.

2. Query the smallest and largest line size that MONITOR uses.
Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. 



Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
On Wed, Jun 13, 2018 at 07:34:53PM -0300, Marcelo Tosatti wrote:
> On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote:
> > This adds ability to expose host CPU power management capabilities to
> > guests. For intel guests, this is sufficient for guest to enable
> > low power CPU power management. For AMD guests it isn't sufficient,
> > deeper C-states are entered using System-IO.
> > 
> > mwait based power management is tied closely to specifics of CPUID,
> > making migration challenging. At this point only the non-migrateable
> > -cpu host is supported.
> > 
> > With this patch applied, VM latency is within the noise of
> > baremetal for some benchmarks.
> > 
> > perf bench sched pipe results:
> > Before:
> >     6.452 sec
> > After:
> >     4.382 sec
> > Baremetal:
> >     4.136 sec
> > 
> > Michael S. Tsirkin (2):
> >   kvm: support -realtime cpu-pm=on|off
> >   i386/cpu: make -cpu host support monitor/mwait
> > 
> >  include/sysemu/sysemu.h |  1 +
> >  target/i386/cpu.h       |  9 +++++++++
> >  target/i386/cpu.c       | 19 ++++++++++++++-----
> >  target/i386/kvm.c       | 30 ++++++++++++++++++++++++++++++
> >  vl.c                    |  6 ++++++
> >  qemu-options.hx         |  9 +++++++--
> >  6 files changed, 67 insertions(+), 7 deletions(-)
> > 
> > -- 
> > MST
> 
> Hi Michael,
> 
> 1) Command line option interface
> 
> Why is this not an optional cpu feature such as the other features? 
> 
> 
> -cpu CPU,+mwait  
> 
> rather than a separate, architecture independent "-realtime cpu-pm=on|off" 
> command line option?

Because it's not just a guest flag. With guest pm on, one guest
can severely affect the latency of others on the same host CPU.

> 2) Migration
> 
> Isnt it sufficient to check that both CPUID leafs are the same, 
> to allow migration ?

Not at the moment since linux guests use mwait hints and latency values
from a table in intel_idle.  If the host and guest models do not match,
mwait will get a wrong hint.

It will not do the right thing then!

You want exactly the same host CPU for it to work.

This isn't different from how -host cpu works generally.

> 1. Check that the processor supports MONITOR and MWAIT. If
> CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at
> ring 0.
> 
> 2. Query the smallest and largest line size that MONITOR uses.
> Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. 
> 

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Marcelo Tosatti 5 years, 9 months ago
On Thu, Jun 14, 2018 at 02:37:28AM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 13, 2018 at 07:34:53PM -0300, Marcelo Tosatti wrote:
> > On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote:
> > > This adds ability to expose host CPU power management capabilities to
> > > guests. For intel guests, this is sufficient for guest to enable
> > > low power CPU power management. For AMD guests it isn't sufficient,
> > > deeper C-states are entered using System-IO.
> > > 
> > > mwait based power management is tied closely to specifics of CPUID,
> > > making migration challenging. At this point only the non-migrateable
> > > -cpu host is supported.
> > > 
> > > With this patch applied, VM latency is within the noise of
> > > baremetal for some benchmarks.
> > > 
> > > perf bench sched pipe results:
> > > Before:
> > >     6.452 sec
> > > After:
> > >     4.382 sec
> > > Baremetal:
> > >     4.136 sec
> > > 
> > > Michael S. Tsirkin (2):
> > >   kvm: support -realtime cpu-pm=on|off
> > >   i386/cpu: make -cpu host support monitor/mwait
> > > 
> > >  include/sysemu/sysemu.h |  1 +
> > >  target/i386/cpu.h       |  9 +++++++++
> > >  target/i386/cpu.c       | 19 ++++++++++++++-----
> > >  target/i386/kvm.c       | 30 ++++++++++++++++++++++++++++++
> > >  vl.c                    |  6 ++++++
> > >  qemu-options.hx         |  9 +++++++--
> > >  6 files changed, 67 insertions(+), 7 deletions(-)
> > > 
> > > -- 
> > > MST
> > 
> > Hi Michael,
> > 
> > 1) Command line option interface
> > 
> > Why is this not an optional cpu feature such as the other features? 
> > 
> > 
> > -cpu CPU,+mwait  
> > 
> > rather than a separate, architecture independent "-realtime cpu-pm=on|off" 
> > command line option?
> 
> Because it's not just a guest flag. With guest pm on, one guest
> can severely affect the latency of others on the same host CPU.

How so ?

> > 2) Migration
> > 
> > Isnt it sufficient to check that both CPUID leafs are the same, 
> > to allow migration ?
> 
> Not at the moment since linux guests use mwait hints and latency values
> from a table in intel_idle.  If the host and guest models do not match,
> mwait will get a wrong hint.
> 
> It will not do the right thing then!
> 
> You want exactly the same host CPU for it to work.
> 
> This isn't different from how -host cpu works generally.

Ok, makes sense.

> > 1. Check that the processor supports MONITOR and MWAIT. If
> > CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at
> > ring 0.
> > 
> > 2. Query the smallest and largest line size that MONITOR uses.
> > Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. 
> > 

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
On Fri, Jun 15, 2018 at 03:32:27PM -0300, Marcelo Tosatti wrote:
> On Thu, Jun 14, 2018 at 02:37:28AM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2018 at 07:34:53PM -0300, Marcelo Tosatti wrote:
> > > On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote:
> > > > This adds ability to expose host CPU power management capabilities to
> > > > guests. For intel guests, this is sufficient for guest to enable
> > > > low power CPU power management. For AMD guests it isn't sufficient,
> > > > deeper C-states are entered using System-IO.
> > > > 
> > > > mwait based power management is tied closely to specifics of CPUID,
> > > > making migration challenging. At this point only the non-migrateable
> > > > -cpu host is supported.
> > > > 
> > > > With this patch applied, VM latency is within the noise of
> > > > baremetal for some benchmarks.
> > > > 
> > > > perf bench sched pipe results:
> > > > Before:
> > > >     6.452 sec
> > > > After:
> > > >     4.382 sec
> > > > Baremetal:
> > > >     4.136 sec
> > > > 
> > > > Michael S. Tsirkin (2):
> > > >   kvm: support -realtime cpu-pm=on|off
> > > >   i386/cpu: make -cpu host support monitor/mwait
> > > > 
> > > >  include/sysemu/sysemu.h |  1 +
> > > >  target/i386/cpu.h       |  9 +++++++++
> > > >  target/i386/cpu.c       | 19 ++++++++++++++-----
> > > >  target/i386/kvm.c       | 30 ++++++++++++++++++++++++++++++
> > > >  vl.c                    |  6 ++++++
> > > >  qemu-options.hx         |  9 +++++++--
> > > >  6 files changed, 67 insertions(+), 7 deletions(-)
> > > > 
> > > > -- 
> > > > MST
> > > 
> > > Hi Michael,
> > > 
> > > 1) Command line option interface
> > > 
> > > Why is this not an optional cpu feature such as the other features? 
> > > 
> > > 
> > > -cpu CPU,+mwait  
> > > 
> > > rather than a separate, architecture independent "-realtime cpu-pm=on|off" 
> > > command line option?
> > 
> > Because it's not just a guest flag. With guest pm on, one guest
> > can severely affect the latency of others on the same host CPU.
> 
> How so ?

Look at drivers/idle/intel_idle.c
There are states with exit latencies of 
10000

> > > 2) Migration
> > > 
> > > Isnt it sufficient to check that both CPUID leafs are the same, 
> > > to allow migration ?
> > 
> > Not at the moment since linux guests use mwait hints and latency values
> > from a table in intel_idle.  If the host and guest models do not match,
> > mwait will get a wrong hint.
> > 
> > It will not do the right thing then!
> > 
> > You want exactly the same host CPU for it to work.
> > 
> > This isn't different from how -host cpu works generally.
> 
> Ok, makes sense.
> 
> > > 1. Check that the processor supports MONITOR and MWAIT. If
> > > CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at
> > > ring 0.
> > > 
> > > 2. Query the smallest and largest line size that MONITOR uses.
> > > Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. 
> > > 

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Daniel P. Berrangé 5 years, 9 months ago
On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote:
> This adds ability to expose host CPU power management capabilities to
> guests. For intel guests, this is sufficient for guest to enable
> low power CPU power management. For AMD guests it isn't sufficient,
> deeper C-states are entered using System-IO.
> 
> mwait based power management is tied closely to specifics of CPUID,
> making migration challenging. At this point only the non-migrateable
> -cpu host is supported.
> 
> With this patch applied, VM latency is within the noise of
> baremetal for some benchmarks.
> 
> perf bench sched pipe results:
> Before:
>     6.452 sec
> After:
>     4.382 sec
> Baremetal:
>     4.136 sec
> 
> Michael S. Tsirkin (2):
>   kvm: support -realtime cpu-pm=on|off

IMHO this really shouldn't be under the -realtime flag. I don't think
the -realtime flag should ever have been introduced, and we certainly
shouldn't add more stuff under it.

"-realtime" is referring to a very specific use case, while the
properties listed under it are all general purpose features. Real
time guests just happen to be one possible use case, but it is
valid to use them for non-real time guests.

IOW, I think we should just have this as an option under -cpu or
some other *functionally* named option, not a option named after
a specific usage scenario.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Paolo Bonzini 5 years, 9 months ago
On 14/06/2018 10:18, Daniel P. Berrangé wrote:
> I don't think
> the -realtime flag should ever have been introduced, and we certainly
> shouldn't add more stuff under it.
> 
> "-realtime" is referring to a very specific use case, while the
> properties listed under it are all general purpose features. Real
> time guests just happen to be one possible use case, but it is
> valid to use them for non-real time guests.
> 
> IOW, I think we should just have this as an option under -cpu or
> some other *functionally* named option, not a option named after
> a specific usage scenario.

"-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a
device option, while this is about host behavior.  "-realtime"'s name is
awful, but I still think it's the best place for this option.  Maybe we
could call it "-realtime power-mgmt={host|guest}".

A separate issue is whether the same flag should control both
KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf.  Eduardo,
what do you think?

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Daniel P. Berrangé 5 years, 9 months ago
On Thu, Jun 14, 2018 at 05:40:41PM +0200, Paolo Bonzini wrote:
> On 14/06/2018 10:18, Daniel P. Berrangé wrote:
> > I don't think
> > the -realtime flag should ever have been introduced, and we certainly
> > shouldn't add more stuff under it.
> > 
> > "-realtime" is referring to a very specific use case, while the
> > properties listed under it are all general purpose features. Real
> > time guests just happen to be one possible use case, but it is
> > valid to use them for non-real time guests.
> > 
> > IOW, I think we should just have this as an option under -cpu or
> > some other *functionally* named option, not a option named after
> > a specific usage scenario.
> 
> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a
> device option, while this is about host behavior.  "-realtime"'s name is
> awful, but I still think it's the best place for this option.  Maybe we
> could call it "-realtime power-mgmt={host|guest}".

If none of the existing ones are a suitable fit, then we should just
introduce a new CLI arg instead stuffing it into somewhere odd.

IOW, why not just "--power-mgmt host|guest"

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Paolo Bonzini 5 years, 9 months ago
On 14/06/2018 17:44, Daniel P. Berrangé wrote:
>> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a
>> device option, while this is about host behavior.  "-realtime"'s name is
>> awful, but I still think it's the best place for this option.  Maybe we
>> could call it "-realtime power-mgmt={host|guest}".
> If none of the existing ones are a suitable fit, then we should just
> introduce a new CLI arg instead stuffing it into somewhere odd.

It is related to mlock in my opinion.  Both options are about applying
settings that reduce latency but may not be applicable in general.
Maybe "-realtime" could be renamed to "-perftune" or something like
that, but I'd like to keep the grouping.

Paolo

> IOW, why not just "--power-mgmt host|guest"


Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
On Thu, Jun 14, 2018 at 10:32:00PM +0200, Paolo Bonzini wrote:
> On 14/06/2018 17:44, Daniel P. Berrangé wrote:
> >> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a
> >> device option, while this is about host behavior.  "-realtime"'s name is
> >> awful, but I still think it's the best place for this option.  Maybe we
> >> could call it "-realtime power-mgmt={host|guest}".
> > If none of the existing ones are a suitable fit, then we should just
> > introduce a new CLI arg instead stuffing it into somewhere odd.
> 
> It is related to mlock in my opinion.  Both options are about applying
> settings that reduce latency but may not be applicable in general.

I would add, it's also about host resource handling.

With mlock guest memory is locked so there's no memory overcommit.
With cpu-pm guest does not give up host cpu which mostly disables
host cpu overcommit.


> Maybe "-realtime" could be renamed to "-perftune" or something like
> that, but I'd like to keep the grouping.
> 
> Paolo

-overcommit ?

> > IOW, why not just "--power-mgmt host|guest"

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Eduardo Habkost 5 years, 9 months ago
On Thu, Jun 14, 2018 at 05:40:41PM +0200, Paolo Bonzini wrote:
> On 14/06/2018 10:18, Daniel P. Berrangé wrote:
> > I don't think
> > the -realtime flag should ever have been introduced, and we certainly
> > shouldn't add more stuff under it.
> > 
> > "-realtime" is referring to a very specific use case, while the
> > properties listed under it are all general purpose features. Real
> > time guests just happen to be one possible use case, but it is
> > valid to use them for non-real time guests.
> > 
> > IOW, I think we should just have this as an option under -cpu or
> > some other *functionally* named option, not a option named after
> > a specific usage scenario.
> 
> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a
> device option, while this is about host behavior.  "-realtime"'s name is
> awful, but I still think it's the best place for this option.  Maybe we
> could call it "-realtime power-mgmt={host|guest}".
> 
> A separate issue is whether the same flag should control both
> KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf.  Eduardo,
> what do you think?

Making "-cpu host" be affected by a host-side option is
acceptable to me.  A "-cpu" option would be more appropriate if
we decide to allow monitor/mwait be enabled for other CPU models
too.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Paolo Bonzini 5 years, 9 months ago
On 14/06/2018 18:53, Eduardo Habkost wrote:
>>
>> A separate issue is whether the same flag should control both
>> KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf.  Eduardo,
>> what do you think?
> Making "-cpu host" be affected by a host-side option is
> acceptable to me.  A "-cpu" option would be more appropriate if
> we decide to allow monitor/mwait be enabled for other CPU models
> too.

Sounds fair enough, we should probably add a note in the documentation
of "-cpu host" though.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
On Thu, Jun 14, 2018 at 05:40:41PM +0200, Paolo Bonzini wrote:
> On 14/06/2018 10:18, Daniel P. Berrangé wrote:
> > I don't think
> > the -realtime flag should ever have been introduced, and we certainly
> > shouldn't add more stuff under it.
> > 
> > "-realtime" is referring to a very specific use case, while the
> > properties listed under it are all general purpose features. Real
> > time guests just happen to be one possible use case, but it is
> > valid to use them for non-real time guests.
> > 
> > IOW, I think we should just have this as an option under -cpu or
> > some other *functionally* named option, not a option named after
> > a specific usage scenario.
> 
> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a
> device option, while this is about host behavior.  "-realtime"'s name is
> awful, but I still think it's the best place for this option.  Maybe we
> could call it "-realtime power-mgmt={host|guest}".

I don't feel it's a good name.  The new flag allows guest to put CPU
into a low power state without host knowing about it.  This will affect
other guests on the same host CPU.  It doesn't however limit power
management to guest only, things like speedstep remain under host
control.

> A separate issue is whether the same flag should control both
> KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf.  Eduardo,
> what do you think?
> 
> Paolo

It's just for -cpu host.

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Kashyap Chamarthy 5 years, 9 months ago
(Cc: Thomas Huth)

On Thu, Jun 14, 2018 at 09:18:00AM +0100, Daniel P. Berrangé wrote:

[...]

> IMHO this really shouldn't be under the -realtime flag. I don't think
> the -realtime flag should ever have been introduced, and we certainly
> shouldn't add more stuff under it.
> 
> "-realtime" is referring to a very specific use case, while the
> properties listed under it are all general purpose features. Real
> time guests just happen to be one possible use case, but it is
> valid to use them for non-real time guests.

Given what you say above, then it sounds like the "-realtime" QEMU
option should be deprecated, and removed in a future release.

[...]


-- 
/kashyap

Re: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management
Posted by Michael S. Tsirkin 5 years, 9 months ago
On Fri, Jun 22, 2018 at 03:06:59PM +0200, Kashyap Chamarthy wrote:
> (Cc: Thomas Huth)
> 
> On Thu, Jun 14, 2018 at 09:18:00AM +0100, Daniel P. Berrangé wrote:
> 
> [...]
> 
> > IMHO this really shouldn't be under the -realtime flag. I don't think
> > the -realtime flag should ever have been introduced, and we certainly
> > shouldn't add more stuff under it.
> > 
> > "-realtime" is referring to a very specific use case, while the
> > properties listed under it are all general purpose features. Real
> > time guests just happen to be one possible use case, but it is
> > valid to use them for non-real time guests.
> 
> Given what you say above, then it sounds like the "-realtime" QEMU
> option should be deprecated, and removed in a future release.
> 
> [...]
> 

As a patch on top,  sure.

> -- 
> /kashyap