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

[Igor Mammedov]

On Fri, 10 Aug 2018 02:09:58 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> 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.
Ok, I'll add support for more CStates

> 
> > 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.
it's fairly recent, but if we plan in advance we can support it
as well.

> 
> > >   
> > > > 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