[Qemu-devel] [PATCH 1/2] vl: boolize acpi_enabled

[Qemu-devel] [PATCH 1/2] vl: boolize acpi_enabled

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 include/hw/acpi/acpi.h |    2 +-
 vl.c                   |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 635be7b..b01a9dc 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -161,7 +161,7 @@ void acpi_gpe_ioport_writeb(ACPIREGS *ar, uint32_t addr, uint32_t val);
 uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t addr);
 
 /* acpi.c */
-extern int acpi_enabled;
+extern bool acpi_enabled;
 extern char unsigned *acpi_tables;
 extern size_t acpi_tables_len;
 
diff --git a/vl.c b/vl.c
index 6e6225f..07acd5f 100644
--- a/vl.c
+++ b/vl.c
@@ -216,7 +216,7 @@ int smp_threads = 1;
 #ifdef CONFIG_VNC
 const char *vnc_display;
 #endif
-int acpi_enabled = 1;
+bool acpi_enabled = true;
 int no_hpet = 0;
 int fd_bootchk = 1;
 static int no_reboot;
@@ -3651,7 +3651,7 @@ int main(int argc, char **argv, char **envp)
 #endif
                 break;
             case QEMU_OPTION_no_acpi:
-                acpi_enabled = 0;
+                acpi_enabled = false;
                 break;
             case QEMU_OPTION_no_hpet:
                 no_hpet = 1;
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/2] pc: reject do pc_acpi_init if acpi_enabled is false

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 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;
     }
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 2/2] pc: reject do pc_acpi_init if acpi_enabled is false

[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?

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;
>      }
> 

Re: [Qemu-devel] [PATCH 2/2] pc: reject do pc_acpi_init if acpi_enabled is false

[li guang]

在 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;
> >      }
> > 
> 

Re: [Qemu-devel] [PATCH 2/2] pc: reject do pc_acpi_init if acpi_enabled is false

[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.

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

Re: [Qemu-devel] [PATCH 2/2] pc: reject do pc_acpi_init if acpi_enabled is false

[li guang]

在 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
> 

Re: [Qemu-devel] [PATCH 2/2] pc: reject do pc_acpi_init if acpi_enabled is false

[Laszlo Ersek]

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