Patchwork [2/2] pc: reject do pc_acpi_init if acpi_enabled is false

login
register
mail settings
Submitter liguang
Date May 15, 2013, 4:01 a.m.
Message ID <1368590499-11707-2-git-send-email-lig.fnst@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/243900/
State New
Headers show

Comments

liguang - May 15, 2013, 4:01 a.m.
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Paolo Bonzini - May 15, 2013, 8:38 a.m.
Il 15/05/2013 06:01, liguang ha scritto:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>

--verbose, please.

What problem does this patch fix?

Paolo

> ---
>  hw/i386/pc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 197d218..77025a8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -982,7 +982,7 @@ void pc_acpi_init(const char *default_dsdt)
>  {
>      char *filename;
>  
> -    if (acpi_tables != NULL) {
> +    if (acpi_tables != NULL || !acpi_enabled) {
>          /* manually set via -acpitable, leave it alone */
>          return;
>      }
>
liguang - May 15, 2013, 8:54 a.m.
在 2013-05-15三的 10:38 +0200,Paolo Bonzini写道:
> Il 15/05/2013 06:01, liguang ha scritto:
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> 
> --verbose, please.
> 
> What problem does this patch fix?

Oh, sorry to be lazy ...
QEMU's option '-no-acpi' seems does not play
a correct role, even started with this, 
ACPI tables will also be embedded into BIOS, 
and there's no different between with or without it
for q35, as i can see.

here, I'm assuming '-no-acpi' is to disable ACPI.

> 
> > ---
> >  hw/i386/pc.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 197d218..77025a8 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -982,7 +982,7 @@ void pc_acpi_init(const char *default_dsdt)
> >  {
> >      char *filename;
> >  
> > -    if (acpi_tables != NULL) {
> > +    if (acpi_tables != NULL || !acpi_enabled) {
> >          /* manually set via -acpitable, leave it alone */
> >          return;
> >      }
> > 
>
Laszlo Ersek - May 15, 2013, 1:44 p.m.
On 05/15/13 10:54, li guang wrote:
> 在 2013-05-15三的 10:38 +0200,Paolo Bonzini写道:
>> Il 15/05/2013 06:01, liguang ha scritto:
>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>
>> --verbose, please.
>>
>> What problem does this patch fix?
> 
> Oh, sorry to be lazy ...
> QEMU's option '-no-acpi' seems does not play
> a correct role, even started with this, 
> ACPI tables will also be embedded into BIOS, 
> and there's no different between with or without it
> for q35, as i can see.
> 
> here, I'm assuming '-no-acpi' is to disable ACPI.

-no-acpi disables a block of code in pc_init1() [hw/i386/pc_piix.c],
namely piix4_pm_init() and smbus_eeprom_init().

pc_acpi_init() loads / exports a default DSDT for the boot firmware. If
the -acpitable switch is passed, then that code doesn't run.

I think disabling PM but keeping the default DSDT from SeaBIOS is a
valid use case; the DSDT seems to contain a bunch of non-PM
functionality (see src/acpi-dsdt*.dsl in SeaBIOS). The "-no-acpi" switch
is likely a misnomer (it should say "-no-acpi-pm" or some such), but in
any case I believe it should not prevent exporting the DSDT.

Currently you can prevent exporting the default DSDT for example with:

  -acpitable sig=NONE,data=/dev/null

This will export a table with signature NONE, otherwise qemu-default
ACPI table headers, and no table contents. It will also prevent
pc_acpi_init() from running. See "AcpiTableOptions" in
"qapi-schema.json" and "hw/acpi/core.c".

Thanks,
Laszlo
liguang - May 16, 2013, 12:14 a.m.
在 2013-05-15三的 15:44 +0200,Laszlo Ersek写道:
> On 05/15/13 10:54, li guang wrote:
> > 在 2013-05-15三的 10:38 +0200,Paolo Bonzini写道:
> >> Il 15/05/2013 06:01, liguang ha scritto:
> >>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >>
> >> --verbose, please.
> >>
> >> What problem does this patch fix?
> > 
> > Oh, sorry to be lazy ...
> > QEMU's option '-no-acpi' seems does not play
> > a correct role, even started with this, 
> > ACPI tables will also be embedded into BIOS, 
> > and there's no different between with or without it
> > for q35, as i can see.
> > 
> > here, I'm assuming '-no-acpi' is to disable ACPI.
> 
> -no-acpi disables a block of code in pc_init1() [hw/i386/pc_piix.c],
> namely piix4_pm_init() and smbus_eeprom_init().
> 
> pc_acpi_init() loads / exports a default DSDT for the boot firmware. If
> the -acpitable switch is passed, then that code doesn't run.

Yes, if '-acpitable' & '-no-acpi' passed, I think QEMU is valid to
override '-no-acpi' by '-acpitable'.

> 
> I think disabling PM but keeping the default DSDT from SeaBIOS is a
> valid use case; the DSDT seems to contain a bunch of non-PM
> functionality (see src/acpi-dsdt*.dsl in SeaBIOS). The "-no-acpi" switch
> is likely a misnomer (it should say "-no-acpi-pm" or some such), but in
> any case I believe it should not prevent exporting the DSDT.

what's the purpose of only disable PM by '-no-acpi'?
can we use -no-acpi to disable whole ACPI?

Thanks!

> 
> Currently you can prevent exporting the default DSDT for example with:
> 
>   -acpitable sig=NONE,data=/dev/null
> 
> This will export a table with signature NONE, otherwise qemu-default
> ACPI table headers, and no table contents. It will also prevent
> pc_acpi_init() from running. See "AcpiTableOptions" in
> "qapi-schema.json" and "hw/acpi/core.c".
> 
> Thanks,
> Laszlo
>
Laszlo Ersek - May 16, 2013, 10:22 a.m.
On 05/16/13 02:14, li guang wrote:
> 在 2013-05-15三的 15:44 +0200,Laszlo Ersek写道:
>> On 05/15/13 10:54, li guang wrote:
>>> 在 2013-05-15三的 10:38 +0200,Paolo Bonzini写道:
>>>> Il 15/05/2013 06:01, liguang ha scritto:
>>>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>>>
>>>> --verbose, please.
>>>>
>>>> What problem does this patch fix?
>>>
>>> Oh, sorry to be lazy ...
>>> QEMU's option '-no-acpi' seems does not play
>>> a correct role, even started with this, 
>>> ACPI tables will also be embedded into BIOS, 
>>> and there's no different between with or without it
>>> for q35, as i can see.
>>>
>>> here, I'm assuming '-no-acpi' is to disable ACPI.
>>
>> -no-acpi disables a block of code in pc_init1() [hw/i386/pc_piix.c],
>> namely piix4_pm_init() and smbus_eeprom_init().
>>
>> pc_acpi_init() loads / exports a default DSDT for the boot firmware. If
>> the -acpitable switch is passed, then that code doesn't run.
> 
> Yes, if '-acpitable' & '-no-acpi' passed, I think QEMU is valid to
> override '-no-acpi' by '-acpitable'.

I think these flags are orthogonal. As far as I can see, the former
turns off a piece of hardware emulation (like support for suspend /
hibernate and the System Control Interrupt, and more).

The latter enables the user to provide custom ACPI tables for SeaBIOS
(or other boot firmware) in place of the default DSDT.

It seems that originally "acpi_enabled" (-no-acpi) used to control both
purposes, see commit 6515b20: the call to acpi_bios_init() used to
depend on acpi_enabled != 0. However the two goals got separated with
commit a5954d5 apparently.

>> I think disabling PM but keeping the default DSDT from SeaBIOS is a
>> valid use case; the DSDT seems to contain a bunch of non-PM
>> functionality (see src/acpi-dsdt*.dsl in SeaBIOS). The "-no-acpi" switch
>> is likely a misnomer (it should say "-no-acpi-pm" or some such), but in
>> any case I believe it should not prevent exporting the DSDT.
> 
> what's the purpose of only disable PM by '-no-acpi'?

I mentioned the src/acpi-dsdt*.dsl files in SeaBIOS that get built into
the default DSDT. With "-no-acpi" only, qemu keeps at least the
following working:
- the \DBUG ACPI method [src/acpi-dsdt-dbug.dsl],
- definition of the \_SB\HPET ("PNP0103") device [src/acpi-dsdt-hpet.dsl],
- descriptions (interrupts, io-ports) of ISA devices (RTC, keyboard,
mouse, floppy disk controller, parport, serial port)
[src/acpi-dsdt-isa.dsl],
- description of the PCI IO windows (PCI host bridge windows)
[src/acpi-dsdt-pci-crs.dsl], whose lack can mess up VGA display for
example (see the "pci=nocrs" kernel option),
- the PCI interrupt routing table [acpi-dsdt.dsl].

Again, I believe "-no-acpi" has probably become a misnomer by now and it
means "-no-acpi-pm" in reality.

> can we use -no-acpi to disable whole ACPI?

I cannot *prove* it would be wrong, but I feel insecure about it,
considering the commit history and the non-PM-related functionality in
the default DSDT. Currently a user may expect the above contents to be
present in ACPI tables, and only PM (plus maybe hotplug) not to work,
when passing -no-acpi.

Anyway I'm no authority on this -- anyone, feel free to chime in. I'm
certainly not NACKing the patch, I just have concerns.

Thanks,
Laszlo

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 197d218..77025a8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -982,7 +982,7 @@  void pc_acpi_init(const char *default_dsdt)
 {
     char *filename;
 
-    if (acpi_tables != NULL) {
+    if (acpi_tables != NULL || !acpi_enabled) {
         /* manually set via -acpitable, leave it alone */
         return;
     }