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

[Kevin O'Connor]

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?

[...]
> --- a/src/paravirt.c
> +++ b/src/paravirt.c
> @@ -92,6 +92,22 @@ int qemu_cfg_irq0_override(void)
>      return v;
>  }
>  
> +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).

-Kevin