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

[Gleb Natapov]

On Tue, May 15, 2012 at 07:18:10PM -0400, Kevin O'Connor wrote:
> 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.
> 
I saw that, but I don't get why doing it this way instead of defining
the object in AML and patching it? I can define Name(S4VL, 0x2) and path
0x2 to whatever QEMU wants me to use, or I can patch Package directly
like I did.

> >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.
Seems to be so.

> 
> > 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.
Fine by me. I verified and Windows/Linux can cope with _Sx definitions
being in SSDT. If we a going to move all the code that needs patching to
this file may we should rename it to something like ssdp_dynamic.dsl?

> 
> > > > +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.
>
Each time Seabios wants to read a file it need to iterate over all/most
existing files. I can understand advantage of using files for code that
is shared between coreboot and qemu since files is what real HW uses,
but for QEMU internal code it is just overhead for the sake of it.  I do
not have strong fillings about this issue. If you think that files is
the only way forward may be you should communicate this to QEMU and put
a comment in hw/fw_cfg.h explaining that and increasing FW_CFG_FILE_SLOTS
to some ridiculously large value.

--
                        Gleb.