Re: [Qemu-devel] [PATCH 2/2] Get system state configuration from QEMU and patcth DSDT with it.

[Kevin O'Connor]

On Tue, May 15, 2012 at 11:06:05AM +0300, Gleb Natapov wrote:
> On Mon, May 14, 2012 at 09:43:19PM -0400, Kevin O'Connor wrote:
> > On Mon, May 14, 2012 at 03:35:23PM +0300, Gleb Natapov wrote:
> > > QEMU may want to disable guest's S3/S4 support and it wants to distinguish
> > > between regular powerdown and S4 powerdown. To support that new fw_cfg
> > > option was added that passes supported system states and what value should
> > > guest use to enter each state. States are passed in 6 byte array. Each
> > > byte represents one system state. If byte at offset X has its MSB set
> > > it means that system state X is supported and to enter it guest should
> > > use the value from lowest 7 bits. Patch also detects old QEMU and uses
> > > values that work in backwards compatible way there.
> > 
> > A couple of comments - see below.
> > 
> > [...]
> > > --- a/src/acpi-dsdt.dsl
> > > +++ b/src/acpi-dsdt.dsl
> > > @@ -613,6 +613,7 @@ DefinitionBlock (
> > >       * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type 
> > > codes:
> > >       * must match piix4 emulation.
> > >       */
> > > +    ACPI_EXTRACT_NAME_STRING acpi_s3_name
> > >      Name (\_S3, Package (0x04)
> > >      {
> > >          0x01,  /* PM1a_CNT.SLP_TYP */
> > > @@ -620,10 +621,12 @@ DefinitionBlock (
> > >          Zero,  /* reserved */
> > >          Zero   /* reserved */
> > >      })
> > > +    ACPI_EXTRACT_NAME_STRING acpi_s4_name
> > > +    ACPI_EXTRACT_PKG_START acpi_s4_pkg
> > 
> > The DSDT is quite complex and has a diverse usage.  I'd feel more
> > comfortable leaving it as static and doing any dynamic work in an
> > SSDT.  In this particular case, can't the objects be turned into
> > methods which calculate the associated values and return the correct
> > results?
> Checked with WindowsXP and Linux and they work if I make _S3 to be a
> method that returns package, so we can drop ACPI_EXTRACT_PKG_START and
> do runtime calculation, but what this calculation will be based on? We
> will have to pass QEMU S4 value to AML somehow and this will involve
> patching of something eventually.

As in the other recent discussion, a struct can be built by the BIOS
and a pointer passed in via a dynamic SSDT (eg, BDAT).  Whatever data
is needed can then be passed in via that struct.

>And of course we will still have to
> patch out _S3/_S4 names in case qemu want to disable them. I do not see
> how we can disable them in any other way.

If the mere existence of _S3 tells the OS that S3 is supported, then
it will have to be patched in.

> I think the use of patching will only increase now after we let that
> genie out of the bottle, so moving each part that we want to patch in
> separate SSDT will not scale.

Why?  Just put the definitions in ssdp_pcihp.dsl instead of
acpi-dsdt.dsl - there's no real difference.

> > > +int qemu_cfg_system_states(char *states)
> > > +{
> > 
> > I'd prefer to see any new fw_cfg entries use the QEMU_CFG_FILE_DIR
> > mechanism so that seabios can use romfile_loadfile (or similar).
> > 
> The number of files you can pass over fw_cfg interface is limited due to
> implementation details. I think we should continue using regular
> fw_cfg entries for code that is QEMU specific and files for code that is
> shared with coreboot.

The QEMU_CFG_FILE_DIR is just a list of "names" and "sizes" for each
"port".  There's no fundamental limitation to the interface.  If QEMU
has a limit, we should just fix that.

-Kevin