Re: [Qemu-devel] [PATCH v2 1/2] kvm: support -realtime cpu-pm=on|off

[Michael S. Tsirkin]

On Wed, Jun 13, 2018 at 06:00:22PM -0300, Eduardo Habkost wrote:
> On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote:
> > With this flag, kvm allows guest to control host CPU power state.  This
> > increases latency for other processes using same host CPU in an
> > unpredictable way, but if decreases idle entry/exit times for the
> > running VCPU.
> > 
> > Follow-up patches will expose this capability to guest
> > (using mwait leaf).
> > 
> > Based on a patch by Wanpeng Li <address@hidden> .
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  include/sysemu/sysemu.h |  1 +
> >  target/i386/kvm.c       | 22 ++++++++++++++++++++++
> >  vl.c                    |  6 ++++++
> >  qemu-options.hx         |  9 +++++++--
> >  4 files changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > index e893f72f3b..b921c6f3b7 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -128,6 +128,7 @@ extern bool boot_strict;
> >  extern uint8_t *boot_splash_filedata;
> >  extern size_t boot_splash_filedata_size;
> >  extern bool enable_mlock;
> > +extern bool enable_cpu_pm;
> 
> After looking at patch 2/2, I see that the global variable is
> useful, and it's consistent with the existing enable_mlock
> variable.
> 
> >  extern uint8_t qemu_extra_params_fw[2];
> >  extern QEMUClockType rtc_clock;
> >  extern const char *mem_path;
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 44f70733e7..f093d55209 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -1357,6 +1357,28 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >          smram_machine_done.notify = register_smram_listener;
> >          qemu_add_machine_init_done_notifier(&smram_machine_done);
> >      }
> > +
> > +    if (enable_cpu_pm) {
> > +        int disable_exits = kvm_check_extension(s, 
> > KVM_CAP_X86_DISABLE_EXITS);
> > +        int ret;
> > +
> > +/* Work around for kernel header with a typo. TODO: fix header and drop. */
> > +#if defined(KVM_X86_DISABLE_EXITS_HTL) && 
> > !defined(KVM_X86_DISABLE_EXITS_HLT)
> > +#define KVM_X86_DISABLE_EXITS_HLT KVM_X86_DISABLE_EXITS_HTL
> > +#endif
> > +        if (disable_exits) {
> > +            disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT |
> > +                              KVM_X86_DISABLE_EXITS_HLT |
> > +                              KVM_X86_DISABLE_EXITS_PAUSE);
> > +        }
> > +
> > +        ret = kvm_vm_enable_cap(s, KVM_CAP_X86_DISABLE_EXITS, 0,
> > +                                disable_exits);
> 
> Isn't the kvm_vm_enable_cap() call supposed to be inside the "if
> (disable_exits)" block?

Doing it like this causes a warning if pm is requested but
disable halt is not supported.

But I can move it, sure - let me know.

> > +        if (ret < 0) {
> > +            error_report("kvm: guest stopping CPU not supported: %s", 
> > strerror(-ret));
> > +        }
> > +    }
> > +
> >      return 0;
> >  }
> [...]
> 
> -- 
> Eduardo