Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation

[Igor Mammedov]

On Thu, 26 Jul 2018 19:09:22 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote:
> > On Wed, 25 Jul 2018 15:44:37 +0300
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >   
> > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 10 Jul 2018 03:01:30 +0300
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > >     
> > > > > Now that basic support for guest CPU PM is upstream, I started looking
> > > > > for making it migrateable.  Since a VM can be migrated between 
> > > > > different
> > > > > hosts, PM info needs to change each time with move the VM to a 
> > > > > different
> > > > > host.    
> > > > Considering notification is async, so there will be a window when
> > > > guest will be using old Cstates on new host. What will happen if
> > > > machine is migrated to host that doesn't support a Cstate
> > > > that was used on source when guest was started?    
> > > 
> > > My testing shows mwait with a wrong hint works, presumably it just uses
> > > a lot of power.
> > >   
> > > > > This adds infrastructure - based on Load/Unload - to support exactly
> > > > > that: QEMU generates AML (changing it on migration) and stores it in
> > > > > reserved memory, OSPM loads _CST from there on demand.    
> > > > Cool and very tempting idea but I have 2 worries about this approach:
> > > > 1. How well does it work with Windows based guests?
> > > >    (I've tried something similar but generating new AML from AML itself
> > > >     to get rid of our long if/else chains there to make up Notify 
> > > > target name.
> > > >     I ditched it (unfortunately I don't recall why) )    
> > > 
> > > After trying it, I can tell you why - it's a horrid mess of
> > > unreadable code, with arbitrary restraints on package
> > > length etc.  
> > in my case it was only 4 character NameString, but Windows probably
> > wasn't happy or just ignored it.
> > Considering recent development (TPM series) it might have been issue
> > with SYSTEM_MEMORY not working properly if used as byte buffer field.
> > 
> > Even if it's an unreadable mess it's still stable mess and very constrained
> > one within a single firmware blob that came from source.  
> 
> That's exectly the argument we had for keeping ACPI
> generation in bios. It's just not an interface that scales.
> 
> > Hence it's more preferable than split brain in this series.
> > 
> > But I don't think we even need dynamic AML for _CST usecase at all,
> > existing cpuhp interface should work just fine for it and should be
> > simpler as all infrastructure is already there.  
> 
> Not sure I get what you mean. Could you post a patch?
> 
> > > > 2. (probably biggest issue) Loading dynamically generated AML
> > > >    basically would make all AML code ABI, so that static AML
> > > >    in RAM of old QEMU version would match dynamic generated
> > > >    one on target side with new QEMU (/me generalizing approach beyond 
> > > > _CST support).
> > > >    I'd try to keep our AML version less as much as possible
> > > >    and go this route only if there is no other way to do it.    
> > > 
> > > That's a good point, thanks for bringing this up!
> > > 
> > > So it seems that we will need to define the ABI, yes. All AML code is
> > > an over-statement though, there are specific entry points
> > > we must maintain, right?  
> > Well, I'm rather unsure what and where could break,
> > hence I'm afraid of a new beast and it probably won't be easy
> > to convince me that ABI would be able to keep things manageable
> > and stable.
> > Considering big amount of AML code that we already have
> > I'm not confident eye balling during review and testing the latest
> > firmware/qemu would be sufficient as we suddenly would have exploded
> > test matrix where firmware in use is a mixed from old/and new parts.  
> 
> Well there's one exported method so far and it relies on one container
> device in static table set which does not sound too hard to keep stable.
> 
> Given how simple the dynamic table is, how about just checking it
> with a unit test? It is literally a single return statement.
> If it returns a valid package, that is all that we
> care about. I can write some firmware to test that constraint.
> 
> 
> > > And that in the end isn't fundamentally different from the ABIs
> > > mandated by the ACPI spec.
> > > 
> > > So I have these ideas to try to mitigate the issues:
> > > - document the ABI (like we have in the ACPI spec)
> > > - use a specific prefix for all external calls (like _ for ACPI spec ones)
> > > - use a specific (different) prefix for all internal loaded calls (like
> > >   [A-Z] for non-ACPI spec ones)  
> > ACPI spec horrible mess and bad example (even committee isn't able to keep 
> > it consistent)
> > but it's necessary evil (compared to no spec at all).
> > Adding ABI requirement will complicate coding/reviewing for
> > already complex AML compared to current non ABI way.
> > Hence from maintainability POV we should stay away from this approach
> > if there is a viable alternative.  
> 
> I agree. Not that I see one.

I'll try to come up with a patch to make use of cpuhp registers
to pass CST values and it's AML counterpart.
Then you'd need to just wire it up to whatever means you use
to get these values from host.

BTW:
I've noticed you have used SystemIO registers in CST
(my patch used FFH registers) using mmio defeats purpose
as it causes guest exit instead of mwait that we are targeting.

Is it just for demo purpose of RFC with new AML interface?

> 
> > > 
> > > Thoughts?
> > >   
> > > > > This is a partially functional prototype.
> > > > > 
> > > > > What works:
> > > > >     loading _CST dynamically and reporting it to OSPM
> > > > > 
> > > > > What doesn't:
> > > > >     detecting host configuration and generating correct _CST package
> > > > >     notifying guest about the change to trigger _CST re-evaluation
> > > > >     disabling mwait/halt exists as appropriate
> > > > > 
> > > > > Michael S. Tsirkin (6):
> > > > >   acpi: aml: add aml_register()
> > > > >   acpi: generalize aml_package / aml_varpackage
> > > > >   acpi: aml_load/aml_unload
> > > > >   acpi: export acpi_checksum
> > > > >   acpi: init header without linking
> > > > >   acpi: aml generation for _CST
> > > > >   pc: HACK: acpi: tie in _CST object to Processor
> > > > > 
> > > > >  include/hw/acpi/acpi.h      |   2 +
> > > > >  include/hw/acpi/aml-build.h |  14 ++-
> > > > >  include/hw/acpi/cst.h       |   8 ++
> > > > >  include/hw/i386/pc.h        |   5 ++
> > > > >  hw/acpi/aml-build.c         |  81 ++++++++++++++---
> > > > >  hw/acpi/core.c              |   2 +-
> > > > >  hw/acpi/cpu.c               |   5 ++
> > > > >  hw/acpi/cpu_hotplug.c       |  11 +--
> > > > >  hw/acpi/cst.c               | 173 
> > > > > ++++++++++++++++++++++++++++++++++++
> > > > >  hw/arm/virt-acpi-build.c    |   2 +-
> > > > >  hw/i386/acpi-build.c        |  10 ++-
> > > > >  hw/acpi/Makefile.objs       |   2 +-
> > > > >  12 files changed, 290 insertions(+), 25 deletions(-)
> > > > >  create mode 100644 include/hw/acpi/cst.h
> > > > >  create mode 100644 hw/acpi/cst.c
> > > > >     
> > >