| Message ID | 1366316544-1428-8-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 18, 2013 at 10:22:24PM +0200, Laszlo Ersek wrote: > This patch reuses some code from SeaBIOS, which was originally under > LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This > relicensing has been acked by all contributors that had contributed to the > code since the v2->v3 relicense. ACKs approving the v2+ relicensing are > listed below. The list might include ACKs from people not holding > copyright on any parts of the reused code, but it's better to err on the > side of caution and include them. > > Affected SeaBIOS files (GPLv2+ license headers added) > <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>: > > src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++ > src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++ > src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++ > src/acpi-dsdt-isa.dsl | 15 +++++++++++++++ > src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++ > src/acpi.c | 14 +++++++++++++- > src/acpi.h | 14 ++++++++++++++ > src/ssdt-misc.dsl | 15 +++++++++++++++ > src/ssdt-pcihp.dsl | 15 +++++++++++++++ > src/ssdt-proc.dsl | 15 +++++++++++++++ > tools/acpi_extract.py | 13 ++++++++++++- > tools/acpi_extract_preprocess.py | 13 ++++++++++++- > 12 files changed, 171 insertions(+), 3 deletions(-) > > Each one of the listed people agreed to the following: > > > If you allow the use of your contribution in QEMU under the > > terms of GPLv2 or later as proposed by this patch, > > please respond to this mail including the line: > > > > Acked-by: Name <email address> > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > Acked-by: Jan Kiszka <jan.kiszka@siemens.com> > Acked-by: Jason Baron <jbaron@akamai.com> > Acked-by: David Woodhouse <David.Woodhouse@intel.com> > Acked-by: Gleb Natapov <gleb@redhat.com> > Acked-by: Marcelo Tosatti <mtosatti@redhat.com> > Acked-by: Dave Frodin <dave.frodin@se-eng.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Acked-by: Kevin O'Connor <kevin@koconnor.net> > Acked-by: Laszlo Ersek <lersek@redhat.com> > Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> > Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> > Acked-by: Magnus Christensson <magnus.christensson@intel.com> > Acked-by: Hu Tao <hutao@cn.fujitsu.com> > Acked-by: Eduardo Habkost <ehabkost@redhat.com> > > The patch incorporates ideas/suggestions from Michael Tsirkin's prototype > code: > - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with > i386-specific ACPI table stuff, > - separate preparation of individual tables from their installation as > fw_cfg files, > - install these fw_cfg files inside pc_memory_init(), which is shared by > piix4/q35, > - add the above licensing-related block to the commit message. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > configure | 12 ++++ > hw/i386/Makefile.objs | 1 + > hw/i386/acpi.h | 9 +++ > hw/i386/acpi.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++ > hw/i386/pc.c | 23 +++++++ > 5 files changed, 204 insertions(+), 0 deletions(-) > create mode 100644 hw/i386/acpi.h > create mode 100644 hw/i386/acpi.c > > diff --git a/configure b/configure > index ed49f91..45a5f55 100755 > --- a/configure > +++ b/configure > @@ -241,6 +241,7 @@ gtk="" > gtkabi="2.0" > tpm="no" > libssh2="" > +dynamic_acpi="no" > > # parse CC options first > for opt do > @@ -928,6 +929,10 @@ for opt do > ;; > --enable-libssh2) libssh2="yes" > ;; > + --disable-dynamic-acpi) dynamic_acpi="no" > + ;; > + --enable-dynamic-acpi) dynamic_acpi="yes" > + ;; > *) echo "ERROR: unknown option $opt"; show_help="yes" > ;; > esac > @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" > echo " --enable-tpm enable TPM support" > echo " --disable-libssh2 disable ssh block device support" > echo " --enable-libssh2 enable ssh block device support" > +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" > +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" > echo "" > echo "NOTE: The object files are built at the place where configure is launched" > exit 1 > @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" > echo "TPM support $tpm" > echo "libssh2 support $libssh2" > echo "TPM passthrough $tpm_passthrough" > +echo "dynamic ACPI tables $dynamic_acpi" > > if test "$sdl_too_old" = "yes"; then > echo "-> Your SDL version is too old - please upgrade to have SDL support" > @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then > echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak > fi > > +if test "$dynamic_acpi" = "yes"; then > + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak > +fi > + > # USB host support > case "$usb" in > linux) > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index 205d22e..8429d52 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -1,6 +1,7 @@ > obj-$(CONFIG_KVM) += kvm/ > obj-y += multiboot.o smbios.o > obj-y += pc.o pc_piix.o pc_q35.o > +obj-$(CONFIG_DYN_ACPI) += acpi.o > obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o > > obj-y += kvmvapic.o > diff --git a/hw/i386/acpi.h b/hw/i386/acpi.h > new file mode 100644 > index 0000000..94f9ad3 > --- /dev/null > +++ b/hw/i386/acpi.h > @@ -0,0 +1,9 @@ > +#ifndef QEMU_HW_I386_ACPI_H > +#define QEMU_HW_I386_ACPI_H > + > +#include <stddef.h> > + > +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, > + unsigned num_lapic); > + > +#endif > diff --git a/hw/i386/acpi.c b/hw/i386/acpi.c > new file mode 100644 > index 0000000..179cdf5 > --- /dev/null > +++ b/hw/i386/acpi.c > @@ -0,0 +1,159 @@ > +/* > + * Copyright (c) 2013 Red Hat Inc. > + * Copyright (C) 2008-2010 Kevin O'Connor <kevin@koconnor.net> > + * Copyright (C) 2006 Fabrice Bellard > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#include "hw/acpi/acpi.h" > +#include "hw/i386/acpi.h" > +#include "kvm_i386.h" > +#include "sysemu/sysemu.h" > + > +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, > + const char *sig) > +{ > + g_assert(blob_size >= sizeof *std_hdr); > + > + *std_hdr = acpi_dfl_hdr; > + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); > + strncpy(std_hdr->oem_id, "QEMU ", sizeof std_hdr->oem_id); > + strncpy(std_hdr->oem_table_id + 4, sig, sizeof std_hdr->oem_table_id - 4); > + std_hdr->length = cpu_to_le32(blob_size); > + std_hdr->checksum = acpi_checksum((uint8_t *)std_hdr, blob_size); > +} > + > +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, > + unsigned num_lapic) > +{ > + typedef struct { > + uint8_t type; > + uint8_t length; > + } QEMU_PACKED AcpiSubHdr; > + > + AcpiTableStdHdr *std_hdr; > + struct { > + uint32_t lapic_addr; /* Local Interrupt Controller Address */ > + uint32_t flags; /* Multiple APIC flags */ > + } QEMU_PACKED *madt; > + struct { > + AcpiSubHdr hdr; > + uint8_t processor_id; /* ACPI Processor ID */ > + uint8_t apic_id; /* APIC ID */ > + uint32_t flags; /* LOcal APIC flags */ > + } QEMU_PACKED *lapic; > + struct { > + AcpiSubHdr hdr; > + uint8_t io_apic_id; /* The I/O APIC's ID */ > + uint8_t reserved; /* constant zero */ > + uint32_t io_apic_addr; /* 32-bit physical address to access */ > + uint32_t gsi_base; /* interrupt inputs start here */ > + } QEMU_PACKED *io_apic; > + struct { > + AcpiSubHdr hdr; > + uint8_t bus; /* constant zero: ISA */ > + uint8_t source; /* this bus-relative interrupt source... */ > + uint32_t gsi; /* ... will signal this global system interrupt */ > + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ > + } QEMU_PACKED *int_src_ovr; > + struct { > + AcpiSubHdr hdr; > + uint8_t processor_id; /* ACPI Processor ID */ > + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ > + uint8_t lint; /* LAPIC interrupt input for NMI */ > + } QEMU_PACKED *lapic_nmi; > + > + static const uint8_t pci_isa_irq[] = { 5, 9, 10, 11 }; > + > + unsigned num_int_src_ovr, i; > + size_t blob_size; > + char unsigned *blob; > + > + num_int_src_ovr = sizeof pci_isa_irq + kvm_allows_irq0_override(); > + > + blob_size = (sizeof *std_hdr) * 1 + > + (sizeof *madt) * 1 + > + (sizeof *lapic) * num_lapic + > + (sizeof *io_apic) * 1 + > + (sizeof *int_src_ovr) * num_int_src_ovr + > + (sizeof *lapic_nmi) * 1; > + blob = g_malloc(blob_size); > + > + std_hdr = (void *)blob; > + madt = (void *)(std_hdr + 1 ); > + lapic = (void *)(madt + 1 ); > + io_apic = (void *)(lapic + num_lapic ); > + int_src_ovr = (void *)(io_apic + 1 ); > + lapic_nmi = (void *)(int_src_ovr + num_int_src_ovr); > + > + madt->lapic_addr = cpu_to_le32(APIC_DEFAULT_ADDRESS); > + madt->flags = cpu_to_le32(1); /* PCAT_COMPAT */ > + > + /* create a Local APIC structure for each possible APIC ID */ > + for (i = 0; i < num_lapic; ++i) { > + lapic[i].hdr.type = 0; /* Processor Local APIC */ > + lapic[i].hdr.length = sizeof *lapic; > + lapic[i].processor_id = i; > + lapic[i].apic_id = i; > + lapic[i].flags = cpu_to_le32(0); /* disabled */ > + } > + /* enable the CPUs with a CPU index in the [0..smp_cpus-1] range */ > + for (i = 0; i < smp_cpus; ++i) { > + lapic[x86_cpu_apic_id_from_index(i)].flags = cpu_to_le32(1); > + } > + > + io_apic->hdr.type = 1; /* I/O APIC */ > + io_apic->hdr.length = sizeof *io_apic; > + io_apic->io_apic_id = 0; > + io_apic->reserved = 0; > + io_apic->io_apic_addr = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS); > + io_apic->gsi_base = cpu_to_le32(0); > + > + for (i = 0; i < sizeof pci_isa_irq; ++i) { > + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ > + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; > + int_src_ovr[i].bus = 0; > + int_src_ovr[i].source = pci_isa_irq[i]; > + int_src_ovr[i].gsi = cpu_to_le32(pci_isa_irq[i]); > + int_src_ovr[i].flags = cpu_to_le16(0xd); > + /* active high, level-triggered */ > + } > + if (kvm_allows_irq0_override()) { > + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ > + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; > + int_src_ovr[i].bus = 0; > + int_src_ovr[i].source = 0; > + int_src_ovr[i].gsi = cpu_to_le32(2); > + int_src_ovr[i].flags = cpu_to_le16(0); /* conforms to bus spec */ > + } > + > + lapic_nmi->hdr.type = 4; /* Local APIC NMI */ > + lapic_nmi->hdr.length = sizeof *lapic_nmi; > + lapic_nmi->processor_id = 0xff; /* all processors */ > + lapic_nmi->flags = cpu_to_le16(0); /* conforms to bus spec */ > + lapic_nmi->lint = 1; /* NMI connected to LAPIC input LINT1 */ > + > + acpi_table_fill_hdr(std_hdr, blob_size, "APIC"); > + *out_blob = blob; > + *out_blob_size = blob_size; > +} > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 8727489..15c6284 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -54,6 +54,10 @@ > #include "qemu/config-file.h" > #include "hw/acpi/acpi.h" > > +#ifdef CONFIG_DYN_ACPI > +# include "hw/i386/acpi.h" > +#endif > + > /* debug PC/ISA interrupts */ > //#define DEBUG_IRQ > > @@ -922,6 +926,21 @@ void pc_acpi_init(const char *default_dsdt) > } > } > > +#ifdef CONFIG_DYN_ACPI > +static void pc_acpi_madt(FWCfgState *fw_cfg) > +{ > + unsigned num_lapic; > + char unsigned *blob; > + size_t blob_size; > + > + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ > + num_lapic = pc_apic_id_limit(max_cpus); > + > + acpi_build_madt(&blob, &blob_size, num_lapic); > + fw_cfg_add_file(fw_cfg, "etc/acpi/APIC", blob, blob_size); > +} > +#endif > + > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > const char *kernel_filename, > const char *kernel_cmdline, > @@ -974,6 +993,10 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > fw_cfg = bochs_bios_init(); > rom_set_fw(fw_cfg); Let's cut out the ifdefs, just use an if below. Compiler is smart enough to do it right. > +#ifdef CONFIG_DYN_ACPI > + pc_acpi_madt(fw_cfg); > +#endif > + > if (linux_boot) { > load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size); > } > -- > 1.7.1 >
On 04/18/13 22:30, Michael S. Tsirkin wrote: > On Thu, Apr 18, 2013 at 10:22:24PM +0200, Laszlo Ersek wrote: >> This patch reuses some code from SeaBIOS, which was originally under >> LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This >> relicensing has been acked by all contributors that had contributed to the >> code since the v2->v3 relicense. ACKs approving the v2+ relicensing are >> listed below. The list might include ACKs from people not holding >> copyright on any parts of the reused code, but it's better to err on the >> side of caution and include them. >> >> Affected SeaBIOS files (GPLv2+ license headers added) >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>: >> >> src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-isa.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++ >> src/acpi.c | 14 +++++++++++++- >> src/acpi.h | 14 ++++++++++++++ >> src/ssdt-misc.dsl | 15 +++++++++++++++ >> src/ssdt-pcihp.dsl | 15 +++++++++++++++ >> src/ssdt-proc.dsl | 15 +++++++++++++++ >> tools/acpi_extract.py | 13 ++++++++++++- >> tools/acpi_extract_preprocess.py | 13 ++++++++++++- >> 12 files changed, 171 insertions(+), 3 deletions(-) >> >> Each one of the listed people agreed to the following: >> >>> If you allow the use of your contribution in QEMU under the >>> terms of GPLv2 or later as proposed by this patch, >>> please respond to this mail including the line: >>> >>> Acked-by: Name <email address> >> >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> Acked-by: Jan Kiszka <jan.kiszka@siemens.com> >> Acked-by: Jason Baron <jbaron@akamai.com> >> Acked-by: David Woodhouse <David.Woodhouse@intel.com> >> Acked-by: Gleb Natapov <gleb@redhat.com> >> Acked-by: Marcelo Tosatti <mtosatti@redhat.com> >> Acked-by: Dave Frodin <dave.frodin@se-eng.com> >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >> Acked-by: Kevin O'Connor <kevin@koconnor.net> >> Acked-by: Laszlo Ersek <lersek@redhat.com> >> Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> >> Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> >> Acked-by: Magnus Christensson <magnus.christensson@intel.com> >> Acked-by: Hu Tao <hutao@cn.fujitsu.com> >> Acked-by: Eduardo Habkost <ehabkost@redhat.com> >> >> The patch incorporates ideas/suggestions from Michael Tsirkin's prototype >> code: >> - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with >> i386-specific ACPI table stuff, >> - separate preparation of individual tables from their installation as >> fw_cfg files, >> - install these fw_cfg files inside pc_memory_init(), which is shared by >> piix4/q35, >> - add the above licensing-related block to the commit message. >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> configure | 12 ++++ >> hw/i386/Makefile.objs | 1 + >> hw/i386/acpi.h | 9 +++ >> hw/i386/acpi.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/i386/pc.c | 23 +++++++ >> 5 files changed, 204 insertions(+), 0 deletions(-) >> create mode 100644 hw/i386/acpi.h >> create mode 100644 hw/i386/acpi.c >> >> diff --git a/configure b/configure >> index ed49f91..45a5f55 100755 >> --- a/configure >> +++ b/configure >> @@ -241,6 +241,7 @@ gtk="" >> gtkabi="2.0" >> tpm="no" >> libssh2="" >> +dynamic_acpi="no" >> >> # parse CC options first >> for opt do >> @@ -928,6 +929,10 @@ for opt do >> ;; >> --enable-libssh2) libssh2="yes" >> ;; >> + --disable-dynamic-acpi) dynamic_acpi="no" >> + ;; >> + --enable-dynamic-acpi) dynamic_acpi="yes" >> + ;; >> *) echo "ERROR: unknown option $opt"; show_help="yes" >> ;; >> esac >> @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" >> echo " --enable-tpm enable TPM support" >> echo " --disable-libssh2 disable ssh block device support" >> echo " --enable-libssh2 enable ssh block device support" >> +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" >> +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" >> echo "" >> echo "NOTE: The object files are built at the place where configure is launched" >> exit 1 >> @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" >> echo "TPM support $tpm" >> echo "libssh2 support $libssh2" >> echo "TPM passthrough $tpm_passthrough" >> +echo "dynamic ACPI tables $dynamic_acpi" >> >> if test "$sdl_too_old" = "yes"; then >> echo "-> Your SDL version is too old - please upgrade to have SDL support" >> @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then >> echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak >> fi >> >> +if test "$dynamic_acpi" = "yes"; then >> + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak >> +fi >> + >> # USB host support >> case "$usb" in >> linux) >> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs >> index 205d22e..8429d52 100644 >> --- a/hw/i386/Makefile.objs >> +++ b/hw/i386/Makefile.objs >> @@ -1,6 +1,7 @@ >> obj-$(CONFIG_KVM) += kvm/ >> obj-y += multiboot.o smbios.o >> obj-y += pc.o pc_piix.o pc_q35.o >> +obj-$(CONFIG_DYN_ACPI) += acpi.o >> obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o >> >> obj-y += kvmvapic.o >> diff --git a/hw/i386/acpi.h b/hw/i386/acpi.h >> new file mode 100644 >> index 0000000..94f9ad3 >> --- /dev/null >> +++ b/hw/i386/acpi.h >> @@ -0,0 +1,9 @@ >> +#ifndef QEMU_HW_I386_ACPI_H >> +#define QEMU_HW_I386_ACPI_H >> + >> +#include <stddef.h> >> + >> +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, >> + unsigned num_lapic); >> + >> +#endif >> diff --git a/hw/i386/acpi.c b/hw/i386/acpi.c >> new file mode 100644 >> index 0000000..179cdf5 >> --- /dev/null >> +++ b/hw/i386/acpi.c >> @@ -0,0 +1,159 @@ >> +/* >> + * Copyright (c) 2013 Red Hat Inc. >> + * Copyright (C) 2008-2010 Kevin O'Connor <kevin@koconnor.net> >> + * Copyright (C) 2006 Fabrice Bellard >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, see <http://www.gnu.org/licenses/>. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >> + * THE SOFTWARE. >> + */ >> + >> +#include "hw/acpi/acpi.h" >> +#include "hw/i386/acpi.h" >> +#include "kvm_i386.h" >> +#include "sysemu/sysemu.h" >> + >> +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, >> + const char *sig) >> +{ >> + g_assert(blob_size >= sizeof *std_hdr); >> + >> + *std_hdr = acpi_dfl_hdr; >> + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); >> + strncpy(std_hdr->oem_id, "QEMU ", sizeof std_hdr->oem_id); >> + strncpy(std_hdr->oem_table_id + 4, sig, sizeof std_hdr->oem_table_id - 4); >> + std_hdr->length = cpu_to_le32(blob_size); >> + std_hdr->checksum = acpi_checksum((uint8_t *)std_hdr, blob_size); >> +} >> + >> +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, >> + unsigned num_lapic) >> +{ >> + typedef struct { >> + uint8_t type; >> + uint8_t length; >> + } QEMU_PACKED AcpiSubHdr; >> + >> + AcpiTableStdHdr *std_hdr; >> + struct { >> + uint32_t lapic_addr; /* Local Interrupt Controller Address */ >> + uint32_t flags; /* Multiple APIC flags */ >> + } QEMU_PACKED *madt; >> + struct { >> + AcpiSubHdr hdr; >> + uint8_t processor_id; /* ACPI Processor ID */ >> + uint8_t apic_id; /* APIC ID */ >> + uint32_t flags; /* LOcal APIC flags */ >> + } QEMU_PACKED *lapic; >> + struct { >> + AcpiSubHdr hdr; >> + uint8_t io_apic_id; /* The I/O APIC's ID */ >> + uint8_t reserved; /* constant zero */ >> + uint32_t io_apic_addr; /* 32-bit physical address to access */ >> + uint32_t gsi_base; /* interrupt inputs start here */ >> + } QEMU_PACKED *io_apic; >> + struct { >> + AcpiSubHdr hdr; >> + uint8_t bus; /* constant zero: ISA */ >> + uint8_t source; /* this bus-relative interrupt source... */ >> + uint32_t gsi; /* ... will signal this global system interrupt */ >> + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ >> + } QEMU_PACKED *int_src_ovr; >> + struct { >> + AcpiSubHdr hdr; >> + uint8_t processor_id; /* ACPI Processor ID */ >> + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ >> + uint8_t lint; /* LAPIC interrupt input for NMI */ >> + } QEMU_PACKED *lapic_nmi; >> + >> + static const uint8_t pci_isa_irq[] = { 5, 9, 10, 11 }; >> + >> + unsigned num_int_src_ovr, i; >> + size_t blob_size; >> + char unsigned *blob; >> + >> + num_int_src_ovr = sizeof pci_isa_irq + kvm_allows_irq0_override(); >> + >> + blob_size = (sizeof *std_hdr) * 1 + >> + (sizeof *madt) * 1 + >> + (sizeof *lapic) * num_lapic + >> + (sizeof *io_apic) * 1 + >> + (sizeof *int_src_ovr) * num_int_src_ovr + >> + (sizeof *lapic_nmi) * 1; >> + blob = g_malloc(blob_size); >> + >> + std_hdr = (void *)blob; >> + madt = (void *)(std_hdr + 1 ); >> + lapic = (void *)(madt + 1 ); >> + io_apic = (void *)(lapic + num_lapic ); >> + int_src_ovr = (void *)(io_apic + 1 ); >> + lapic_nmi = (void *)(int_src_ovr + num_int_src_ovr); >> + >> + madt->lapic_addr = cpu_to_le32(APIC_DEFAULT_ADDRESS); >> + madt->flags = cpu_to_le32(1); /* PCAT_COMPAT */ >> + >> + /* create a Local APIC structure for each possible APIC ID */ >> + for (i = 0; i < num_lapic; ++i) { >> + lapic[i].hdr.type = 0; /* Processor Local APIC */ >> + lapic[i].hdr.length = sizeof *lapic; >> + lapic[i].processor_id = i; >> + lapic[i].apic_id = i; >> + lapic[i].flags = cpu_to_le32(0); /* disabled */ >> + } >> + /* enable the CPUs with a CPU index in the [0..smp_cpus-1] range */ >> + for (i = 0; i < smp_cpus; ++i) { >> + lapic[x86_cpu_apic_id_from_index(i)].flags = cpu_to_le32(1); >> + } >> + >> + io_apic->hdr.type = 1; /* I/O APIC */ >> + io_apic->hdr.length = sizeof *io_apic; >> + io_apic->io_apic_id = 0; >> + io_apic->reserved = 0; >> + io_apic->io_apic_addr = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS); >> + io_apic->gsi_base = cpu_to_le32(0); >> + >> + for (i = 0; i < sizeof pci_isa_irq; ++i) { >> + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ >> + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; >> + int_src_ovr[i].bus = 0; >> + int_src_ovr[i].source = pci_isa_irq[i]; >> + int_src_ovr[i].gsi = cpu_to_le32(pci_isa_irq[i]); >> + int_src_ovr[i].flags = cpu_to_le16(0xd); >> + /* active high, level-triggered */ >> + } >> + if (kvm_allows_irq0_override()) { >> + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ >> + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; >> + int_src_ovr[i].bus = 0; >> + int_src_ovr[i].source = 0; >> + int_src_ovr[i].gsi = cpu_to_le32(2); >> + int_src_ovr[i].flags = cpu_to_le16(0); /* conforms to bus spec */ >> + } >> + >> + lapic_nmi->hdr.type = 4; /* Local APIC NMI */ >> + lapic_nmi->hdr.length = sizeof *lapic_nmi; >> + lapic_nmi->processor_id = 0xff; /* all processors */ >> + lapic_nmi->flags = cpu_to_le16(0); /* conforms to bus spec */ >> + lapic_nmi->lint = 1; /* NMI connected to LAPIC input LINT1 */ >> + >> + acpi_table_fill_hdr(std_hdr, blob_size, "APIC"); >> + *out_blob = blob; >> + *out_blob_size = blob_size; >> +} >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 8727489..15c6284 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -54,6 +54,10 @@ >> #include "qemu/config-file.h" >> #include "hw/acpi/acpi.h" >> >> +#ifdef CONFIG_DYN_ACPI >> +# include "hw/i386/acpi.h" >> +#endif >> + >> /* debug PC/ISA interrupts */ >> //#define DEBUG_IRQ >> >> @@ -922,6 +926,21 @@ void pc_acpi_init(const char *default_dsdt) >> } >> } >> >> +#ifdef CONFIG_DYN_ACPI >> +static void pc_acpi_madt(FWCfgState *fw_cfg) >> +{ >> + unsigned num_lapic; >> + char unsigned *blob; >> + size_t blob_size; >> + >> + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ >> + num_lapic = pc_apic_id_limit(max_cpus); >> + >> + acpi_build_madt(&blob, &blob_size, num_lapic); >> + fw_cfg_add_file(fw_cfg, "etc/acpi/APIC", blob, blob_size); >> +} >> +#endif >> + >> FWCfgState *pc_memory_init(MemoryRegion *system_memory, >> const char *kernel_filename, >> const char *kernel_cmdline, >> @@ -974,6 +993,10 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, >> fw_cfg = bochs_bios_init(); >> rom_set_fw(fw_cfg); > > Let's cut out the ifdefs, just use an if below. Compiler is smart enough > to do it right. I considered that, because it's how SeaBIOS does it. There are several "depths" to do this; for example, I could even build hw/i386/acpi.o with the switch being off, and pass the object file to the linker (ie. no variable substitution in Makefile.objs) and let the linker omit it when it finds no references. However, I obviously checked how existent parts of the code do it, specifically CONFIG_XEN, and I followed that manner. See "hw/i386/pc_piix.c" and "arch_init.c". There's not one CONFIG_XXX feature test macro in the qemu source (that I can find) that is examined with the "if" statement. IOW with all due respect I disagree. Thanks, Laszlo > >> +#ifdef CONFIG_DYN_ACPI >> + pc_acpi_madt(fw_cfg); >> +#endif >> + >> if (linux_boot) { >> load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size); >> } >> -- >> 1.7.1 >>
On Fri, Apr 19, 2013 at 12:58:03PM +0200, Laszlo Ersek wrote: > On 04/18/13 22:30, Michael S. Tsirkin wrote: > > On Thu, Apr 18, 2013 at 10:22:24PM +0200, Laszlo Ersek wrote: > >> This patch reuses some code from SeaBIOS, which was originally under > >> LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This > >> relicensing has been acked by all contributors that had contributed to the > >> code since the v2->v3 relicense. ACKs approving the v2+ relicensing are > >> listed below. The list might include ACKs from people not holding > >> copyright on any parts of the reused code, but it's better to err on the > >> side of caution and include them. > >> > >> Affected SeaBIOS files (GPLv2+ license headers added) > >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>: > >> > >> src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-isa.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++ > >> src/acpi.c | 14 +++++++++++++- > >> src/acpi.h | 14 ++++++++++++++ > >> src/ssdt-misc.dsl | 15 +++++++++++++++ > >> src/ssdt-pcihp.dsl | 15 +++++++++++++++ > >> src/ssdt-proc.dsl | 15 +++++++++++++++ > >> tools/acpi_extract.py | 13 ++++++++++++- > >> tools/acpi_extract_preprocess.py | 13 ++++++++++++- > >> 12 files changed, 171 insertions(+), 3 deletions(-) > >> > >> Each one of the listed people agreed to the following: > >> > >>> If you allow the use of your contribution in QEMU under the > >>> terms of GPLv2 or later as proposed by this patch, > >>> please respond to this mail including the line: > >>> > >>> Acked-by: Name <email address> > >> > >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> > >> Acked-by: Jan Kiszka <jan.kiszka@siemens.com> > >> Acked-by: Jason Baron <jbaron@akamai.com> > >> Acked-by: David Woodhouse <David.Woodhouse@intel.com> > >> Acked-by: Gleb Natapov <gleb@redhat.com> > >> Acked-by: Marcelo Tosatti <mtosatti@redhat.com> > >> Acked-by: Dave Frodin <dave.frodin@se-eng.com> > >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> > >> Acked-by: Kevin O'Connor <kevin@koconnor.net> > >> Acked-by: Laszlo Ersek <lersek@redhat.com> > >> Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> > >> Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> > >> Acked-by: Magnus Christensson <magnus.christensson@intel.com> > >> Acked-by: Hu Tao <hutao@cn.fujitsu.com> > >> Acked-by: Eduardo Habkost <ehabkost@redhat.com> > >> > >> The patch incorporates ideas/suggestions from Michael Tsirkin's prototype > >> code: > >> - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with > >> i386-specific ACPI table stuff, > >> - separate preparation of individual tables from their installation as > >> fw_cfg files, > >> - install these fw_cfg files inside pc_memory_init(), which is shared by > >> piix4/q35, > >> - add the above licensing-related block to the commit message. > >> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> configure | 12 ++++ > >> hw/i386/Makefile.objs | 1 + > >> hw/i386/acpi.h | 9 +++ > >> hw/i386/acpi.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> hw/i386/pc.c | 23 +++++++ > >> 5 files changed, 204 insertions(+), 0 deletions(-) > >> create mode 100644 hw/i386/acpi.h > >> create mode 100644 hw/i386/acpi.c > >> > >> diff --git a/configure b/configure > >> index ed49f91..45a5f55 100755 > >> --- a/configure > >> +++ b/configure > >> @@ -241,6 +241,7 @@ gtk="" > >> gtkabi="2.0" > >> tpm="no" > >> libssh2="" > >> +dynamic_acpi="no" > >> > >> # parse CC options first > >> for opt do > >> @@ -928,6 +929,10 @@ for opt do > >> ;; > >> --enable-libssh2) libssh2="yes" > >> ;; > >> + --disable-dynamic-acpi) dynamic_acpi="no" > >> + ;; > >> + --enable-dynamic-acpi) dynamic_acpi="yes" > >> + ;; > >> *) echo "ERROR: unknown option $opt"; show_help="yes" > >> ;; > >> esac > >> @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" > >> echo " --enable-tpm enable TPM support" > >> echo " --disable-libssh2 disable ssh block device support" > >> echo " --enable-libssh2 enable ssh block device support" > >> +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" > >> +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" > >> echo "" > >> echo "NOTE: The object files are built at the place where configure is launched" > >> exit 1 > >> @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" > >> echo "TPM support $tpm" > >> echo "libssh2 support $libssh2" > >> echo "TPM passthrough $tpm_passthrough" > >> +echo "dynamic ACPI tables $dynamic_acpi" > >> > >> if test "$sdl_too_old" = "yes"; then > >> echo "-> Your SDL version is too old - please upgrade to have SDL support" > >> @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then > >> echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak > >> fi > >> > >> +if test "$dynamic_acpi" = "yes"; then > >> + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak > >> +fi > >> + > >> # USB host support > >> case "$usb" in > >> linux) > >> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > >> index 205d22e..8429d52 100644 > >> --- a/hw/i386/Makefile.objs > >> +++ b/hw/i386/Makefile.objs > >> @@ -1,6 +1,7 @@ > >> obj-$(CONFIG_KVM) += kvm/ > >> obj-y += multiboot.o smbios.o > >> obj-y += pc.o pc_piix.o pc_q35.o > >> +obj-$(CONFIG_DYN_ACPI) += acpi.o > >> obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o > >> > >> obj-y += kvmvapic.o > >> diff --git a/hw/i386/acpi.h b/hw/i386/acpi.h > >> new file mode 100644 > >> index 0000000..94f9ad3 > >> --- /dev/null > >> +++ b/hw/i386/acpi.h > >> @@ -0,0 +1,9 @@ > >> +#ifndef QEMU_HW_I386_ACPI_H > >> +#define QEMU_HW_I386_ACPI_H > >> + > >> +#include <stddef.h> > >> + > >> +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, > >> + unsigned num_lapic); > >> + > >> +#endif > >> diff --git a/hw/i386/acpi.c b/hw/i386/acpi.c > >> new file mode 100644 > >> index 0000000..179cdf5 > >> --- /dev/null > >> +++ b/hw/i386/acpi.c > >> @@ -0,0 +1,159 @@ > >> +/* > >> + * Copyright (c) 2013 Red Hat Inc. > >> + * Copyright (C) 2008-2010 Kevin O'Connor <kevin@koconnor.net> > >> + * Copyright (C) 2006 Fabrice Bellard > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License as published by > >> + * the Free Software Foundation; either version 2 of the License, or > >> + * (at your option) any later version. > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > >> + * You should have received a copy of the GNU General Public License along > >> + * with this program; if not, see <http://www.gnu.org/licenses/>. > >> + * > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > >> + * THE SOFTWARE. > >> + */ > >> + > >> +#include "hw/acpi/acpi.h" > >> +#include "hw/i386/acpi.h" > >> +#include "kvm_i386.h" > >> +#include "sysemu/sysemu.h" > >> + > >> +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, > >> + const char *sig) > >> +{ > >> + g_assert(blob_size >= sizeof *std_hdr); > >> + > >> + *std_hdr = acpi_dfl_hdr; > >> + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); > >> + strncpy(std_hdr->oem_id, "QEMU ", sizeof std_hdr->oem_id); > >> + strncpy(std_hdr->oem_table_id + 4, sig, sizeof std_hdr->oem_table_id - 4); > >> + std_hdr->length = cpu_to_le32(blob_size); > >> + std_hdr->checksum = acpi_checksum((uint8_t *)std_hdr, blob_size); > >> +} > >> + > >> +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, > >> + unsigned num_lapic) > >> +{ > >> + typedef struct { > >> + uint8_t type; > >> + uint8_t length; > >> + } QEMU_PACKED AcpiSubHdr; > >> + > >> + AcpiTableStdHdr *std_hdr; > >> + struct { > >> + uint32_t lapic_addr; /* Local Interrupt Controller Address */ > >> + uint32_t flags; /* Multiple APIC flags */ > >> + } QEMU_PACKED *madt; > >> + struct { > >> + AcpiSubHdr hdr; > >> + uint8_t processor_id; /* ACPI Processor ID */ > >> + uint8_t apic_id; /* APIC ID */ > >> + uint32_t flags; /* LOcal APIC flags */ > >> + } QEMU_PACKED *lapic; > >> + struct { > >> + AcpiSubHdr hdr; > >> + uint8_t io_apic_id; /* The I/O APIC's ID */ > >> + uint8_t reserved; /* constant zero */ > >> + uint32_t io_apic_addr; /* 32-bit physical address to access */ > >> + uint32_t gsi_base; /* interrupt inputs start here */ > >> + } QEMU_PACKED *io_apic; > >> + struct { > >> + AcpiSubHdr hdr; > >> + uint8_t bus; /* constant zero: ISA */ > >> + uint8_t source; /* this bus-relative interrupt source... */ > >> + uint32_t gsi; /* ... will signal this global system interrupt */ > >> + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ > >> + } QEMU_PACKED *int_src_ovr; > >> + struct { > >> + AcpiSubHdr hdr; > >> + uint8_t processor_id; /* ACPI Processor ID */ > >> + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ > >> + uint8_t lint; /* LAPIC interrupt input for NMI */ > >> + } QEMU_PACKED *lapic_nmi; > >> + > >> + static const uint8_t pci_isa_irq[] = { 5, 9, 10, 11 }; > >> + > >> + unsigned num_int_src_ovr, i; > >> + size_t blob_size; > >> + char unsigned *blob; > >> + > >> + num_int_src_ovr = sizeof pci_isa_irq + kvm_allows_irq0_override(); > >> + > >> + blob_size = (sizeof *std_hdr) * 1 + > >> + (sizeof *madt) * 1 + > >> + (sizeof *lapic) * num_lapic + > >> + (sizeof *io_apic) * 1 + > >> + (sizeof *int_src_ovr) * num_int_src_ovr + > >> + (sizeof *lapic_nmi) * 1; > >> + blob = g_malloc(blob_size); > >> + > >> + std_hdr = (void *)blob; > >> + madt = (void *)(std_hdr + 1 ); > >> + lapic = (void *)(madt + 1 ); > >> + io_apic = (void *)(lapic + num_lapic ); > >> + int_src_ovr = (void *)(io_apic + 1 ); > >> + lapic_nmi = (void *)(int_src_ovr + num_int_src_ovr); > >> + > >> + madt->lapic_addr = cpu_to_le32(APIC_DEFAULT_ADDRESS); > >> + madt->flags = cpu_to_le32(1); /* PCAT_COMPAT */ > >> + > >> + /* create a Local APIC structure for each possible APIC ID */ > >> + for (i = 0; i < num_lapic; ++i) { > >> + lapic[i].hdr.type = 0; /* Processor Local APIC */ > >> + lapic[i].hdr.length = sizeof *lapic; > >> + lapic[i].processor_id = i; > >> + lapic[i].apic_id = i; > >> + lapic[i].flags = cpu_to_le32(0); /* disabled */ > >> + } > >> + /* enable the CPUs with a CPU index in the [0..smp_cpus-1] range */ > >> + for (i = 0; i < smp_cpus; ++i) { > >> + lapic[x86_cpu_apic_id_from_index(i)].flags = cpu_to_le32(1); > >> + } > >> + > >> + io_apic->hdr.type = 1; /* I/O APIC */ > >> + io_apic->hdr.length = sizeof *io_apic; > >> + io_apic->io_apic_id = 0; > >> + io_apic->reserved = 0; > >> + io_apic->io_apic_addr = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS); > >> + io_apic->gsi_base = cpu_to_le32(0); > >> + > >> + for (i = 0; i < sizeof pci_isa_irq; ++i) { > >> + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ > >> + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; > >> + int_src_ovr[i].bus = 0; > >> + int_src_ovr[i].source = pci_isa_irq[i]; > >> + int_src_ovr[i].gsi = cpu_to_le32(pci_isa_irq[i]); > >> + int_src_ovr[i].flags = cpu_to_le16(0xd); > >> + /* active high, level-triggered */ > >> + } > >> + if (kvm_allows_irq0_override()) { > >> + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ > >> + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; > >> + int_src_ovr[i].bus = 0; > >> + int_src_ovr[i].source = 0; > >> + int_src_ovr[i].gsi = cpu_to_le32(2); > >> + int_src_ovr[i].flags = cpu_to_le16(0); /* conforms to bus spec */ > >> + } > >> + > >> + lapic_nmi->hdr.type = 4; /* Local APIC NMI */ > >> + lapic_nmi->hdr.length = sizeof *lapic_nmi; > >> + lapic_nmi->processor_id = 0xff; /* all processors */ > >> + lapic_nmi->flags = cpu_to_le16(0); /* conforms to bus spec */ > >> + lapic_nmi->lint = 1; /* NMI connected to LAPIC input LINT1 */ > >> + > >> + acpi_table_fill_hdr(std_hdr, blob_size, "APIC"); > >> + *out_blob = blob; > >> + *out_blob_size = blob_size; > >> +} > >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >> index 8727489..15c6284 100644 > >> --- a/hw/i386/pc.c > >> +++ b/hw/i386/pc.c > >> @@ -54,6 +54,10 @@ > >> #include "qemu/config-file.h" > >> #include "hw/acpi/acpi.h" > >> > >> +#ifdef CONFIG_DYN_ACPI > >> +# include "hw/i386/acpi.h" > >> +#endif > >> + > >> /* debug PC/ISA interrupts */ > >> //#define DEBUG_IRQ > >> > >> @@ -922,6 +926,21 @@ void pc_acpi_init(const char *default_dsdt) > >> } > >> } > >> > >> +#ifdef CONFIG_DYN_ACPI > >> +static void pc_acpi_madt(FWCfgState *fw_cfg) > >> +{ > >> + unsigned num_lapic; > >> + char unsigned *blob; > >> + size_t blob_size; > >> + > >> + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ > >> + num_lapic = pc_apic_id_limit(max_cpus); > >> + > >> + acpi_build_madt(&blob, &blob_size, num_lapic); > >> + fw_cfg_add_file(fw_cfg, "etc/acpi/APIC", blob, blob_size); > >> +} > >> +#endif > >> + > >> FWCfgState *pc_memory_init(MemoryRegion *system_memory, > >> const char *kernel_filename, > >> const char *kernel_cmdline, > >> @@ -974,6 +993,10 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > >> fw_cfg = bochs_bios_init(); > >> rom_set_fw(fw_cfg); > > > > Let's cut out the ifdefs, just use an if below. Compiler is smart enough > > to do it right. > > I considered that, because it's how SeaBIOS does it. > > There are several "depths" to do this; for example, I could even build > hw/i386/acpi.o with the switch being off, and pass the object file to > the linker (ie. no variable substitution in Makefile.objs) and let the > linker omit it when it finds no references. > > However, I obviously checked how existent parts of the code do it, > specifically CONFIG_XEN, and I followed that manner. See > "hw/i386/pc_piix.c" and "arch_init.c". There's not one CONFIG_XXX > feature test macro in the qemu source (that I can find) that is examined > with the "if" statement. Good point. > IOW with all due respect I disagree. > > Thanks, > Laszlo Fine, it's not a big deal. > > > >> +#ifdef CONFIG_DYN_ACPI > >> + pc_acpi_madt(fw_cfg); > >> +#endif > >> + > >> if (linux_boot) { > >> load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size); > >> } > >> -- > >> 1.7.1 > >>
Laszlo Ersek <lersek@redhat.com> writes: > This patch reuses some code from SeaBIOS, which was originally under > LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This > relicensing has been acked by all contributors that had contributed to the > code since the v2->v3 relicense. ACKs approving the v2+ relicensing are > listed below. The list might include ACKs from people not holding > copyright on any parts of the reused code, but it's better to err on the > side of caution and include them. > > Affected SeaBIOS files (GPLv2+ license headers added) > <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>: > > src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++ > src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++ > src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++ > src/acpi-dsdt-isa.dsl | 15 +++++++++++++++ > src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++ > src/acpi.c | 14 +++++++++++++- > src/acpi.h | 14 ++++++++++++++ > src/ssdt-misc.dsl | 15 +++++++++++++++ > src/ssdt-pcihp.dsl | 15 +++++++++++++++ > src/ssdt-proc.dsl | 15 +++++++++++++++ > tools/acpi_extract.py | 13 ++++++++++++- > tools/acpi_extract_preprocess.py | 13 ++++++++++++- > 12 files changed, 171 insertions(+), 3 deletions(-) > > Each one of the listed people agreed to the following: > >> If you allow the use of your contribution in QEMU under the >> terms of GPLv2 or later as proposed by this patch, >> please respond to this mail including the line: >> >> Acked-by: Name <email address> > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > Acked-by: Jan Kiszka <jan.kiszka@siemens.com> > Acked-by: Jason Baron <jbaron@akamai.com> > Acked-by: David Woodhouse <David.Woodhouse@intel.com> > Acked-by: Gleb Natapov <gleb@redhat.com> > Acked-by: Marcelo Tosatti <mtosatti@redhat.com> > Acked-by: Dave Frodin <dave.frodin@se-eng.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Acked-by: Kevin O'Connor <kevin@koconnor.net> > Acked-by: Laszlo Ersek <lersek@redhat.com> > Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> > Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> > Acked-by: Magnus Christensson <magnus.christensson@intel.com> > Acked-by: Hu Tao <hutao@cn.fujitsu.com> > Acked-by: Eduardo Habkost <ehabkost@redhat.com> > > The patch incorporates ideas/suggestions from Michael Tsirkin's prototype > code: > - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with > i386-specific ACPI table stuff, > - separate preparation of individual tables from their installation as > fw_cfg files, > - install these fw_cfg files inside pc_memory_init(), which is shared by > piix4/q35, > - add the above licensing-related block to the commit message. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > configure | 12 ++++ > hw/i386/Makefile.objs | 1 + > hw/i386/acpi.h | 9 +++ > hw/i386/acpi.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++ > hw/i386/pc.c | 23 +++++++ > 5 files changed, 204 insertions(+), 0 deletions(-) > create mode 100644 hw/i386/acpi.h > create mode 100644 hw/i386/acpi.c > > diff --git a/configure b/configure > index ed49f91..45a5f55 100755 > --- a/configure > +++ b/configure > @@ -241,6 +241,7 @@ gtk="" > gtkabi="2.0" > tpm="no" > libssh2="" > +dynamic_acpi="no" > > # parse CC options first > for opt do > @@ -928,6 +929,10 @@ for opt do > ;; > --enable-libssh2) libssh2="yes" > ;; > + --disable-dynamic-acpi) dynamic_acpi="no" > + ;; > + --enable-dynamic-acpi) dynamic_acpi="yes" > + ;; > *) echo "ERROR: unknown option $opt"; show_help="yes" > ;; > esac > @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" > echo " --enable-tpm enable TPM support" > echo " --disable-libssh2 disable ssh block device support" > echo " --enable-libssh2 enable ssh block device support" > +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" > +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" > echo "" > echo "NOTE: The object files are built at the place where configure is launched" > exit 1 > @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" > echo "TPM support $tpm" > echo "libssh2 support $libssh2" > echo "TPM passthrough $tpm_passthrough" > +echo "dynamic ACPI tables $dynamic_acpi" > > if test "$sdl_too_old" = "yes"; then > echo "-> Your SDL version is too old - please upgrade to have SDL support" > @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then > echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak > fi > > +if test "$dynamic_acpi" = "yes"; then > + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak > +fi > + > # USB host support > case "$usb" in > linux) No reason for this to be a configure option. > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs > index 205d22e..8429d52 100644 > --- a/hw/i386/Makefile.objs > +++ b/hw/i386/Makefile.objs > @@ -1,6 +1,7 @@ > obj-$(CONFIG_KVM) += kvm/ > obj-y += multiboot.o smbios.o > obj-y += pc.o pc_piix.o pc_q35.o > +obj-$(CONFIG_DYN_ACPI) += acpi.o > obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o > > obj-y += kvmvapic.o > diff --git a/hw/i386/acpi.h b/hw/i386/acpi.h > new file mode 100644 > index 0000000..94f9ad3 > --- /dev/null > +++ b/hw/i386/acpi.h > @@ -0,0 +1,9 @@ > +#ifndef QEMU_HW_I386_ACPI_H > +#define QEMU_HW_I386_ACPI_H Missing copyright. > + > +#include <stddef.h> QEMU style would normally be to use qemu-common.h here but honestly I prefer using system headers when it's possible. Just FYI. > + > +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, > + unsigned num_lapic); > + > +#endif > diff --git a/hw/i386/acpi.c b/hw/i386/acpi.c > new file mode 100644 > index 0000000..179cdf5 > --- /dev/null > +++ b/hw/i386/acpi.c > @@ -0,0 +1,159 @@ > +/* > + * Copyright (c) 2013 Red Hat Inc. > + * Copyright (C) 2008-2010 Kevin O'Connor <kevin@koconnor.net> > + * Copyright (C) 2006 Fabrice Bellard > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#include "hw/acpi/acpi.h" > +#include "hw/i386/acpi.h" > +#include "kvm_i386.h" > +#include "sysemu/sysemu.h" > + > +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, > + const char *sig) > +{ > + g_assert(blob_size >= sizeof *std_hdr); > + > + *std_hdr = acpi_dfl_hdr; > + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); Should use () with sizeof and avoid strncpy. It almost never has the behavior you want with respect to NULL termination (unless you explicitly want to not NULL terminate when hitting bufsize). > + strncpy(std_hdr->oem_id, "QEMU ", sizeof std_hdr->oem_id); > + strncpy(std_hdr->oem_table_id + 4, sig, sizeof std_hdr->oem_table_id - 4); > + std_hdr->length = cpu_to_le32(blob_size); > + std_hdr->checksum = acpi_checksum((uint8_t *)std_hdr, blob_size); > +} > + > +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, > + unsigned num_lapic) > +{ > + typedef struct { > + uint8_t type; > + uint8_t length; > + } QEMU_PACKED AcpiSubHdr; > + > + AcpiTableStdHdr *std_hdr; > + struct { > + uint32_t lapic_addr; /* Local Interrupt Controller Address */ > + uint32_t flags; /* Multiple APIC flags */ > + } QEMU_PACKED *madt; > + struct { > + AcpiSubHdr hdr; > + uint8_t processor_id; /* ACPI Processor ID */ > + uint8_t apic_id; /* APIC ID */ > + uint32_t flags; /* LOcal APIC flags */ > + } QEMU_PACKED *lapic; > + struct { > + AcpiSubHdr hdr; > + uint8_t io_apic_id; /* The I/O APIC's ID */ > + uint8_t reserved; /* constant zero */ > + uint32_t io_apic_addr; /* 32-bit physical address to access */ > + uint32_t gsi_base; /* interrupt inputs start here */ > + } QEMU_PACKED *io_apic; > + struct { > + AcpiSubHdr hdr; > + uint8_t bus; /* constant zero: ISA */ > + uint8_t source; /* this bus-relative interrupt source... */ > + uint32_t gsi; /* ... will signal this global system interrupt */ > + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ > + } QEMU_PACKED *int_src_ovr; > + struct { > + AcpiSubHdr hdr; > + uint8_t processor_id; /* ACPI Processor ID */ > + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ > + uint8_t lint; /* LAPIC interrupt input for NMI */ > + } QEMU_PACKED *lapic_nmi; > + > + static const uint8_t pci_isa_irq[] = { 5, 9, 10, 11 }; I understand why it's hardcoded but I wish it wasn't. > + unsigned num_int_src_ovr, i; > + size_t blob_size; > + char unsigned *blob; > + > + num_int_src_ovr = sizeof pci_isa_irq + kvm_allows_irq0_override(); > + > + blob_size = (sizeof *std_hdr) * 1 + > + (sizeof *madt) * 1 + > + (sizeof *lapic) * num_lapic + > + (sizeof *io_apic) * 1 + > + (sizeof *int_src_ovr) * num_int_src_ovr + > + (sizeof *lapic_nmi) * 1; > + blob = g_malloc(blob_size); > + > + std_hdr = (void *)blob; > + madt = (void *)(std_hdr + 1 ); > + lapic = (void *)(madt + 1 ); > + io_apic = (void *)(lapic + num_lapic ); > + int_src_ovr = (void *)(io_apic + 1 ); > + lapic_nmi = (void *)(int_src_ovr + num_int_src_ovr); > + > + madt->lapic_addr = cpu_to_le32(APIC_DEFAULT_ADDRESS); > + madt->flags = cpu_to_le32(1); /* PCAT_COMPAT */ > + > + /* create a Local APIC structure for each possible APIC ID */ > + for (i = 0; i < num_lapic; ++i) { > + lapic[i].hdr.type = 0; /* Processor Local APIC */ > + lapic[i].hdr.length = sizeof *lapic; > + lapic[i].processor_id = i; > + lapic[i].apic_id = i; > + lapic[i].flags = cpu_to_le32(0); /* disabled */ > + } > + /* enable the CPUs with a CPU index in the [0..smp_cpus-1] range */ > + for (i = 0; i < smp_cpus; ++i) { > + lapic[x86_cpu_apic_id_from_index(i)].flags = cpu_to_le32(1); > + } > + > + io_apic->hdr.type = 1; /* I/O APIC */ > + io_apic->hdr.length = sizeof *io_apic; > + io_apic->io_apic_id = 0; > + io_apic->reserved = 0; > + io_apic->io_apic_addr = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS); > + io_apic->gsi_base = cpu_to_le32(0); > + > + for (i = 0; i < sizeof pci_isa_irq; ++i) { > + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ > + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; > + int_src_ovr[i].bus = 0; > + int_src_ovr[i].source = pci_isa_irq[i]; > + int_src_ovr[i].gsi = cpu_to_le32(pci_isa_irq[i]); > + int_src_ovr[i].flags = cpu_to_le16(0xd); > + /* active high, level-triggered */ > + } > + if (kvm_allows_irq0_override()) { > + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ > + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; > + int_src_ovr[i].bus = 0; > + int_src_ovr[i].source = 0; > + int_src_ovr[i].gsi = cpu_to_le32(2); > + int_src_ovr[i].flags = cpu_to_le16(0); /* conforms to bus spec */ > + } > + > + lapic_nmi->hdr.type = 4; /* Local APIC NMI */ > + lapic_nmi->hdr.length = sizeof *lapic_nmi; > + lapic_nmi->processor_id = 0xff; /* all processors */ > + lapic_nmi->flags = cpu_to_le16(0); /* conforms to bus spec */ > + lapic_nmi->lint = 1; /* NMI connected to LAPIC input LINT1 */ > + > + acpi_table_fill_hdr(std_hdr, blob_size, "APIC"); > + *out_blob = blob; > + *out_blob_size = blob_size; > +} > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 8727489..15c6284 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -54,6 +54,10 @@ > #include "qemu/config-file.h" > #include "hw/acpi/acpi.h" > > +#ifdef CONFIG_DYN_ACPI > +# include "hw/i386/acpi.h" > +#endif > + > /* debug PC/ISA interrupts */ > //#define DEBUG_IRQ > > @@ -922,6 +926,21 @@ void pc_acpi_init(const char *default_dsdt) > } > } > > +#ifdef CONFIG_DYN_ACPI > +static void pc_acpi_madt(FWCfgState *fw_cfg) > +{ > + unsigned num_lapic; > + char unsigned *blob; > + size_t blob_size; > + > + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ > + num_lapic = pc_apic_id_limit(max_cpus); > + > + acpi_build_madt(&blob, &blob_size, num_lapic); I'd be a lot happier if we were passing more information to this routine and not hard coding it. For instance, the PCI interrupt assignments, the APIC ids, the number of available CPUs, etc. The commit message also doesn't provide any reason about why we would want this. The cover letter provides a reference at least but cover letters don't end up in git history. Regards, Anthony Liguori > + fw_cfg_add_file(fw_cfg, "etc/acpi/APIC", blob, blob_size); > +} > +#endif > + > FWCfgState *pc_memory_init(MemoryRegion *system_memory, > const char *kernel_filename, > const char *kernel_cmdline, > @@ -974,6 +993,10 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, > fw_cfg = bochs_bios_init(); > rom_set_fw(fw_cfg); > > +#ifdef CONFIG_DYN_ACPI > + pc_acpi_madt(fw_cfg); > +#endif > + > if (linux_boot) { > load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size); > } > -- > 1.7.1
On Thu, Apr 25, 2013 at 02:03:05PM -0500, Anthony Liguori wrote: > Laszlo Ersek <lersek@redhat.com> writes: > > --- /dev/null > > +++ b/hw/i386/acpi.h > > + > > +#include <stddef.h> > > QEMU style would normally be to use qemu-common.h here but honestly I > prefer using system headers when it's possible. Just FYI. I thought we were actively trying to stop including qemu-common.h from other header files, because it easily leads to unexpected (and hard to fix) circular header dependencies.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Apr 25, 2013 at 02:03:05PM -0500, Anthony Liguori wrote: >> Laszlo Ersek <lersek@redhat.com> writes: >> > --- /dev/null >> > +++ b/hw/i386/acpi.h >> > + >> > +#include <stddef.h> >> >> QEMU style would normally be to use qemu-common.h here but honestly I >> prefer using system headers when it's possible. Just FYI. > > I thought we were actively trying to stop including qemu-common.h from > other header files, because it easily leads to unexpected (and hard to > fix) circular header dependencies. The problem is qemu-common including other headers, not other headers including qemu-common... But like I said in my original note, I don't like using qemu-common in headers anyway. Regards, Anthony Liguori > > -- > Eduardo
On Thu, Apr 25, 2013 at 03:45:04PM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Apr 25, 2013 at 02:03:05PM -0500, Anthony Liguori wrote: > >> Laszlo Ersek <lersek@redhat.com> writes: > >> > --- /dev/null > >> > +++ b/hw/i386/acpi.h > >> > + > >> > +#include <stddef.h> > >> > >> QEMU style would normally be to use qemu-common.h here but honestly I > >> prefer using system headers when it's possible. Just FYI. > > > > I thought we were actively trying to stop including qemu-common.h from > > other header files, because it easily leads to unexpected (and hard to > > fix) circular header dependencies. > > The problem is qemu-common including other headers, not other headers > including qemu-common... Well, it depends on what's the stated purpose/rules of qemu-common.h. If its purpose if to allow .c files to have many commonly-used definitions available without including commonly-used header files one by one, qemu-common.h will inevitably include other QEMU header files. > > But like I said in my original note, I don't like using qemu-common in > headers anyway. Agreed on this specific case. But I would really like to clarify the purpose of qemu-common.h, because I always believed that it was supposed to be a "includes lots of other stuff" header, not a "is included by lots of other stuff" header (and it can't be both). BTW, qemu-common.h already have a comment stating the following: """ This file is supposed to be included only by .c files. No header file should depend on qemu-common.h, as this would easily lead to circular header dependencies. """ If this is not true, we must correct it.
On Thu, Apr 25, 2013 at 05:57:41PM -0300, Eduardo Habkost wrote: > On Thu, Apr 25, 2013 at 03:45:04PM -0500, Anthony Liguori wrote: > > Eduardo Habkost <ehabkost@redhat.com> writes: > > > > > On Thu, Apr 25, 2013 at 02:03:05PM -0500, Anthony Liguori wrote: > > >> Laszlo Ersek <lersek@redhat.com> writes: > > >> > --- /dev/null > > >> > +++ b/hw/i386/acpi.h > > >> > + > > >> > +#include <stddef.h> > > >> > > >> QEMU style would normally be to use qemu-common.h here but honestly I > > >> prefer using system headers when it's possible. Just FYI. > > > > > > I thought we were actively trying to stop including qemu-common.h from > > > other header files, because it easily leads to unexpected (and hard to > > > fix) circular header dependencies. > > > > The problem is qemu-common including other headers, not other headers > > including qemu-common... > > Well, it depends on what's the stated purpose/rules of qemu-common.h. If > its purpose if to allow .c files to have many commonly-used definitions > available without including commonly-used header files one by one, > qemu-common.h will inevitably include other QEMU header files. > > > > > But like I said in my original note, I don't like using qemu-common in > > headers anyway. > > Agreed on this specific case. But I would really like to clarify the > purpose of qemu-common.h, because I always believed that it was supposed > to be a "includes lots of other stuff" header, not a "is included by > lots of other stuff" header (and it can't be both). > > > BTW, qemu-common.h already have a comment stating the following: > > """ > This file is supposed to be included only by .c files. No header file should > depend on qemu-common.h, as this would easily lead to circular header > dependencies. > """ > > If this is not true, we must correct it. I think most of these should be switched to include qemu/typedefs.h. qemu-common used to have all typedefs a while ago. > -- > Eduardo
On 04/25/13 21:03, Anthony Liguori wrote: > Laszlo Ersek <lersek@redhat.com> writes: > >> This patch reuses some code from SeaBIOS, which was originally under >> LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This >> relicensing has been acked by all contributors that had contributed to the >> code since the v2->v3 relicense. ACKs approving the v2+ relicensing are >> listed below. The list might include ACKs from people not holding >> copyright on any parts of the reused code, but it's better to err on the >> side of caution and include them. >> >> Affected SeaBIOS files (GPLv2+ license headers added) >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>: >> >> src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-isa.dsl | 15 +++++++++++++++ >> src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++ >> src/acpi.c | 14 +++++++++++++- >> src/acpi.h | 14 ++++++++++++++ >> src/ssdt-misc.dsl | 15 +++++++++++++++ >> src/ssdt-pcihp.dsl | 15 +++++++++++++++ >> src/ssdt-proc.dsl | 15 +++++++++++++++ >> tools/acpi_extract.py | 13 ++++++++++++- >> tools/acpi_extract_preprocess.py | 13 ++++++++++++- >> 12 files changed, 171 insertions(+), 3 deletions(-) >> >> Each one of the listed people agreed to the following: >> >>> If you allow the use of your contribution in QEMU under the >>> terms of GPLv2 or later as proposed by this patch, >>> please respond to this mail including the line: >>> >>> Acked-by: Name <email address> >> >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> Acked-by: Jan Kiszka <jan.kiszka@siemens.com> >> Acked-by: Jason Baron <jbaron@akamai.com> >> Acked-by: David Woodhouse <David.Woodhouse@intel.com> >> Acked-by: Gleb Natapov <gleb@redhat.com> >> Acked-by: Marcelo Tosatti <mtosatti@redhat.com> >> Acked-by: Dave Frodin <dave.frodin@se-eng.com> >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >> Acked-by: Kevin O'Connor <kevin@koconnor.net> >> Acked-by: Laszlo Ersek <lersek@redhat.com> >> Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> >> Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> >> Acked-by: Magnus Christensson <magnus.christensson@intel.com> >> Acked-by: Hu Tao <hutao@cn.fujitsu.com> >> Acked-by: Eduardo Habkost <ehabkost@redhat.com> >> >> The patch incorporates ideas/suggestions from Michael Tsirkin's prototype >> code: >> - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with >> i386-specific ACPI table stuff, >> - separate preparation of individual tables from their installation as >> fw_cfg files, >> - install these fw_cfg files inside pc_memory_init(), which is shared by >> piix4/q35, >> - add the above licensing-related block to the commit message. >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> configure | 12 ++++ >> hw/i386/Makefile.objs | 1 + >> hw/i386/acpi.h | 9 +++ >> hw/i386/acpi.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/i386/pc.c | 23 +++++++ >> 5 files changed, 204 insertions(+), 0 deletions(-) >> create mode 100644 hw/i386/acpi.h >> create mode 100644 hw/i386/acpi.c >> >> diff --git a/configure b/configure >> index ed49f91..45a5f55 100755 >> --- a/configure >> +++ b/configure >> @@ -241,6 +241,7 @@ gtk="" >> gtkabi="2.0" >> tpm="no" >> libssh2="" >> +dynamic_acpi="no" >> >> # parse CC options first >> for opt do >> @@ -928,6 +929,10 @@ for opt do >> ;; >> --enable-libssh2) libssh2="yes" >> ;; >> + --disable-dynamic-acpi) dynamic_acpi="no" >> + ;; >> + --enable-dynamic-acpi) dynamic_acpi="yes" >> + ;; >> *) echo "ERROR: unknown option $opt"; show_help="yes" >> ;; >> esac >> @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" >> echo " --enable-tpm enable TPM support" >> echo " --disable-libssh2 disable ssh block device support" >> echo " --enable-libssh2 enable ssh block device support" >> +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" >> +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" >> echo "" >> echo "NOTE: The object files are built at the place where configure is launched" >> exit 1 >> @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" >> echo "TPM support $tpm" >> echo "libssh2 support $libssh2" >> echo "TPM passthrough $tpm_passthrough" >> +echo "dynamic ACPI tables $dynamic_acpi" >> >> if test "$sdl_too_old" = "yes"; then >> echo "-> Your SDL version is too old - please upgrade to have SDL support" >> @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then >> echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak >> fi >> >> +if test "$dynamic_acpi" = "yes"; then >> + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak >> +fi >> + >> # USB host support >> case "$usb" in >> linux) > > No reason for this to be a configure option. :-/ I added this from v3 to v4 because Michael asked me for it <http://thread.gmane.org/gmane.comp.emulators.qemu/206435/focus=207146>. From the SeaBIOS side, I believe Kevin O'Connor also wants to keep out related code from SeaBIOS until a full set of tables is passed. I disagree with flipping a big switch in the end (ie. I agree a config option (let alone a separate SeaBIOS branch) is unwarranted, which is why I didn't add the former in v3), but I have no say in it. Do you want me to rip out these hunks (and adapt the dependencies)? >> +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, >> + const char *sig) >> +{ >> + g_assert(blob_size >= sizeof *std_hdr); >> + >> + *std_hdr = acpi_dfl_hdr; >> + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); > > Should use () with sizeof and avoid strncpy. It almost never has the > behavior you want with respect to NULL termination (unless you > explicitly want to not NULL terminate when hitting bufsize). Correct, I explicitly wanted to avoid NUL-termination here. The destination ACPI fields are not NUL-terminated but fixed size. The caller specifies a C string. The bytes not covered by that string in the ACPI field, ie. coming from the default header, should be overwritten by NULs. >> +#ifdef CONFIG_DYN_ACPI >> +static void pc_acpi_madt(FWCfgState *fw_cfg) >> +{ >> + unsigned num_lapic; >> + char unsigned *blob; >> + size_t blob_size; >> + >> + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ >> + num_lapic = pc_apic_id_limit(max_cpus); >> + >> + acpi_build_madt(&blob, &blob_size, num_lapic); > > I'd be a lot happier if we were passing more information to this routine > and not hard coding it. For instance, the PCI interrupt assignments, > the APIC ids, the number of available CPUs, etc. I can try to extract all dependencies as parameters, but I think it will lead us down an unpleasant path. In my understanding the migration of ACPI tables from boot firmware(s) to qemu is *also* motivated that ACPI tables are very quirky ^W flexible, and passing down every bit of info to build them is (a) a chore, because there are many parameters erecting the whole bunch of tables, (b) the set of parameters is a constantly moving target, as tables are added or their ACPI versions are upgraded (eg. from ACPI 1.0 to ACPI 2.0). Hence my thinking about this went as the polar opposite of yours. In these ACPI preparation functions, where we're composing a "portable" description of the machine, we're fishing from a global pool of information. Nothing should be off limits. Just as well as I can call kvm_allows_irq0_override(), I should be able to access whatever global data and functions, without explicitly specifying in a function prototype what I need. I need *everything* -- that's (also) why we're doing the tables in QEMU. Because everything is accessible there without jumping through hoops. There's nothing regular in the kinds of information stored in various ACPI tables; they are arbitrary. A function prototype according to your suggestion would be justified by nothing more than "well, that's what's required for the MADT", and if we bump an ACPI version (maybe not for the MADT but for another table), we'll have to adapt the prototype and the caller too. (I already dislike the "num_lapic" parameter; IMO the MADT function should directly call x86_cpu_apic_id_from_index() with both max_cpus and smp_cpus, or *maybe* export and call pc_apic_id_limit() for the former.) Of course this is just my two cents. > The commit message also doesn't provide any reason about why we would > want this. The cover letter provides a reference at least but cover > letters don't end up in git history. Well I'll need help wording that. From my perspective there are two goals: - primary goal: stop OVMF chasing SeaBIOS's tail in ACPI features -- prepare the ACPI tables in QEMU and let whatever boot firmware use the same set of tables, - secondary goal: stop bumping into fw_cfg boundaries when needing new ACPI dependencies in boot firmware (see above). I believe that Michael is interested in the ACPI move because of a new chipset he's introducing (or some such, please excuse my ignorance). Thanks! Laszlo
On Fri, Apr 26, 2013 at 01:13:28PM +0200, Laszlo Ersek wrote: > On 04/25/13 21:03, Anthony Liguori wrote: > > Laszlo Ersek <lersek@redhat.com> writes: > > > >> This patch reuses some code from SeaBIOS, which was originally under > >> LGPLv2 and then relicensed to GPLv3 or LGPLv3, in QEMU under GPLv2+. This > >> relicensing has been acked by all contributors that had contributed to the > >> code since the v2->v3 relicense. ACKs approving the v2+ relicensing are > >> listed below. The list might include ACKs from people not holding > >> copyright on any parts of the reused code, but it's better to err on the > >> side of caution and include them. > >> > >> Affected SeaBIOS files (GPLv2+ license headers added) > >> <http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5949>: > >> > >> src/acpi-dsdt-cpu-hotplug.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-dbug.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-hpet.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-isa.dsl | 15 +++++++++++++++ > >> src/acpi-dsdt-pci-crs.dsl | 15 +++++++++++++++ > >> src/acpi.c | 14 +++++++++++++- > >> src/acpi.h | 14 ++++++++++++++ > >> src/ssdt-misc.dsl | 15 +++++++++++++++ > >> src/ssdt-pcihp.dsl | 15 +++++++++++++++ > >> src/ssdt-proc.dsl | 15 +++++++++++++++ > >> tools/acpi_extract.py | 13 ++++++++++++- > >> tools/acpi_extract_preprocess.py | 13 ++++++++++++- > >> 12 files changed, 171 insertions(+), 3 deletions(-) > >> > >> Each one of the listed people agreed to the following: > >> > >>> If you allow the use of your contribution in QEMU under the > >>> terms of GPLv2 or later as proposed by this patch, > >>> please respond to this mail including the line: > >>> > >>> Acked-by: Name <email address> > >> > >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> > >> Acked-by: Jan Kiszka <jan.kiszka@siemens.com> > >> Acked-by: Jason Baron <jbaron@akamai.com> > >> Acked-by: David Woodhouse <David.Woodhouse@intel.com> > >> Acked-by: Gleb Natapov <gleb@redhat.com> > >> Acked-by: Marcelo Tosatti <mtosatti@redhat.com> > >> Acked-by: Dave Frodin <dave.frodin@se-eng.com> > >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> > >> Acked-by: Kevin O'Connor <kevin@koconnor.net> > >> Acked-by: Laszlo Ersek <lersek@redhat.com> > >> Acked-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> > >> Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> > >> Acked-by: Magnus Christensson <magnus.christensson@intel.com> > >> Acked-by: Hu Tao <hutao@cn.fujitsu.com> > >> Acked-by: Eduardo Habkost <ehabkost@redhat.com> > >> > >> The patch incorporates ideas/suggestions from Michael Tsirkin's prototype > >> code: > >> - "hw/i386/pc.c" is too big, create new file "hw/i386/acpi.c" with > >> i386-specific ACPI table stuff, > >> - separate preparation of individual tables from their installation as > >> fw_cfg files, > >> - install these fw_cfg files inside pc_memory_init(), which is shared by > >> piix4/q35, > >> - add the above licensing-related block to the commit message. > >> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> configure | 12 ++++ > >> hw/i386/Makefile.objs | 1 + > >> hw/i386/acpi.h | 9 +++ > >> hw/i386/acpi.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> hw/i386/pc.c | 23 +++++++ > >> 5 files changed, 204 insertions(+), 0 deletions(-) > >> create mode 100644 hw/i386/acpi.h > >> create mode 100644 hw/i386/acpi.c > >> > >> diff --git a/configure b/configure > >> index ed49f91..45a5f55 100755 > >> --- a/configure > >> +++ b/configure > >> @@ -241,6 +241,7 @@ gtk="" > >> gtkabi="2.0" > >> tpm="no" > >> libssh2="" > >> +dynamic_acpi="no" > >> > >> # parse CC options first > >> for opt do > >> @@ -928,6 +929,10 @@ for opt do > >> ;; > >> --enable-libssh2) libssh2="yes" > >> ;; > >> + --disable-dynamic-acpi) dynamic_acpi="no" > >> + ;; > >> + --enable-dynamic-acpi) dynamic_acpi="yes" > >> + ;; > >> *) echo "ERROR: unknown option $opt"; show_help="yes" > >> ;; > >> esac > >> @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" > >> echo " --enable-tpm enable TPM support" > >> echo " --disable-libssh2 disable ssh block device support" > >> echo " --enable-libssh2 enable ssh block device support" > >> +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" > >> +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" > >> echo "" > >> echo "NOTE: The object files are built at the place where configure is launched" > >> exit 1 > >> @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" > >> echo "TPM support $tpm" > >> echo "libssh2 support $libssh2" > >> echo "TPM passthrough $tpm_passthrough" > >> +echo "dynamic ACPI tables $dynamic_acpi" > >> > >> if test "$sdl_too_old" = "yes"; then > >> echo "-> Your SDL version is too old - please upgrade to have SDL support" > >> @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then > >> echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak > >> fi > >> > >> +if test "$dynamic_acpi" = "yes"; then > >> + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak > >> +fi > >> + > >> # USB host support > >> case "$usb" in > >> linux) > > > > No reason for this to be a configure option. > > :-/ > > I added this from v3 to v4 because Michael asked me for it > <http://thread.gmane.org/gmane.comp.emulators.qemu/206435/focus=207146>. > > >From the SeaBIOS side, I believe Kevin O'Connor also wants to keep out > related code from SeaBIOS until a full set of tables is passed. I > disagree with flipping a big switch in the end (ie. I agree a config > option (let alone a separate SeaBIOS branch) is unwarranted, which is > why I didn't add the former in v3), but I have no say in it. > > Do you want me to rip out these hunks (and adapt the dependencies)? Essentially, what seabios wants to do is operate in two modes: - (mostly) use builtin acpi tables - use acpi tables from hypervisor in particular, seabios wants to interpret presence of any file in etc/acpi as a signal not to generate its own tables. So merging this patch but without the config option will break this plan. The only two ways I see are: - merge this last patch with the config option, disabled by default the idea being we can improve it in-tree, gradually. - keep this patch out of tree until we have a complete set of tables. Both are fine with me. > >> +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, > >> + const char *sig) > >> +{ > >> + g_assert(blob_size >= sizeof *std_hdr); > >> + > >> + *std_hdr = acpi_dfl_hdr; > >> + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); > > > > Should use () with sizeof and avoid strncpy. It almost never has the > > behavior you want with respect to NULL termination (unless you > > explicitly want to not NULL terminate when hitting bufsize). > > Correct, I explicitly wanted to avoid NUL-termination here. The > destination ACPI fields are not NUL-terminated but fixed size. The > caller specifies a C string. The bytes not covered by that string in the > ACPI field, ie. coming from the default header, should be overwritten by > NULs. > > > >> +#ifdef CONFIG_DYN_ACPI > >> +static void pc_acpi_madt(FWCfgState *fw_cfg) > >> +{ > >> + unsigned num_lapic; > >> + char unsigned *blob; > >> + size_t blob_size; > >> + > >> + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ > >> + num_lapic = pc_apic_id_limit(max_cpus); > >> + > >> + acpi_build_madt(&blob, &blob_size, num_lapic); > > > > I'd be a lot happier if we were passing more information to this routine > > and not hard coding it. For instance, the PCI interrupt assignments, > > the APIC ids, the number of available CPUs, etc. > > I can try to extract all dependencies as parameters, but I think it will > lead us down an unpleasant path. In my understanding the migration of > ACPI tables from boot firmware(s) to qemu is *also* motivated that ACPI > tables are very quirky ^W flexible, and passing down every bit of info > to build them is (a) a chore, because there are many parameters erecting > the whole bunch of tables, (b) the set of parameters is a constantly > moving target, as tables are added or their ACPI versions are upgraded > (eg. from ACPI 1.0 to ACPI 2.0). > > Hence my thinking about this went as the polar opposite of yours. In > these ACPI preparation functions, where we're composing a "portable" > description of the machine, we're fishing from a global pool of > information. Nothing should be off limits. Just as well as I can call > kvm_allows_irq0_override(), I should be able to access whatever global > data and functions, without explicitly specifying in a function > prototype what I need. I need *everything* -- that's (also) why we're > doing the tables in QEMU. Because everything is accessible there without > jumping through hoops. > > There's nothing regular in the kinds of information stored in various > ACPI tables; they are arbitrary. A function prototype according to your > suggestion would be justified by nothing more than "well, that's what's > required for the MADT", and if we bump an ACPI version (maybe not for > the MADT but for another table), we'll have to adapt the prototype and > the caller too. > > (I already dislike the "num_lapic" parameter; IMO the MADT function > should directly call x86_cpu_apic_id_from_index() with both max_cpus and > smp_cpus, or *maybe* export and call pc_apic_id_limit() for the former.) > > Of course this is just my two cents. > > > The commit message also doesn't provide any reason about why we would > > want this. The cover letter provides a reference at least but cover > > letters don't end up in git history. > > Well I'll need help wording that. From my perspective there are two goals: > - primary goal: stop OVMF chasing SeaBIOS's tail in ACPI features -- > prepare the ACPI tables in QEMU and let whatever boot firmware use the > same set of tables, > - secondary goal: stop bumping into fw_cfg boundaries when needing new > ACPI dependencies in boot firmware (see above). > > I believe that Michael is interested in the ACPI move because of a new > chipset he's introducing (or some such, please excuse my ignorance). > > Thanks! > Laszlo Yes, and hotplug for PCI bridge as well.
On Mon, Apr 29, 2013 at 11:20:15AM +0300, Michael S. Tsirkin wrote: > On Fri, Apr 26, 2013 at 01:13:28PM +0200, Laszlo Ersek wrote: > > > > I added this from v3 to v4 because Michael asked me for it > > <http://thread.gmane.org/gmane.comp.emulators.qemu/206435/focus=207146>. > > > > >From the SeaBIOS side, I believe Kevin O'Connor also wants to keep out > > related code from SeaBIOS until a full set of tables is passed. I > > disagree with flipping a big switch in the end (ie. I agree a config > > option (let alone a separate SeaBIOS branch) is unwarranted, which is > > why I didn't add the former in v3), but I have no say in it. > > > > Do you want me to rip out these hunks (and adapt the dependencies)? > > Essentially, what seabios wants to do is operate in two > modes: > - (mostly) use builtin acpi tables > - use acpi tables from hypervisor > > in particular, seabios wants to interpret presence > of any file in etc/acpi as a signal not to generate > its own tables. Right. > So merging this patch but without the config option will break > this plan. The only two ways I see are: > - merge this last patch with the config option, disabled by default > the idea being we can improve it in-tree, gradually. > - keep this patch out of tree until we have a complete > set of tables. > > Both are fine with me. Why? As long as QEMU places the new tables under new fwcfg entries, old seabios will totally ignore the new tables. I don't see why a QEMU config option is needed - it's safe for QEMU to always create both old and new fwcfg entries. -Kevin
On Mon, Apr 29, 2013 at 08:39:26AM -0400, Kevin O'Connor wrote: > On Mon, Apr 29, 2013 at 11:20:15AM +0300, Michael S. Tsirkin wrote: > > On Fri, Apr 26, 2013 at 01:13:28PM +0200, Laszlo Ersek wrote: > > > > > > I added this from v3 to v4 because Michael asked me for it > > > <http://thread.gmane.org/gmane.comp.emulators.qemu/206435/focus=207146>. > > > > > > >From the SeaBIOS side, I believe Kevin O'Connor also wants to keep out > > > related code from SeaBIOS until a full set of tables is passed. I > > > disagree with flipping a big switch in the end (ie. I agree a config > > > option (let alone a separate SeaBIOS branch) is unwarranted, which is > > > why I didn't add the former in v3), but I have no say in it. > > > > > > Do you want me to rip out these hunks (and adapt the dependencies)? > > > > Essentially, what seabios wants to do is operate in two > > modes: > > - (mostly) use builtin acpi tables > > - use acpi tables from hypervisor > > > > in particular, seabios wants to interpret presence > > of any file in etc/acpi as a signal not to generate > > its own tables. > > Right. > > > So merging this patch but without the config option will break > > this plan. The only two ways I see are: > > - merge this last patch with the config option, disabled by default > > the idea being we can improve it in-tree, gradually. > > - keep this patch out of tree until we have a complete > > set of tables. > > > > Both are fine with me. > > Why? As long as QEMU places the new tables under new fwcfg entries, > old seabios will totally ignore the new tables. I don't see why a > QEMU config option is needed - it's safe for QEMU to always create > both old and new fwcfg entries. > > -Kevin Yes backwards compatibility is fine, but the problem here is forwards compatibility. Future BIOS will say: "there's something in etc/acpi/ therefore I won't generate any tables" so we should not release QEMU that puts only MADT under etc/acpi/madt.
On 04/29/13 14:39, Kevin O'Connor wrote: > On Mon, Apr 29, 2013 at 11:20:15AM +0300, Michael S. Tsirkin wrote: >> in particular, seabios wants to interpret presence >> of any file in etc/acpi as a signal not to generate >> its own tables. > > Right. In that case, >> So merging this patch but without the config option will break >> this plan. The only two ways I see are: >> - merge this last patch with the config option, disabled by default >> the idea being we can improve it in-tree, gradually. >> - keep this patch out of tree until we have a complete >> set of tables. >> >> Both are fine with me. > > Why? As long as QEMU places the new tables under new fwcfg entries, > old seabios will totally ignore the new tables. I don't see why a > QEMU config option is needed - it's safe for QEMU to always create > both old and new fwcfg entries. the new-style fw_cfg entry for the MADT (= "etc/acpi/APIC") would prevent the generation of all other (yet unexported by qemu) ACPI tables in *new* seabios. [SeaBIOS] [PATCH RFC 2/3] acpi: load and link tables from /etc/acpi/ <http://thread.gmane.org/gmane.comp.emulators.qemu/208457/focus=208458> Thanks, Laszlo
diff --git a/configure b/configure index ed49f91..45a5f55 100755 --- a/configure +++ b/configure @@ -241,6 +241,7 @@ gtk="" gtkabi="2.0" tpm="no" libssh2="" +dynamic_acpi="no" # parse CC options first for opt do @@ -928,6 +929,10 @@ for opt do ;; --enable-libssh2) libssh2="yes" ;; + --disable-dynamic-acpi) dynamic_acpi="no" + ;; + --enable-dynamic-acpi) dynamic_acpi="yes" + ;; *) echo "ERROR: unknown option $opt"; show_help="yes" ;; esac @@ -1195,6 +1200,8 @@ echo " --gcov=GCOV use specified gcov [$gcov_tool]" echo " --enable-tpm enable TPM support" echo " --disable-libssh2 disable ssh block device support" echo " --enable-libssh2 enable ssh block device support" +echo " --disable-dynamic-acpi disable dynamic ACPI table generation (default)" +echo " --enable-dynamic-acpi enable dynamic ACPI table generation (work in progress)" echo "" echo "NOTE: The object files are built at the place where configure is launched" exit 1 @@ -3573,6 +3580,7 @@ echo "gcov enabled $gcov" echo "TPM support $tpm" echo "libssh2 support $libssh2" echo "TPM passthrough $tpm_passthrough" +echo "dynamic ACPI tables $dynamic_acpi" if test "$sdl_too_old" = "yes"; then echo "-> Your SDL version is too old - please upgrade to have SDL support" @@ -3958,6 +3966,10 @@ if test "$virtio_blk_data_plane" = "yes" ; then echo 'CONFIG_VIRTIO_BLK_DATA_PLANE=$(CONFIG_VIRTIO)' >> $config_host_mak fi +if test "$dynamic_acpi" = "yes"; then + echo "CONFIG_DYN_ACPI=y" >> $config_host_mak +fi + # USB host support case "$usb" in linux) diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs index 205d22e..8429d52 100644 --- a/hw/i386/Makefile.objs +++ b/hw/i386/Makefile.objs @@ -1,6 +1,7 @@ obj-$(CONFIG_KVM) += kvm/ obj-y += multiboot.o smbios.o obj-y += pc.o pc_piix.o pc_q35.o +obj-$(CONFIG_DYN_ACPI) += acpi.o obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o obj-y += kvmvapic.o diff --git a/hw/i386/acpi.h b/hw/i386/acpi.h new file mode 100644 index 0000000..94f9ad3 --- /dev/null +++ b/hw/i386/acpi.h @@ -0,0 +1,9 @@ +#ifndef QEMU_HW_I386_ACPI_H +#define QEMU_HW_I386_ACPI_H + +#include <stddef.h> + +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, + unsigned num_lapic); + +#endif diff --git a/hw/i386/acpi.c b/hw/i386/acpi.c new file mode 100644 index 0000000..179cdf5 --- /dev/null +++ b/hw/i386/acpi.c @@ -0,0 +1,159 @@ +/* + * Copyright (c) 2013 Red Hat Inc. + * Copyright (C) 2008-2010 Kevin O'Connor <kevin@koconnor.net> + * Copyright (C) 2006 Fabrice Bellard + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, see <http://www.gnu.org/licenses/>. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "hw/acpi/acpi.h" +#include "hw/i386/acpi.h" +#include "kvm_i386.h" +#include "sysemu/sysemu.h" + +static void acpi_table_fill_hdr(AcpiTableStdHdr *std_hdr, size_t blob_size, + const char *sig) +{ + g_assert(blob_size >= sizeof *std_hdr); + + *std_hdr = acpi_dfl_hdr; + strncpy(std_hdr->sig, sig, sizeof std_hdr->sig); + strncpy(std_hdr->oem_id, "QEMU ", sizeof std_hdr->oem_id); + strncpy(std_hdr->oem_table_id + 4, sig, sizeof std_hdr->oem_table_id - 4); + std_hdr->length = cpu_to_le32(blob_size); + std_hdr->checksum = acpi_checksum((uint8_t *)std_hdr, blob_size); +} + +void acpi_build_madt(unsigned char **out_blob, size_t *out_blob_size, + unsigned num_lapic) +{ + typedef struct { + uint8_t type; + uint8_t length; + } QEMU_PACKED AcpiSubHdr; + + AcpiTableStdHdr *std_hdr; + struct { + uint32_t lapic_addr; /* Local Interrupt Controller Address */ + uint32_t flags; /* Multiple APIC flags */ + } QEMU_PACKED *madt; + struct { + AcpiSubHdr hdr; + uint8_t processor_id; /* ACPI Processor ID */ + uint8_t apic_id; /* APIC ID */ + uint32_t flags; /* LOcal APIC flags */ + } QEMU_PACKED *lapic; + struct { + AcpiSubHdr hdr; + uint8_t io_apic_id; /* The I/O APIC's ID */ + uint8_t reserved; /* constant zero */ + uint32_t io_apic_addr; /* 32-bit physical address to access */ + uint32_t gsi_base; /* interrupt inputs start here */ + } QEMU_PACKED *io_apic; + struct { + AcpiSubHdr hdr; + uint8_t bus; /* constant zero: ISA */ + uint8_t source; /* this bus-relative interrupt source... */ + uint32_t gsi; /* ... will signal this global system interrupt */ + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ + } QEMU_PACKED *int_src_ovr; + struct { + AcpiSubHdr hdr; + uint8_t processor_id; /* ACPI Processor ID */ + uint16_t flags; /* MPS INTI Flags: Polarity, Trigger Mode */ + uint8_t lint; /* LAPIC interrupt input for NMI */ + } QEMU_PACKED *lapic_nmi; + + static const uint8_t pci_isa_irq[] = { 5, 9, 10, 11 }; + + unsigned num_int_src_ovr, i; + size_t blob_size; + char unsigned *blob; + + num_int_src_ovr = sizeof pci_isa_irq + kvm_allows_irq0_override(); + + blob_size = (sizeof *std_hdr) * 1 + + (sizeof *madt) * 1 + + (sizeof *lapic) * num_lapic + + (sizeof *io_apic) * 1 + + (sizeof *int_src_ovr) * num_int_src_ovr + + (sizeof *lapic_nmi) * 1; + blob = g_malloc(blob_size); + + std_hdr = (void *)blob; + madt = (void *)(std_hdr + 1 ); + lapic = (void *)(madt + 1 ); + io_apic = (void *)(lapic + num_lapic ); + int_src_ovr = (void *)(io_apic + 1 ); + lapic_nmi = (void *)(int_src_ovr + num_int_src_ovr); + + madt->lapic_addr = cpu_to_le32(APIC_DEFAULT_ADDRESS); + madt->flags = cpu_to_le32(1); /* PCAT_COMPAT */ + + /* create a Local APIC structure for each possible APIC ID */ + for (i = 0; i < num_lapic; ++i) { + lapic[i].hdr.type = 0; /* Processor Local APIC */ + lapic[i].hdr.length = sizeof *lapic; + lapic[i].processor_id = i; + lapic[i].apic_id = i; + lapic[i].flags = cpu_to_le32(0); /* disabled */ + } + /* enable the CPUs with a CPU index in the [0..smp_cpus-1] range */ + for (i = 0; i < smp_cpus; ++i) { + lapic[x86_cpu_apic_id_from_index(i)].flags = cpu_to_le32(1); + } + + io_apic->hdr.type = 1; /* I/O APIC */ + io_apic->hdr.length = sizeof *io_apic; + io_apic->io_apic_id = 0; + io_apic->reserved = 0; + io_apic->io_apic_addr = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS); + io_apic->gsi_base = cpu_to_le32(0); + + for (i = 0; i < sizeof pci_isa_irq; ++i) { + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; + int_src_ovr[i].bus = 0; + int_src_ovr[i].source = pci_isa_irq[i]; + int_src_ovr[i].gsi = cpu_to_le32(pci_isa_irq[i]); + int_src_ovr[i].flags = cpu_to_le16(0xd); + /* active high, level-triggered */ + } + if (kvm_allows_irq0_override()) { + int_src_ovr[i].hdr.type = 2; /* Interrupt Source Override */ + int_src_ovr[i].hdr.length = sizeof *int_src_ovr; + int_src_ovr[i].bus = 0; + int_src_ovr[i].source = 0; + int_src_ovr[i].gsi = cpu_to_le32(2); + int_src_ovr[i].flags = cpu_to_le16(0); /* conforms to bus spec */ + } + + lapic_nmi->hdr.type = 4; /* Local APIC NMI */ + lapic_nmi->hdr.length = sizeof *lapic_nmi; + lapic_nmi->processor_id = 0xff; /* all processors */ + lapic_nmi->flags = cpu_to_le16(0); /* conforms to bus spec */ + lapic_nmi->lint = 1; /* NMI connected to LAPIC input LINT1 */ + + acpi_table_fill_hdr(std_hdr, blob_size, "APIC"); + *out_blob = blob; + *out_blob_size = blob_size; +} diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 8727489..15c6284 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -54,6 +54,10 @@ #include "qemu/config-file.h" #include "hw/acpi/acpi.h" +#ifdef CONFIG_DYN_ACPI +# include "hw/i386/acpi.h" +#endif + /* debug PC/ISA interrupts */ //#define DEBUG_IRQ @@ -922,6 +926,21 @@ void pc_acpi_init(const char *default_dsdt) } } +#ifdef CONFIG_DYN_ACPI +static void pc_acpi_madt(FWCfgState *fw_cfg) +{ + unsigned num_lapic; + char unsigned *blob; + size_t blob_size; + + /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */ + num_lapic = pc_apic_id_limit(max_cpus); + + acpi_build_madt(&blob, &blob_size, num_lapic); + fw_cfg_add_file(fw_cfg, "etc/acpi/APIC", blob, blob_size); +} +#endif + FWCfgState *pc_memory_init(MemoryRegion *system_memory, const char *kernel_filename, const char *kernel_cmdline, @@ -974,6 +993,10 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory, fw_cfg = bochs_bios_init(); rom_set_fw(fw_cfg); +#ifdef CONFIG_DYN_ACPI + pc_acpi_madt(fw_cfg); +#endif + if (linux_boot) { load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size); }