Re: [Qemu-devel] [RFC PATCH 0/4] "pc: acpi: _CST support"

[Michael S. Tsirkin]

On Thu, Aug 09, 2018 at 12:13:24PM +0200, Igor Mammedov wrote:
> On Wed, 8 Aug 2018 23:20:25 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, Aug 08, 2018 at 05:15:45PM +0200, Igor Mammedov wrote:
> > > It's an alternative approach to
> > >  1) [PATCH hack dontapply v2 0/7] Dynamic _CST generation
> > > which instead of dynamic AML loading uses static AML with
> > > dynamic values.  It allows us to keep firmware blob static and
> > > to avoid split firmware issue (1) in case of cross version migration.  
> > 
> > I think there's a misunderstanding. That patch only declares a couple of
> > states but that is just for debugging/demonstration purposes.  A typical
> > real CPU has more states (e.g. some intel CPUs have ~10 levels).
> yep, baremetal has more CStates, but that depends on what we are trying to
> achieve here.
> If goal is to use mwait/monitor instead o halt when idle
> to reduce wakeup latencies then 1 entry in _CST is sufficient.

1 entry is not sufficient in my testing. My testing shows
we want to stay in guest for states with latency up to ~200 usec
to get measureable latency gains.


> If target is reduce power consumption than we would need more entries.
> In the later case _CST is probably not sufficient as it provides only
> power/latency pair while intel_idle also provides target residence.
> So if power mgmt target is considered, we need to use new _LPI
> which means guest will need support for it as well, not sure if x86
> kernel supports it nor aware of such support in Windows (hopefully it 
> would just ignore unknown method)

I think we want to address both goals.

Linux on x86 does support _LPI and I agree it's a good idea to be able
to pass that in. Will take a look. But intel_idle seems to just set
target = latency * 2 so I am not sure it's all that important.

> > 
> > > ABI in this case is confined to cpu hotplug IO registers
> > > (i.e. do it old school way, like we used to do so far).
> > > This way we don't have to add yet another ABI to keep dynamic
> > > AML code under control (1).
> > > 
> > > Tested  with: XPsp3 - ws2106 guests.
> > > 
> > > CC: "Michael S. Tsirkin" <address@hidden>
> > > 
> > > 
> > > Igor Mammedov (3):
> > >   acpi: add aml_create_byte_field()
> > >   pc: acpi: add _CST support
> > >   acpi: add support for CST update notification
> > > 
> > > Michael S. Tsirkin (1):
> > >   acpi: aml: add aml_register()
> > > 
> > >  include/hw/acpi/aml-build.h     |   6 ++
> > >  include/hw/acpi/cpu.h           |  10 +++
> > >  docs/specs/acpi_cpu_hotplug.txt |  21 +++++-
> > >  hw/acpi/aml-build.c             |  28 +++++++
> > >  hw/acpi/cpu.c                   | 158 
> > > +++++++++++++++++++++++++++++++++++++++-
> > >  hw/acpi/piix4.c                 |   2 +
> > >  hw/i386/acpi-build.c            |   5 +-
> > >  tests/bios-tables-test.c        |   1 +
> > >  8 files changed, 225 insertions(+), 6 deletions(-)
> > > 
> > > -- 
> > > 2.7.4