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

[Laszlo Ersek <lersek@redhat.com>]

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