| Message ID | 1368590499-11707-2-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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; > } >
在 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; > > } > > >
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
在 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 >
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
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; }
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- hw/i386/pc.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)