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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 2 Aug 2018 11:18:08 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 26 Jul 2018 19:09:22 +0300
> "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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" <mst@redhat.com> 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.
Posted a non dynamic variant
  [RFC PATCH 0/4] "pc: acpi: _CST support"
with uses conventional approach to compose _CST object
without opening AML to ABI issues

[...]