* [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg
@ 2013-04-18 20:22 Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly Laszlo Ersek
` (7 more replies)
0 siblings, 8 replies; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
v4:
- patches 1-6 are unchanged and carry Michael's ACK,
- patch 7 is rendered dependent on a new configure switch (default off)
[Michael Tsirkin].
v3:
- rebased to current master 24a6e7f4,
- added Michael's S-o-b to 6/7 [Eric Blake],
- added Dave Frodin's relicensing ACK to 7/7, originally sent to Michael
in private [Michael Tsirkin],
- slightly reworded 7/7's commit message where it credits Michael's
prototype [Eric Blake].
v2:
- address (1) in
<http://thread.gmane.org/gmane.comp.emulators.qemu/206146/focus=206195>:
- rebase to Paolo's recent series
<http://thread.gmane.org/gmane.comp.emulators.qemu/206196/focus=206278>,
- patch 2 is new, fixes style of function parameter references in a
comment [mst],
- patch 6 is new, moves a macro definition [mst],
- patch 7 should be structured more logically now, plus the commit
message has grown some bits related to licensing [mst].
acpi_table_fill_hdr() is kept distinct from the -acpitable switch's
implementation on purpose, similarly to pc_acpi_install() in v1.
- Tested APIC / DSDT / RSDT against contents saved from v1.
v1 blurb:
This series exports the MADT (APIC) ACPI table under the new
"etc/acpi/APIC" fw_cfg file. I sought to follow the requirements set
forth in [1], the new table is only visible in the patched/patched
case. I cross-tested { master, patched } qemu with { master, patched }
seabios (the APIC, DSDT and RSDT tables) using guest acpidump and
dmesg.
The -acpitable command line option is purposely ignored based on the
last paragraph of [2]; the user isn't supposed to pass APIC with that
option.
checkpatch.pl complains a little but (as last time) it's a false
alarm.
The series is bisectable.
[1] http://thread.gmane.org/gmane.comp.emulators.qemu/202005/focus=202072
[2] http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5960/focus=6008
Laszlo Ersek (6):
refer to FWCfgState explicitly
acpi_table_install(): fix funcparam formatting in leading comment
hw/acpi: extract standard table headers as a standalone structure
hw/acpi: export default ACPI headers using the type just introduced
hw/acpi: export acpi_checksum()
hw/i386: build ACPI MADT (APIC) for fw_cfg clients
Michael S. Tsirkin (1):
hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h
configure | 12 ++++
hw/i386/Makefile.objs | 1 +
hw/i386/acpi.h | 9 +++
hw/i386/multiboot.h | 4 +-
include/hw/acpi/acpi.h | 15 +++++
include/hw/i386/apic.h | 2 +
include/hw/i386/pc.h | 19 +++---
include/hw/loader.h | 3 +-
hw/acpi/core.c | 91 ++++++++++++++-------------
hw/acpi/piix4.c | 2 +-
hw/core/loader.c | 2 +-
hw/i386/acpi.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++
hw/i386/multiboot.c | 2 +-
hw/i386/pc.c | 49 +++++++++++----
hw/i386/pc_piix.c | 2 +-
hw/sparc/sun4m.c | 6 +-
hw/sparc64/sun4u.c | 2 +-
17 files changed, 303 insertions(+), 77 deletions(-)
create mode 100644 hw/i386/acpi.h
create mode 100644 hw/i386/acpi.c
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-25 18:44 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 2/7] acpi_table_install(): fix funcparam formatting in leading comment Laszlo Ersek
` (6 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
Currently some places use pointer-to-void even though they mean
pointer-to-FWCfgState. Clean them up.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/i386/multiboot.h | 4 +++-
include/hw/i386/pc.h | 19 ++++++++++---------
include/hw/loader.h | 3 ++-
hw/acpi/piix4.c | 2 +-
hw/core/loader.c | 2 +-
hw/i386/multiboot.c | 2 +-
hw/i386/pc.c | 24 ++++++++++++------------
hw/i386/pc_piix.c | 2 +-
hw/sparc/sun4m.c | 6 +++---
hw/sparc64/sun4u.c | 2 +-
10 files changed, 35 insertions(+), 31 deletions(-)
diff --git a/hw/i386/multiboot.h b/hw/i386/multiboot.h
index 98fb1b7..60de309 100644
--- a/hw/i386/multiboot.h
+++ b/hw/i386/multiboot.h
@@ -1,7 +1,9 @@
#ifndef QEMU_MULTIBOOT_H
#define QEMU_MULTIBOOT_H
-int load_multiboot(void *fw_cfg,
+#include "hw/nvram/fw_cfg.h"
+
+int load_multiboot(FWCfgState *fw_cfg,
FILE *f,
const char *kernel_filename,
const char *initrd_filename,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9bcc819..fa4b5ce 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -9,6 +9,7 @@
#include "net/net.h"
#include "exec/memory.h"
#include "hw/i386/ioapic.h"
+#include "hw/nvram/fw_cfg.h"
/* PC-style peripherals (also used by other machines). */
@@ -80,14 +81,14 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
void pc_cpus_init(const char *cpu_model);
void pc_acpi_init(const char *default_dsdt);
-void *pc_memory_init(MemoryRegion *system_memory,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- ram_addr_t below_4g_mem_size,
- ram_addr_t above_4g_mem_size,
- MemoryRegion *rom_memory,
- MemoryRegion **ram_memory);
+FWCfgState *pc_memory_init(MemoryRegion *system_memory,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ ram_addr_t below_4g_mem_size,
+ ram_addr_t above_4g_mem_size,
+ MemoryRegion *rom_memory,
+ MemoryRegion **ram_memory);
qemu_irq *pc_allocate_cpu_irq(void);
DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
@@ -111,7 +112,7 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
qemu_irq sci_irq, qemu_irq smi_irq,
- int kvm_enabled, void *fw_cfg);
+ int kvm_enabled, FWCfgState *fw_cfg);
void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
/* hpet.c */
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 0958f06..15d4cc9 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -1,6 +1,7 @@
#ifndef LOADER_H
#define LOADER_H
#include "qapi/qmp/qdict.h"
+#include "hw/nvram/fw_cfg.h"
/* loader.c */
int get_image_size(const char *filename);
@@ -30,7 +31,7 @@ int rom_add_blob(const char *name, const void *blob, size_t len,
int rom_add_elf_program(const char *name, void *data, size_t datasize,
size_t romsize, hwaddr addr);
int rom_load_all(void);
-void rom_set_fw(void *f);
+void rom_set_fw(FWCfgState *f);
int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
void *rom_ptr(hwaddr addr);
void do_info_roms(Monitor *mon, const QDict *qdict);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 88386d7..200c297 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -435,7 +435,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
qemu_irq sci_irq, qemu_irq smi_irq,
- int kvm_enabled, void *fw_cfg)
+ int kvm_enabled, FWCfgState *fw_cfg)
{
PCIDevice *dev;
PIIX4PMState *s;
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 7507914..a711145 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -733,7 +733,7 @@ int rom_load_all(void)
return 0;
}
-void rom_set_fw(void *f)
+void rom_set_fw(FWCfgState *f)
{
fw_cfg = f;
}
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index d696507..09211e0 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -124,7 +124,7 @@ static void mb_add_mod(MultibootState *s,
s->mb_mods_count++;
}
-int load_multiboot(void *fw_cfg,
+int load_multiboot(FWCfgState *fw_cfg,
FILE *f,
const char *kernel_filename,
const char *initrd_filename,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0d6e72b..3c12b55 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -569,9 +569,9 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
}
-static void *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(void)
{
- void *fw_cfg;
+ FWCfgState *fw_cfg;
uint8_t *smbios_table;
size_t smbios_len;
uint64_t *numa_fw_cfg;
@@ -648,7 +648,7 @@ static long get_file_size(FILE *f)
return size;
}
-static void load_linux(void *fw_cfg,
+static void load_linux(FWCfgState *fw_cfg,
const char *kernel_filename,
const char *initrd_filename,
const char *kernel_cmdline,
@@ -924,19 +924,19 @@ void pc_acpi_init(const char *default_dsdt)
}
}
-void *pc_memory_init(MemoryRegion *system_memory,
- const char *kernel_filename,
- const char *kernel_cmdline,
- const char *initrd_filename,
- ram_addr_t below_4g_mem_size,
- ram_addr_t above_4g_mem_size,
- MemoryRegion *rom_memory,
- MemoryRegion **ram_memory)
+FWCfgState *pc_memory_init(MemoryRegion *system_memory,
+ const char *kernel_filename,
+ const char *kernel_cmdline,
+ const char *initrd_filename,
+ ram_addr_t below_4g_mem_size,
+ ram_addr_t above_4g_mem_size,
+ MemoryRegion *rom_memory,
+ MemoryRegion **ram_memory)
{
int linux_boot, i;
MemoryRegion *ram, *option_rom_mr;
MemoryRegion *ram_below_4g, *ram_above_4g;
- void *fw_cfg;
+ FWCfgState *fw_cfg;
linux_boot = (kernel_filename != NULL);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 943758a..da10e6d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -85,7 +85,7 @@ static void pc_init1(MemoryRegion *system_memory,
MemoryRegion *ram_memory;
MemoryRegion *pci_memory;
MemoryRegion *rom_memory;
- void *fw_cfg = NULL;
+ FWCfgState *fw_cfg = NULL;
pc_cpus_init(cpu_model);
pc_acpi_init("acpi-dsdt.aml");
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 31beb32..0b1b620 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -874,7 +874,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
qemu_irq *cpu_halt;
unsigned long kernel_size;
DriveInfo *fd[MAX_FD];
- void *fw_cfg;
+ FWCfgState *fw_cfg;
unsigned int num_vsimms;
/* init CPUs */
@@ -1592,7 +1592,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
espdma_irq, ledma_irq;
qemu_irq esp_reset, dma_enable;
unsigned long kernel_size;
- void *fw_cfg;
+ FWCfgState *fw_cfg;
DeviceState *dev;
/* init CPUs */
@@ -1793,7 +1793,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
qemu_irq fdc_tc;
unsigned long kernel_size;
DriveInfo *fd[MAX_FD];
- void *fw_cfg;
+ FWCfgState *fw_cfg;
DeviceState *dev;
unsigned int i;
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0d29620..0138252 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -818,7 +818,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
qemu_irq *ivec_irqs, *pbm_irqs;
DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
DriveInfo *fd[MAX_FD];
- void *fw_cfg;
+ FWCfgState *fw_cfg;
/* init CPUs */
cpu = cpu_devinit(cpu_model, hwdef);
--
1.7.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 2/7] acpi_table_install(): fix funcparam formatting in leading comment
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-25 18:44 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure Laszlo Ersek
` (5 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/acpi/core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 64b8718..69cadb0 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -86,7 +86,7 @@ static int acpi_checksum(const uint8_t *data, int len)
* is optionally overwritten from @hdrs.
*
* It is valid to call this function with
- * (@blob == NULL && bloblen == 0 && !has_header).
+ * (@blob == NULL && @bloblen == 0 && !@has_header).
*
* @hdrs->file and @hdrs->data are ignored.
*
--
1.7.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 2/7] acpi_table_install(): fix funcparam formatting in leading comment Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-25 18:47 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced Laszlo Ersek
` (4 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
This enables reuse when preparing per-table fw_cfg blobs later.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/acpi.h | 11 +++++++++++
hw/acpi/core.c | 48 +++++++++++++++++++++---------------------------
2 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 635be7b..0e26f63 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -167,4 +167,15 @@ extern size_t acpi_tables_len;
void acpi_table_add(const QemuOpts *opts, Error **errp);
+typedef struct acpi_table_std_header {
+ char sig[4]; /* ACPI signature (4 ASCII characters) */
+ uint32_t length; /* Length of table, in bytes, including header */
+ uint8_t revision; /* ACPI Specification minor version # */
+ uint8_t checksum; /* To make sum of entire table == 0 */
+ char oem_id[6]; /* OEM identification */
+ char oem_table_id[8]; /* OEM table identification */
+ uint32_t oem_revision; /* OEM revision number */
+ char asl_compiler_id[4]; /* ASL compiler vendor ID */
+ uint32_t asl_compiler_revision; /* ASL compiler revision number */
+} QEMU_PACKED AcpiTableStdHdr;
#endif /* !QEMU_HW_ACPI_H */
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 69cadb0..d348f81 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -31,21 +31,13 @@
struct acpi_table_header {
uint16_t _length; /* our length, not actual part of the hdr */
/* allows easier parsing for fw_cfg clients */
- char sig[4]; /* ACPI signature (4 ASCII characters) */
- uint32_t length; /* Length of table, in bytes, including header */
- uint8_t revision; /* ACPI Specification minor version # */
- uint8_t checksum; /* To make sum of entire table == 0 */
- char oem_id[6]; /* OEM identification */
- char oem_table_id[8]; /* OEM table identification */
- uint32_t oem_revision; /* OEM revision number */
- char asl_compiler_id[4]; /* ASL compiler vendor ID */
- uint32_t asl_compiler_revision; /* ASL compiler revision number */
+ AcpiTableStdHdr std;
} QEMU_PACKED;
-#define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
-#define ACPI_TABLE_PFX_SIZE sizeof(uint16_t) /* size of the extra prefix */
+/* size of the extra prefix */
+#define ACPI_TABLE_PFX_SIZE offsetof(struct acpi_table_header, std)
-static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE] =
+static const char unsigned dfl_hdr[sizeof(AcpiTableStdHdr)] =
"QEMU\0\0\0\0\1\0" /* sig (4), len(4), revno (1), csum (1) */
"QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
"QEMU\1\0\0\0" /* ASL compiler ID (4), version (4) */
@@ -105,6 +97,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
size_t body_size, acpi_payload_size;
struct acpi_table_header *ext_hdr;
unsigned changed_fields;
+ AcpiTableStdHdr *std;
/* Calculate where the ACPI table body starts within the blob, plus where
* to copy the ACPI table header from.
@@ -177,46 +170,47 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
changed_fields = 0;
ext_hdr->_length = cpu_to_le16(acpi_payload_size);
+ std = &ext_hdr->std;
if (hdrs->has_sig) {
- strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
+ strncpy(std->sig, hdrs->sig, sizeof std->sig);
++changed_fields;
}
- if (has_header && le32_to_cpu(ext_hdr->length) != acpi_payload_size) {
+ if (has_header && le32_to_cpu(std->length) != acpi_payload_size) {
fprintf(stderr,
"warning: ACPI table has wrong length, header says "
"%" PRIu32 ", actual size %zu bytes\n",
- le32_to_cpu(ext_hdr->length), acpi_payload_size);
+ le32_to_cpu(std->length), acpi_payload_size);
}
- ext_hdr->length = cpu_to_le32(acpi_payload_size);
+ std->length = cpu_to_le32(acpi_payload_size);
if (hdrs->has_rev) {
- ext_hdr->revision = hdrs->rev;
+ std->revision = hdrs->rev;
++changed_fields;
}
- ext_hdr->checksum = 0;
+ std->checksum = 0;
if (hdrs->has_oem_id) {
- strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
+ strncpy(std->oem_id, hdrs->oem_id, sizeof std->oem_id);
++changed_fields;
}
if (hdrs->has_oem_table_id) {
- strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
- sizeof ext_hdr->oem_table_id);
+ strncpy(std->oem_table_id, hdrs->oem_table_id,
+ sizeof std->oem_table_id);
++changed_fields;
}
if (hdrs->has_oem_rev) {
- ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev);
+ std->oem_revision = cpu_to_le32(hdrs->oem_rev);
++changed_fields;
}
if (hdrs->has_asl_compiler_id) {
- strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
- sizeof ext_hdr->asl_compiler_id);
+ strncpy(std->asl_compiler_id, hdrs->asl_compiler_id,
+ sizeof std->asl_compiler_id);
++changed_fields;
}
if (hdrs->has_asl_compiler_rev) {
- ext_hdr->asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
+ std->asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
++changed_fields;
}
@@ -225,8 +219,8 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
}
/* recalculate checksum */
- ext_hdr->checksum = acpi_checksum((const char unsigned *)ext_hdr +
- ACPI_TABLE_PFX_SIZE, acpi_payload_size);
+ std->checksum = acpi_checksum((const char unsigned *)std,
+ acpi_payload_size);
}
void acpi_table_add(const QemuOpts *opts, Error **errp)
--
1.7.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
` (2 preceding siblings ...)
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-25 18:49 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 5/7] hw/acpi: export acpi_checksum() Laszlo Ersek
` (3 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
This enables reuse when preparing per-table fw_cfg blobs later.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/acpi.h | 2 ++
hw/acpi/core.c | 39 ++++++++++++++++++++++++---------------
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 0e26f63..bc7e107 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -178,4 +178,6 @@ typedef struct acpi_table_std_header {
char asl_compiler_id[4]; /* ASL compiler vendor ID */
uint32_t asl_compiler_revision; /* ASL compiler revision number */
} QEMU_PACKED AcpiTableStdHdr;
+
+extern const AcpiTableStdHdr acpi_dfl_hdr;
#endif /* !QEMU_HW_ACPI_H */
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index d348f81..36a4d03 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -37,11 +37,20 @@ struct acpi_table_header {
/* size of the extra prefix */
#define ACPI_TABLE_PFX_SIZE offsetof(struct acpi_table_header, std)
-static const char unsigned dfl_hdr[sizeof(AcpiTableStdHdr)] =
- "QEMU\0\0\0\0\1\0" /* sig (4), len(4), revno (1), csum (1) */
- "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
- "QEMU\1\0\0\0" /* ASL compiler ID (4), version (4) */
- ;
+const AcpiTableStdHdr acpi_dfl_hdr = {
+ .sig = "QEMU",
+ .revision = 1,
+ .oem_id = "QEMUQE",
+ .oem_table_id = "QEMUQEMU",
+ .asl_compiler_id = "QEMU",
+#ifdef HOST_WORDS_BIGENDIAN
+ .oem_revision = 0x01000000,
+ .asl_compiler_revision = 0x01000000
+#else
+ .oem_revision = 1,
+ .asl_compiler_revision = 1
+#endif
+};
char unsigned *acpi_tables;
size_t acpi_tables_len;
@@ -74,8 +83,8 @@ static int acpi_checksum(const uint8_t *data, int len)
/* Install a copy of the ACPI table specified in @blob.
*
* If @has_header is set, @blob starts with the System Description Table Header
- * structure. Otherwise, "dfl_hdr" is prepended. In any case, each header field
- * is optionally overwritten from @hdrs.
+ * structure. Otherwise, "acpi_dfl_hdr" is prepended. In any case, each header
+ * field is optionally overwritten from @hdrs.
*
* It is valid to call this function with
* (@blob == NULL && @bloblen == 0 && !@has_header).
@@ -105,13 +114,13 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
if (has_header) {
/* _length | ACPI header in blob | blob body
* ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^
- * ACPI_TABLE_PFX_SIZE sizeof dfl_hdr body_size
+ * ACPI_TABLE_PFX_SIZE sizeof acpi_dfl_hdr body_size
* == body_start
*
* ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
* acpi_payload_size == bloblen
*/
- body_start = sizeof dfl_hdr;
+ body_start = sizeof acpi_dfl_hdr;
if (bloblen < body_start) {
error_setg(errp, "ACPI table claiming to have header is too "
@@ -123,17 +132,17 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
} else {
/* _length | ACPI header in template | blob body
* ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^
- * ACPI_TABLE_PFX_SIZE sizeof dfl_hdr body_size
+ * ACPI_TABLE_PFX_SIZE sizeof acpi_dfl_hdr body_size
* == bloblen
*
* ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
* acpi_payload_size
*/
body_start = 0;
- hdr_src = dfl_hdr;
+ hdr_src = (const char unsigned *)&acpi_dfl_hdr;
}
body_size = bloblen - body_start;
- acpi_payload_size = sizeof dfl_hdr + body_size;
+ acpi_payload_size = sizeof acpi_dfl_hdr + body_size;
if (acpi_payload_size > UINT16_MAX) {
error_setg(errp, "ACPI table too big, requested: %zu, max: %u",
@@ -149,13 +158,13 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
ACPI_TABLE_PFX_SIZE +
- sizeof dfl_hdr + body_size);
+ sizeof acpi_dfl_hdr + body_size);
ext_hdr = (struct acpi_table_header *)(acpi_tables + acpi_tables_len);
acpi_tables_len += ACPI_TABLE_PFX_SIZE;
- memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof dfl_hdr);
- acpi_tables_len += sizeof dfl_hdr;
+ memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof acpi_dfl_hdr);
+ acpi_tables_len += sizeof acpi_dfl_hdr;
if (blob != NULL) {
memcpy(acpi_tables + acpi_tables_len, blob + body_start, body_size);
--
1.7.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 5/7] hw/acpi: export acpi_checksum()
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
` (3 preceding siblings ...)
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-25 18:55 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 6/7] hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h Laszlo Ersek
` (2 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
Again, this enables reuse when preparing per-table fw_cfg blobs later.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/acpi/acpi.h | 2 ++
hw/acpi/core.c | 2 +-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index bc7e107..7e51110 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -180,4 +180,6 @@ typedef struct acpi_table_std_header {
} QEMU_PACKED AcpiTableStdHdr;
extern const AcpiTableStdHdr acpi_dfl_hdr;
+
+int acpi_checksum(const uint8_t *data, int len);
#endif /* !QEMU_HW_ACPI_H */
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 36a4d03..3be7e09 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -69,7 +69,7 @@ static void acpi_register_config(void)
machine_init(acpi_register_config);
-static int acpi_checksum(const uint8_t *data, int len)
+int acpi_checksum(const uint8_t *data, int len)
{
int sum, i;
sum = 0;
--
1.7.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 6/7] hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
` (4 preceding siblings ...)
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 5/7] hw/acpi: export acpi_checksum() Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-25 18:55 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
2013-04-24 9:39 ` [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
From: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
include/hw/i386/apic.h | 2 ++
hw/i386/pc.c | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index 1d48e02..edbb37f 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -26,6 +26,8 @@ void apic_designate_bsp(DeviceState *d);
/* pc.c */
DeviceState *cpu_get_current_apic(void);
+#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
+
/* cpu.c */
bool cpu_is_bsp(X86CPU *cpu);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3c12b55..8727489 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -73,8 +73,6 @@
#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
-#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
-
#define E820_NR_ENTRIES 16
struct e820_entry {
--
1.7.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
` (5 preceding siblings ...)
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 6/7] hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h Laszlo Ersek
@ 2013-04-18 20:22 ` Laszlo Ersek
2013-04-18 20:30 ` Michael S. Tsirkin
2013-04-25 19:03 ` Anthony Liguori
2013-04-24 9:39 ` [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
7 siblings, 2 replies; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-18 20:22 UTC (permalink / raw)
To: mst, qemu-devel
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 +++++++++++++++
| 13 ++++++++++++-
| 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);
+#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
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
@ 2013-04-18 20:30 ` Michael S. Tsirkin
2013-04-19 10:58 ` Laszlo Ersek
2013-04-25 19:03 ` Anthony Liguori
1 sibling, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2013-04-18 20:30 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-18 20:30 ` Michael S. Tsirkin
@ 2013-04-19 10:58 ` Laszlo Ersek
2013-04-24 9:42 ` Michael S. Tsirkin
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-19 10:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
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
>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
` (6 preceding siblings ...)
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
@ 2013-04-24 9:39 ` Laszlo Ersek
2013-04-25 16:45 ` Anthony Liguori
7 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-24 9:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin
On 04/18/13 22:22, Laszlo Ersek wrote:
> v4:
> - patches 1-6 are unchanged and carry Michael's ACK,
> - patch 7 is rendered dependent on a new configure switch (default off)
> [Michael Tsirkin].
>
> v3:
> - rebased to current master 24a6e7f4,
> - added Michael's S-o-b to 6/7 [Eric Blake],
> - added Dave Frodin's relicensing ACK to 7/7, originally sent to Michael
> in private [Michael Tsirkin],
> - slightly reworded 7/7's commit message where it credits Michael's
> prototype [Eric Blake].
>
> v2:
> - address (1) in
> <http://thread.gmane.org/gmane.comp.emulators.qemu/206146/focus=206195>:
>
> - rebase to Paolo's recent series
> <http://thread.gmane.org/gmane.comp.emulators.qemu/206196/focus=206278>,
>
> - patch 2 is new, fixes style of function parameter references in a
> comment [mst],
>
> - patch 6 is new, moves a macro definition [mst],
>
> - patch 7 should be structured more logically now, plus the commit
> message has grown some bits related to licensing [mst].
> acpi_table_fill_hdr() is kept distinct from the -acpitable switch's
> implementation on purpose, similarly to pc_acpi_install() in v1.
>
> - Tested APIC / DSDT / RSDT against contents saved from v1.
>
> v1 blurb:
>
> This series exports the MADT (APIC) ACPI table under the new
> "etc/acpi/APIC" fw_cfg file. I sought to follow the requirements set
> forth in [1], the new table is only visible in the patched/patched
> case. I cross-tested { master, patched } qemu with { master, patched }
> seabios (the APIC, DSDT and RSDT tables) using guest acpidump and
> dmesg.
>
> The -acpitable command line option is purposely ignored based on the
> last paragraph of [2]; the user isn't supposed to pass APIC with that
> option.
>
> checkpatch.pl complains a little but (as last time) it's a false
> alarm.
>
> The series is bisectable.
>
> [1] http://thread.gmane.org/gmane.comp.emulators.qemu/202005/focus=202072
> [2] http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5960/focus=6008
>
> Laszlo Ersek (6):
> refer to FWCfgState explicitly
> acpi_table_install(): fix funcparam formatting in leading comment
> hw/acpi: extract standard table headers as a standalone structure
> hw/acpi: export default ACPI headers using the type just introduced
> hw/acpi: export acpi_checksum()
> hw/i386: build ACPI MADT (APIC) for fw_cfg clients
>
> Michael S. Tsirkin (1):
> hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h
>
> configure | 12 ++++
> hw/i386/Makefile.objs | 1 +
> hw/i386/acpi.h | 9 +++
> hw/i386/multiboot.h | 4 +-
> include/hw/acpi/acpi.h | 15 +++++
> include/hw/i386/apic.h | 2 +
> include/hw/i386/pc.h | 19 +++---
> include/hw/loader.h | 3 +-
> hw/acpi/core.c | 91 ++++++++++++++-------------
> hw/acpi/piix4.c | 2 +-
> hw/core/loader.c | 2 +-
> hw/i386/acpi.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++
> hw/i386/multiboot.c | 2 +-
> hw/i386/pc.c | 49 +++++++++++----
> hw/i386/pc_piix.c | 2 +-
> hw/sparc/sun4m.c | 6 +-
> hw/sparc64/sun4u.c | 2 +-
> 17 files changed, 303 insertions(+), 77 deletions(-)
> create mode 100644 hw/i386/acpi.h
> create mode 100644 hw/i386/acpi.c
Ping... When may I expect reviews for this?
(1) AIUI,
- Michael hasn't acked this series, but he didn't NAK it either -- the
two of us disagree on his most recent review, but that shouldn't be
reason for others not to look at the series,
- he's alrady cooking a series based on this one.
(2) I'm aware that we're in soft freeze for 1.5 now. I'm just saying I
won't touch the HPET or SRAT until this MADT series is sufficiently
acked. I need to be able to trust the structure if I want it to carry
the HPET and the SRAT too. (I don't insist in the slightest on
(co-)owning this task, ie. there's no time pressure from my side. I'm
just pointing out a dependency.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-19 10:58 ` Laszlo Ersek
@ 2013-04-24 9:42 ` Michael S. Tsirkin
0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2013-04-24 9:42 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel
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
> >>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg
2013-04-24 9:39 ` [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
@ 2013-04-25 16:45 ` Anthony Liguori
0 siblings, 0 replies; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 16:45 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: Michael S. Tsirkin
Laszlo Ersek <lersek@redhat.com> writes:
> On 04/18/13 22:22, Laszlo Ersek wrote:
>> v4:
>> - patches 1-6 are unchanged and carry Michael's ACK,
>> - patch 7 is rendered dependent on a new configure switch (default off)
>> [Michael Tsirkin].
>>
>> v3:
>> - rebased to current master 24a6e7f4,
>> - added Michael's S-o-b to 6/7 [Eric Blake],
>> - added Dave Frodin's relicensing ACK to 7/7, originally sent to Michael
>> in private [Michael Tsirkin],
>> - slightly reworded 7/7's commit message where it credits Michael's
>> prototype [Eric Blake].
>>
>> v2:
>> - address (1) in
>> <http://thread.gmane.org/gmane.comp.emulators.qemu/206146/focus=206195>:
>>
>> - rebase to Paolo's recent series
>> <http://thread.gmane.org/gmane.comp.emulators.qemu/206196/focus=206278>,
>>
>> - patch 2 is new, fixes style of function parameter references in a
>> comment [mst],
>>
>> - patch 6 is new, moves a macro definition [mst],
>>
>> - patch 7 should be structured more logically now, plus the commit
>> message has grown some bits related to licensing [mst].
>> acpi_table_fill_hdr() is kept distinct from the -acpitable switch's
>> implementation on purpose, similarly to pc_acpi_install() in v1.
>>
>> - Tested APIC / DSDT / RSDT against contents saved from v1.
>>
>> v1 blurb:
>>
>> This series exports the MADT (APIC) ACPI table under the new
>> "etc/acpi/APIC" fw_cfg file. I sought to follow the requirements set
>> forth in [1], the new table is only visible in the patched/patched
>> case. I cross-tested { master, patched } qemu with { master, patched }
>> seabios (the APIC, DSDT and RSDT tables) using guest acpidump and
>> dmesg.
>>
>> The -acpitable command line option is purposely ignored based on the
>> last paragraph of [2]; the user isn't supposed to pass APIC with that
>> option.
>>
>> checkpatch.pl complains a little but (as last time) it's a false
>> alarm.
>>
>> The series is bisectable.
>>
>> [1] http://thread.gmane.org/gmane.comp.emulators.qemu/202005/focus=202072
>> [2] http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5960/focus=6008
>>
>> Laszlo Ersek (6):
> refer to FWCfgState explicitly
>> acpi_table_install(): fix funcparam formatting in leading comment
>> hw/acpi: extract standard table headers as a standalone structure
>> hw/acpi: export default ACPI headers using the type just introduced
>> hw/acpi: export acpi_checksum()
>> hw/i386: build ACPI MADT (APIC) for fw_cfg clients
>>
>> Michael S. Tsirkin (1):
>> hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h
>>
>> configure | 12 ++++
>> hw/i386/Makefile.objs | 1 +
>> hw/i386/acpi.h | 9 +++
>> hw/i386/multiboot.h | 4 +-
>> include/hw/acpi/acpi.h | 15 +++++
>> include/hw/i386/apic.h | 2 +
>> include/hw/i386/pc.h | 19 +++---
>> include/hw/loader.h | 3 +-
>> hw/acpi/core.c | 91 ++++++++++++++-------------
>> hw/acpi/piix4.c | 2 +-
>> hw/core/loader.c | 2 +-
>> hw/i386/acpi.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/i386/multiboot.c | 2 +-
>> hw/i386/pc.c | 49 +++++++++++----
>> hw/i386/pc_piix.c | 2 +-
>> hw/sparc/sun4m.c | 6 +-
>> hw/sparc64/sun4u.c | 2 +-
>> 17 files changed, 303 insertions(+), 77 deletions(-)
>> create mode 100644 hw/i386/acpi.h
>> create mode 100644 hw/i386/acpi.c
>
> Ping... When may I expect reviews for this?
I'll take a look this afternoon.
It helps to CC folks who could potentially review it. When it doubt,
never hurts to CC me.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly Laszlo Ersek
@ 2013-04-25 18:44 ` Anthony Liguori
2013-04-25 21:04 ` Michael S. Tsirkin
0 siblings, 1 reply; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 18:44 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
Laszlo Ersek <lersek@redhat.com> writes:
> Currently some places use pointer-to-void even though they mean
> pointer-to-FWCfgState. Clean them up.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> hw/i386/multiboot.h | 4 +++-
> include/hw/i386/pc.h | 19 ++++++++++---------
> include/hw/loader.h | 3 ++-
> hw/acpi/piix4.c | 2 +-
> hw/core/loader.c | 2 +-
> hw/i386/multiboot.c | 2 +-
> hw/i386/pc.c | 24 ++++++++++++------------
> hw/i386/pc_piix.c | 2 +-
> hw/sparc/sun4m.c | 6 +++---
> hw/sparc64/sun4u.c | 2 +-
> 10 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/hw/i386/multiboot.h b/hw/i386/multiboot.h
> index 98fb1b7..60de309 100644
> --- a/hw/i386/multiboot.h
> +++ b/hw/i386/multiboot.h
> @@ -1,7 +1,9 @@
> #ifndef QEMU_MULTIBOOT_H
> #define QEMU_MULTIBOOT_H
>
> -int load_multiboot(void *fw_cfg,
> +#include "hw/nvram/fw_cfg.h"
> +
> +int load_multiboot(FWCfgState *fw_cfg,
> FILE *f,
> const char *kernel_filename,
> const char *initrd_filename,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 9bcc819..fa4b5ce 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -9,6 +9,7 @@
> #include "net/net.h"
> #include "exec/memory.h"
> #include "hw/i386/ioapic.h"
> +#include "hw/nvram/fw_cfg.h"
>
> /* PC-style peripherals (also used by other machines). */
>
> @@ -80,14 +81,14 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>
> void pc_cpus_init(const char *cpu_model);
> void pc_acpi_init(const char *default_dsdt);
> -void *pc_memory_init(MemoryRegion *system_memory,
> - const char *kernel_filename,
> - const char *kernel_cmdline,
> - const char *initrd_filename,
> - ram_addr_t below_4g_mem_size,
> - ram_addr_t above_4g_mem_size,
> - MemoryRegion *rom_memory,
> - MemoryRegion **ram_memory);
> +FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> + const char *kernel_filename,
> + const char *kernel_cmdline,
> + const char *initrd_filename,
> + ram_addr_t below_4g_mem_size,
> + ram_addr_t above_4g_mem_size,
> + MemoryRegion *rom_memory,
> + MemoryRegion **ram_memory);
> qemu_irq *pc_allocate_cpu_irq(void);
> DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> @@ -111,7 +112,7 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>
> i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> qemu_irq sci_irq, qemu_irq smi_irq,
> - int kvm_enabled, void *fw_cfg);
> + int kvm_enabled, FWCfgState *fw_cfg);
> void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
>
> /* hpet.c */
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 0958f06..15d4cc9 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -1,6 +1,7 @@
> #ifndef LOADER_H
> #define LOADER_H
> #include "qapi/qmp/qdict.h"
> +#include "hw/nvram/fw_cfg.h"
>
> /* loader.c */
> int get_image_size(const char *filename);
> @@ -30,7 +31,7 @@ int rom_add_blob(const char *name, const void *blob, size_t len,
> int rom_add_elf_program(const char *name, void *data, size_t datasize,
> size_t romsize, hwaddr addr);
> int rom_load_all(void);
> -void rom_set_fw(void *f);
> +void rom_set_fw(FWCfgState *f);
> int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
> void *rom_ptr(hwaddr addr);
> void do_info_roms(Monitor *mon, const QDict *qdict);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 88386d7..200c297 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -435,7 +435,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
>
> i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> qemu_irq sci_irq, qemu_irq smi_irq,
> - int kvm_enabled, void *fw_cfg)
> + int kvm_enabled, FWCfgState *fw_cfg)
> {
> PCIDevice *dev;
> PIIX4PMState *s;
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7507914..a711145 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -733,7 +733,7 @@ int rom_load_all(void)
> return 0;
> }
>
> -void rom_set_fw(void *f)
> +void rom_set_fw(FWCfgState *f)
> {
> fw_cfg = f;
> }
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index d696507..09211e0 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -124,7 +124,7 @@ static void mb_add_mod(MultibootState *s,
> s->mb_mods_count++;
> }
>
> -int load_multiboot(void *fw_cfg,
> +int load_multiboot(FWCfgState *fw_cfg,
> FILE *f,
> const char *kernel_filename,
> const char *initrd_filename,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0d6e72b..3c12b55 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -569,9 +569,9 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> }
>
> -static void *bochs_bios_init(void)
> +static FWCfgState *bochs_bios_init(void)
> {
> - void *fw_cfg;
> + FWCfgState *fw_cfg;
> uint8_t *smbios_table;
> size_t smbios_len;
> uint64_t *numa_fw_cfg;
> @@ -648,7 +648,7 @@ static long get_file_size(FILE *f)
> return size;
> }
>
> -static void load_linux(void *fw_cfg,
> +static void load_linux(FWCfgState *fw_cfg,
> const char *kernel_filename,
> const char *initrd_filename,
> const char *kernel_cmdline,
> @@ -924,19 +924,19 @@ void pc_acpi_init(const char *default_dsdt)
> }
> }
>
> -void *pc_memory_init(MemoryRegion *system_memory,
> - const char *kernel_filename,
> - const char *kernel_cmdline,
> - const char *initrd_filename,
> - ram_addr_t below_4g_mem_size,
> - ram_addr_t above_4g_mem_size,
> - MemoryRegion *rom_memory,
> - MemoryRegion **ram_memory)
> +FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> + const char *kernel_filename,
> + const char *kernel_cmdline,
> + const char *initrd_filename,
> + ram_addr_t below_4g_mem_size,
> + ram_addr_t above_4g_mem_size,
> + MemoryRegion *rom_memory,
> + MemoryRegion **ram_memory)
> {
> int linux_boot, i;
> MemoryRegion *ram, *option_rom_mr;
> MemoryRegion *ram_below_4g, *ram_above_4g;
> - void *fw_cfg;
> + FWCfgState *fw_cfg;
>
> linux_boot = (kernel_filename != NULL);
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 943758a..da10e6d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -85,7 +85,7 @@ static void pc_init1(MemoryRegion *system_memory,
> MemoryRegion *ram_memory;
> MemoryRegion *pci_memory;
> MemoryRegion *rom_memory;
> - void *fw_cfg = NULL;
> + FWCfgState *fw_cfg = NULL;
>
> pc_cpus_init(cpu_model);
> pc_acpi_init("acpi-dsdt.aml");
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 31beb32..0b1b620 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -874,7 +874,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
> qemu_irq *cpu_halt;
> unsigned long kernel_size;
> DriveInfo *fd[MAX_FD];
> - void *fw_cfg;
> + FWCfgState *fw_cfg;
> unsigned int num_vsimms;
>
> /* init CPUs */
> @@ -1592,7 +1592,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
> espdma_irq, ledma_irq;
> qemu_irq esp_reset, dma_enable;
> unsigned long kernel_size;
> - void *fw_cfg;
> + FWCfgState *fw_cfg;
> DeviceState *dev;
>
> /* init CPUs */
> @@ -1793,7 +1793,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
> qemu_irq fdc_tc;
> unsigned long kernel_size;
> DriveInfo *fd[MAX_FD];
> - void *fw_cfg;
> + FWCfgState *fw_cfg;
> DeviceState *dev;
> unsigned int i;
>
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 0d29620..0138252 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -818,7 +818,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> qemu_irq *ivec_irqs, *pbm_irqs;
> DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> DriveInfo *fd[MAX_FD];
> - void *fw_cfg;
> + FWCfgState *fw_cfg;
>
> /* init CPUs */
> cpu = cpu_devinit(cpu_model, hwdef);
> --
> 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/7] acpi_table_install(): fix funcparam formatting in leading comment
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 2/7] acpi_table_install(): fix funcparam formatting in leading comment Laszlo Ersek
@ 2013-04-25 18:44 ` Anthony Liguori
0 siblings, 0 replies; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 18:44 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
Laszlo Ersek <lersek@redhat.com> writes:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> hw/acpi/core.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 64b8718..69cadb0 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -86,7 +86,7 @@ static int acpi_checksum(const uint8_t *data, int len)
> * is optionally overwritten from @hdrs.
> *
> * It is valid to call this function with
> - * (@blob == NULL && bloblen == 0 && !has_header).
> + * (@blob == NULL && @bloblen == 0 && !@has_header).
> *
> * @hdrs->file and @hdrs->data are ignored.
> *
> --
> 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure Laszlo Ersek
@ 2013-04-25 18:47 ` Anthony Liguori
2013-04-26 9:32 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 18:47 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
Laszlo Ersek <lersek@redhat.com> writes:
> This enables reuse when preparing per-table fw_cfg blobs later.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/hw/acpi/acpi.h | 11 +++++++++++
> hw/acpi/core.c | 48 +++++++++++++++++++++---------------------------
> 2 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 635be7b..0e26f63 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -167,4 +167,15 @@ extern size_t acpi_tables_len;
>
> void acpi_table_add(const QemuOpts *opts, Error **errp);
>
> +typedef struct acpi_table_std_header {
> + char sig[4]; /* ACPI signature (4 ASCII characters) */
> + uint32_t length; /* Length of table, in bytes, including header */
> + uint8_t revision; /* ACPI Specification minor version # */
> + uint8_t checksum; /* To make sum of entire table == 0 */
> + char oem_id[6]; /* OEM identification */
> + char oem_table_id[8]; /* OEM table identification */
> + uint32_t oem_revision; /* OEM revision number */
> + char asl_compiler_id[4]; /* ASL compiler vendor ID */
> + uint32_t asl_compiler_revision; /* ASL compiler revision number */
> +} QEMU_PACKED AcpiTableStdHdr;
Since you're giving it a CamelCaseName why don't you do the same for the
struct. After that:
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> #endif /* !QEMU_HW_ACPI_H */
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 69cadb0..d348f81 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -31,21 +31,13 @@
> struct acpi_table_header {
> uint16_t _length; /* our length, not actual part of the hdr */
> /* allows easier parsing for fw_cfg clients */
> - char sig[4]; /* ACPI signature (4 ASCII characters) */
> - uint32_t length; /* Length of table, in bytes, including header */
> - uint8_t revision; /* ACPI Specification minor version # */
> - uint8_t checksum; /* To make sum of entire table == 0 */
> - char oem_id[6]; /* OEM identification */
> - char oem_table_id[8]; /* OEM table identification */
> - uint32_t oem_revision; /* OEM revision number */
> - char asl_compiler_id[4]; /* ASL compiler vendor ID */
> - uint32_t asl_compiler_revision; /* ASL compiler revision number */
> + AcpiTableStdHdr std;
> } QEMU_PACKED;
>
> -#define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
> -#define ACPI_TABLE_PFX_SIZE sizeof(uint16_t) /* size of the extra prefix */
> +/* size of the extra prefix */
> +#define ACPI_TABLE_PFX_SIZE offsetof(struct acpi_table_header, std)
>
> -static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE] =
> +static const char unsigned dfl_hdr[sizeof(AcpiTableStdHdr)] =
> "QEMU\0\0\0\0\1\0" /* sig (4), len(4), revno (1), csum (1) */
> "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
> "QEMU\1\0\0\0" /* ASL compiler ID (4), version (4) */
> @@ -105,6 +97,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
> size_t body_size, acpi_payload_size;
> struct acpi_table_header *ext_hdr;
> unsigned changed_fields;
> + AcpiTableStdHdr *std;
>
> /* Calculate where the ACPI table body starts within the blob, plus where
> * to copy the ACPI table header from.
> @@ -177,46 +170,47 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
> changed_fields = 0;
> ext_hdr->_length = cpu_to_le16(acpi_payload_size);
>
> + std = &ext_hdr->std;
> if (hdrs->has_sig) {
> - strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> + strncpy(std->sig, hdrs->sig, sizeof std->sig);
> ++changed_fields;
> }
>
> - if (has_header && le32_to_cpu(ext_hdr->length) != acpi_payload_size) {
> + if (has_header && le32_to_cpu(std->length) != acpi_payload_size) {
> fprintf(stderr,
> "warning: ACPI table has wrong length, header says "
> "%" PRIu32 ", actual size %zu bytes\n",
> - le32_to_cpu(ext_hdr->length), acpi_payload_size);
> + le32_to_cpu(std->length), acpi_payload_size);
> }
> - ext_hdr->length = cpu_to_le32(acpi_payload_size);
> + std->length = cpu_to_le32(acpi_payload_size);
>
> if (hdrs->has_rev) {
> - ext_hdr->revision = hdrs->rev;
> + std->revision = hdrs->rev;
> ++changed_fields;
> }
>
> - ext_hdr->checksum = 0;
> + std->checksum = 0;
>
> if (hdrs->has_oem_id) {
> - strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
> + strncpy(std->oem_id, hdrs->oem_id, sizeof std->oem_id);
> ++changed_fields;
> }
> if (hdrs->has_oem_table_id) {
> - strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
> - sizeof ext_hdr->oem_table_id);
> + strncpy(std->oem_table_id, hdrs->oem_table_id,
> + sizeof std->oem_table_id);
> ++changed_fields;
> }
> if (hdrs->has_oem_rev) {
> - ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev);
> + std->oem_revision = cpu_to_le32(hdrs->oem_rev);
> ++changed_fields;
> }
> if (hdrs->has_asl_compiler_id) {
> - strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
> - sizeof ext_hdr->asl_compiler_id);
> + strncpy(std->asl_compiler_id, hdrs->asl_compiler_id,
> + sizeof std->asl_compiler_id);
> ++changed_fields;
> }
> if (hdrs->has_asl_compiler_rev) {
> - ext_hdr->asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
> + std->asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
> ++changed_fields;
> }
>
> @@ -225,8 +219,8 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
> }
>
> /* recalculate checksum */
> - ext_hdr->checksum = acpi_checksum((const char unsigned *)ext_hdr +
> - ACPI_TABLE_PFX_SIZE, acpi_payload_size);
> + std->checksum = acpi_checksum((const char unsigned *)std,
> + acpi_payload_size);
> }
>
> void acpi_table_add(const QemuOpts *opts, Error **errp)
> --
> 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced Laszlo Ersek
@ 2013-04-25 18:49 ` Anthony Liguori
2013-04-26 9:53 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 18:49 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
Laszlo Ersek <lersek@redhat.com> writes:
> This enables reuse when preparing per-table fw_cfg blobs later.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/hw/acpi/acpi.h | 2 ++
> hw/acpi/core.c | 39 ++++++++++++++++++++++++---------------
> 2 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 0e26f63..bc7e107 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -178,4 +178,6 @@ typedef struct acpi_table_std_header {
> char asl_compiler_id[4]; /* ASL compiler vendor ID */
> uint32_t asl_compiler_revision; /* ASL compiler revision number */
> } QEMU_PACKED AcpiTableStdHdr;
> +
> +extern const AcpiTableStdHdr acpi_dfl_hdr;
Can we do this without using a global variable? I assume this gets
memcpy()'d or referenced as a pointer later on or something like that?
Can we make a function that does that?
Regards,
Anthony Liguori
> #endif /* !QEMU_HW_ACPI_H */
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index d348f81..36a4d03 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -37,11 +37,20 @@ struct acpi_table_header {
> /* size of the extra prefix */
> #define ACPI_TABLE_PFX_SIZE offsetof(struct acpi_table_header, std)
>
> -static const char unsigned dfl_hdr[sizeof(AcpiTableStdHdr)] =
> - "QEMU\0\0\0\0\1\0" /* sig (4), len(4), revno (1), csum (1) */
> - "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
> - "QEMU\1\0\0\0" /* ASL compiler ID (4), version (4) */
> - ;
> +const AcpiTableStdHdr acpi_dfl_hdr = {
> + .sig = "QEMU",
> + .revision = 1,
> + .oem_id = "QEMUQE",
> + .oem_table_id = "QEMUQEMU",
> + .asl_compiler_id = "QEMU",
> +#ifdef HOST_WORDS_BIGENDIAN
> + .oem_revision = 0x01000000,
> + .asl_compiler_revision = 0x01000000
> +#else
> + .oem_revision = 1,
> + .asl_compiler_revision = 1
> +#endif
> +};
>
> char unsigned *acpi_tables;
> size_t acpi_tables_len;
> @@ -74,8 +83,8 @@ static int acpi_checksum(const uint8_t *data, int len)
> /* Install a copy of the ACPI table specified in @blob.
> *
> * If @has_header is set, @blob starts with the System Description Table Header
> - * structure. Otherwise, "dfl_hdr" is prepended. In any case, each header field
> - * is optionally overwritten from @hdrs.
> + * structure. Otherwise, "acpi_dfl_hdr" is prepended. In any case, each header
> + * field is optionally overwritten from @hdrs.
> *
> * It is valid to call this function with
> * (@blob == NULL && @bloblen == 0 && !@has_header).
> @@ -105,13 +114,13 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
> if (has_header) {
> /* _length | ACPI header in blob | blob body
> * ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^
> - * ACPI_TABLE_PFX_SIZE sizeof dfl_hdr body_size
> + * ACPI_TABLE_PFX_SIZE sizeof acpi_dfl_hdr body_size
> * == body_start
> *
> * ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> * acpi_payload_size == bloblen
> */
> - body_start = sizeof dfl_hdr;
> + body_start = sizeof acpi_dfl_hdr;
>
> if (bloblen < body_start) {
> error_setg(errp, "ACPI table claiming to have header is too "
> @@ -123,17 +132,17 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
> } else {
> /* _length | ACPI header in template | blob body
> * ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^
> - * ACPI_TABLE_PFX_SIZE sizeof dfl_hdr body_size
> + * ACPI_TABLE_PFX_SIZE sizeof acpi_dfl_hdr body_size
> * == bloblen
> *
> * ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> * acpi_payload_size
> */
> body_start = 0;
> - hdr_src = dfl_hdr;
> + hdr_src = (const char unsigned *)&acpi_dfl_hdr;
> }
> body_size = bloblen - body_start;
> - acpi_payload_size = sizeof dfl_hdr + body_size;
> + acpi_payload_size = sizeof acpi_dfl_hdr + body_size;
>
> if (acpi_payload_size > UINT16_MAX) {
> error_setg(errp, "ACPI table too big, requested: %zu, max: %u",
> @@ -149,13 +158,13 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
>
> acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
> ACPI_TABLE_PFX_SIZE +
> - sizeof dfl_hdr + body_size);
> + sizeof acpi_dfl_hdr + body_size);
>
> ext_hdr = (struct acpi_table_header *)(acpi_tables + acpi_tables_len);
> acpi_tables_len += ACPI_TABLE_PFX_SIZE;
>
> - memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof dfl_hdr);
> - acpi_tables_len += sizeof dfl_hdr;
> + memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof acpi_dfl_hdr);
> + acpi_tables_len += sizeof acpi_dfl_hdr;
>
> if (blob != NULL) {
> memcpy(acpi_tables + acpi_tables_len, blob + body_start, body_size);
> --
> 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/7] hw/acpi: export acpi_checksum()
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 5/7] hw/acpi: export acpi_checksum() Laszlo Ersek
@ 2013-04-25 18:55 ` Anthony Liguori
0 siblings, 0 replies; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 18:55 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
Laszlo Ersek <lersek@redhat.com> writes:
> Again, this enables reuse when preparing per-table fw_cfg blobs later.
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> include/hw/acpi/acpi.h | 2 ++
> hw/acpi/core.c | 2 +-
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index bc7e107..7e51110 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -180,4 +180,6 @@ typedef struct acpi_table_std_header {
> } QEMU_PACKED AcpiTableStdHdr;
>
> extern const AcpiTableStdHdr acpi_dfl_hdr;
> +
> +int acpi_checksum(const uint8_t *data, int len);
> #endif /* !QEMU_HW_ACPI_H */
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 36a4d03..3be7e09 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -69,7 +69,7 @@ static void acpi_register_config(void)
>
> machine_init(acpi_register_config);
>
> -static int acpi_checksum(const uint8_t *data, int len)
> +int acpi_checksum(const uint8_t *data, int len)
> {
> int sum, i;
> sum = 0;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 6/7] hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 6/7] hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h Laszlo Ersek
@ 2013-04-25 18:55 ` Anthony Liguori
0 siblings, 0 replies; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 18:55 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
Laszlo Ersek <lersek@redhat.com> writes:
> From: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> include/hw/i386/apic.h | 2 ++
> hw/i386/pc.c | 2 --
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
> index 1d48e02..edbb37f 100644
> --- a/include/hw/i386/apic.h
> +++ b/include/hw/i386/apic.h
> @@ -26,6 +26,8 @@ void apic_designate_bsp(DeviceState *d);
> /* pc.c */
> DeviceState *cpu_get_current_apic(void);
>
> +#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
> +
> /* cpu.c */
> bool cpu_is_bsp(X86CPU *cpu);
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3c12b55..8727489 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -73,8 +73,6 @@
> #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>
> -#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
> -
> #define E820_NR_ENTRIES 16
>
> struct e820_entry {
> --
> 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
2013-04-18 20:30 ` Michael S. Tsirkin
@ 2013-04-25 19:03 ` Anthony Liguori
2013-04-25 20:11 ` Eduardo Habkost
2013-04-26 11:13 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
1 sibling, 2 replies; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 19:03 UTC (permalink / raw)
To: Laszlo Ersek, mst, qemu-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-25 19:03 ` Anthony Liguori
@ 2013-04-25 20:11 ` Eduardo Habkost
2013-04-25 20:45 ` Anthony Liguori
2013-04-26 11:13 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
1 sibling, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2013-04-25 20:11 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Laszlo Ersek, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-25 20:11 ` Eduardo Habkost
@ 2013-04-25 20:45 ` Anthony Liguori
2013-04-25 20:57 ` [Qemu-devel] Purpose of qemu-common.h (was Re: [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients) Eduardo Habkost
0 siblings, 1 reply; 32+ messages in thread
From: Anthony Liguori @ 2013-04-25 20:45 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Laszlo Ersek, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [Qemu-devel] Purpose of qemu-common.h (was Re: [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients)
2013-04-25 20:45 ` Anthony Liguori
@ 2013-04-25 20:57 ` Eduardo Habkost
2013-04-25 21:33 ` Michael S. Tsirkin
0 siblings, 1 reply; 32+ messages in thread
From: Eduardo Habkost @ 2013-04-25 20:57 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Laszlo Ersek, qemu-devel, mst
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.
--
Eduardo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly
2013-04-25 18:44 ` Anthony Liguori
@ 2013-04-25 21:04 ` Michael S. Tsirkin
0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2013-04-25 21:04 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Laszlo Ersek, qemu-devel
On Thu, Apr 25, 2013 at 01:44:20PM -0500, Anthony Liguori wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>
> > Currently some places use pointer-to-void even though they mean
> > pointer-to-FWCfgState. Clean them up.
> >
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>
> Regards,
>
> Anthony Liguori
So which tree should this be merged through?
Should I pick it up?
> > ---
> > hw/i386/multiboot.h | 4 +++-
> > include/hw/i386/pc.h | 19 ++++++++++---------
> > include/hw/loader.h | 3 ++-
> > hw/acpi/piix4.c | 2 +-
> > hw/core/loader.c | 2 +-
> > hw/i386/multiboot.c | 2 +-
> > hw/i386/pc.c | 24 ++++++++++++------------
> > hw/i386/pc_piix.c | 2 +-
> > hw/sparc/sun4m.c | 6 +++---
> > hw/sparc64/sun4u.c | 2 +-
> > 10 files changed, 35 insertions(+), 31 deletions(-)
> >
> > diff --git a/hw/i386/multiboot.h b/hw/i386/multiboot.h
> > index 98fb1b7..60de309 100644
> > --- a/hw/i386/multiboot.h
> > +++ b/hw/i386/multiboot.h
> > @@ -1,7 +1,9 @@
> > #ifndef QEMU_MULTIBOOT_H
> > #define QEMU_MULTIBOOT_H
> >
> > -int load_multiboot(void *fw_cfg,
> > +#include "hw/nvram/fw_cfg.h"
> > +
> > +int load_multiboot(FWCfgState *fw_cfg,
> > FILE *f,
> > const char *kernel_filename,
> > const char *initrd_filename,
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 9bcc819..fa4b5ce 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -9,6 +9,7 @@
> > #include "net/net.h"
> > #include "exec/memory.h"
> > #include "hw/i386/ioapic.h"
> > +#include "hw/nvram/fw_cfg.h"
> >
> > /* PC-style peripherals (also used by other machines). */
> >
> > @@ -80,14 +81,14 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
> >
> > void pc_cpus_init(const char *cpu_model);
> > void pc_acpi_init(const char *default_dsdt);
> > -void *pc_memory_init(MemoryRegion *system_memory,
> > - const char *kernel_filename,
> > - const char *kernel_cmdline,
> > - const char *initrd_filename,
> > - ram_addr_t below_4g_mem_size,
> > - ram_addr_t above_4g_mem_size,
> > - MemoryRegion *rom_memory,
> > - MemoryRegion **ram_memory);
> > +FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > + const char *kernel_filename,
> > + const char *kernel_cmdline,
> > + const char *initrd_filename,
> > + ram_addr_t below_4g_mem_size,
> > + ram_addr_t above_4g_mem_size,
> > + MemoryRegion *rom_memory,
> > + MemoryRegion **ram_memory);
> > qemu_irq *pc_allocate_cpu_irq(void);
> > DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> > void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> > @@ -111,7 +112,7 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
> >
> > i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> > qemu_irq sci_irq, qemu_irq smi_irq,
> > - int kvm_enabled, void *fw_cfg);
> > + int kvm_enabled, FWCfgState *fw_cfg);
> > void piix4_smbus_register_device(SMBusDevice *dev, uint8_t addr);
> >
> > /* hpet.c */
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index 0958f06..15d4cc9 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -1,6 +1,7 @@
> > #ifndef LOADER_H
> > #define LOADER_H
> > #include "qapi/qmp/qdict.h"
> > +#include "hw/nvram/fw_cfg.h"
> >
> > /* loader.c */
> > int get_image_size(const char *filename);
> > @@ -30,7 +31,7 @@ int rom_add_blob(const char *name, const void *blob, size_t len,
> > int rom_add_elf_program(const char *name, void *data, size_t datasize,
> > size_t romsize, hwaddr addr);
> > int rom_load_all(void);
> > -void rom_set_fw(void *f);
> > +void rom_set_fw(FWCfgState *f);
> > int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
> > void *rom_ptr(hwaddr addr);
> > void do_info_roms(Monitor *mon, const QDict *qdict);
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 88386d7..200c297 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -435,7 +435,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
> >
> > i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> > qemu_irq sci_irq, qemu_irq smi_irq,
> > - int kvm_enabled, void *fw_cfg)
> > + int kvm_enabled, FWCfgState *fw_cfg)
> > {
> > PCIDevice *dev;
> > PIIX4PMState *s;
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 7507914..a711145 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -733,7 +733,7 @@ int rom_load_all(void)
> > return 0;
> > }
> >
> > -void rom_set_fw(void *f)
> > +void rom_set_fw(FWCfgState *f)
> > {
> > fw_cfg = f;
> > }
> > diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> > index d696507..09211e0 100644
> > --- a/hw/i386/multiboot.c
> > +++ b/hw/i386/multiboot.c
> > @@ -124,7 +124,7 @@ static void mb_add_mod(MultibootState *s,
> > s->mb_mods_count++;
> > }
> >
> > -int load_multiboot(void *fw_cfg,
> > +int load_multiboot(FWCfgState *fw_cfg,
> > FILE *f,
> > const char *kernel_filename,
> > const char *initrd_filename,
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 0d6e72b..3c12b55 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -569,9 +569,9 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> > return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > }
> >
> > -static void *bochs_bios_init(void)
> > +static FWCfgState *bochs_bios_init(void)
> > {
> > - void *fw_cfg;
> > + FWCfgState *fw_cfg;
> > uint8_t *smbios_table;
> > size_t smbios_len;
> > uint64_t *numa_fw_cfg;
> > @@ -648,7 +648,7 @@ static long get_file_size(FILE *f)
> > return size;
> > }
> >
> > -static void load_linux(void *fw_cfg,
> > +static void load_linux(FWCfgState *fw_cfg,
> > const char *kernel_filename,
> > const char *initrd_filename,
> > const char *kernel_cmdline,
> > @@ -924,19 +924,19 @@ void pc_acpi_init(const char *default_dsdt)
> > }
> > }
> >
> > -void *pc_memory_init(MemoryRegion *system_memory,
> > - const char *kernel_filename,
> > - const char *kernel_cmdline,
> > - const char *initrd_filename,
> > - ram_addr_t below_4g_mem_size,
> > - ram_addr_t above_4g_mem_size,
> > - MemoryRegion *rom_memory,
> > - MemoryRegion **ram_memory)
> > +FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > + const char *kernel_filename,
> > + const char *kernel_cmdline,
> > + const char *initrd_filename,
> > + ram_addr_t below_4g_mem_size,
> > + ram_addr_t above_4g_mem_size,
> > + MemoryRegion *rom_memory,
> > + MemoryRegion **ram_memory)
> > {
> > int linux_boot, i;
> > MemoryRegion *ram, *option_rom_mr;
> > MemoryRegion *ram_below_4g, *ram_above_4g;
> > - void *fw_cfg;
> > + FWCfgState *fw_cfg;
> >
> > linux_boot = (kernel_filename != NULL);
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 943758a..da10e6d 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -85,7 +85,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > MemoryRegion *ram_memory;
> > MemoryRegion *pci_memory;
> > MemoryRegion *rom_memory;
> > - void *fw_cfg = NULL;
> > + FWCfgState *fw_cfg = NULL;
> >
> > pc_cpus_init(cpu_model);
> > pc_acpi_init("acpi-dsdt.aml");
> > diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> > index 31beb32..0b1b620 100644
> > --- a/hw/sparc/sun4m.c
> > +++ b/hw/sparc/sun4m.c
> > @@ -874,7 +874,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
> > qemu_irq *cpu_halt;
> > unsigned long kernel_size;
> > DriveInfo *fd[MAX_FD];
> > - void *fw_cfg;
> > + FWCfgState *fw_cfg;
> > unsigned int num_vsimms;
> >
> > /* init CPUs */
> > @@ -1592,7 +1592,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
> > espdma_irq, ledma_irq;
> > qemu_irq esp_reset, dma_enable;
> > unsigned long kernel_size;
> > - void *fw_cfg;
> > + FWCfgState *fw_cfg;
> > DeviceState *dev;
> >
> > /* init CPUs */
> > @@ -1793,7 +1793,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
> > qemu_irq fdc_tc;
> > unsigned long kernel_size;
> > DriveInfo *fd[MAX_FD];
> > - void *fw_cfg;
> > + FWCfgState *fw_cfg;
> > DeviceState *dev;
> > unsigned int i;
> >
> > diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> > index 0d29620..0138252 100644
> > --- a/hw/sparc64/sun4u.c
> > +++ b/hw/sparc64/sun4u.c
> > @@ -818,7 +818,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> > qemu_irq *ivec_irqs, *pbm_irqs;
> > DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> > DriveInfo *fd[MAX_FD];
> > - void *fw_cfg;
> > + FWCfgState *fw_cfg;
> >
> > /* init CPUs */
> > cpu = cpu_devinit(cpu_model, hwdef);
> > --
> > 1.7.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] Purpose of qemu-common.h (was Re: [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients)
2013-04-25 20:57 ` [Qemu-devel] Purpose of qemu-common.h (was Re: [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients) Eduardo Habkost
@ 2013-04-25 21:33 ` Michael S. Tsirkin
0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2013-04-25 21:33 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Laszlo Ersek, qemu-devel, Anthony Liguori
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure
2013-04-25 18:47 ` Anthony Liguori
@ 2013-04-26 9:32 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-26 9:32 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, mst
On 04/25/13 20:47, Anthony Liguori wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>> +typedef struct acpi_table_std_header {
>> + char sig[4]; /* ACPI signature (4 ASCII characters) */
>> + uint32_t length; /* Length of table, in bytes, including header */
>> + uint8_t revision; /* ACPI Specification minor version # */
>> + uint8_t checksum; /* To make sum of entire table == 0 */
>> + char oem_id[6]; /* OEM identification */
>> + char oem_table_id[8]; /* OEM table identification */
>> + uint32_t oem_revision; /* OEM revision number */
>> + char asl_compiler_id[4]; /* ASL compiler vendor ID */
>> + uint32_t asl_compiler_revision; /* ASL compiler revision number */
>> +} QEMU_PACKED AcpiTableStdHdr;
>
> Since you're giving it a CamelCaseName why don't you do the same for the
> struct. After that:
This was on purpose. The original "struct acpi_table_header" that I was
extracting from had lower_case_underscore_separated name. My impression
was that structure tags were named_like_this, while ordinary identifiers
denoting types were NamedLikeThis. I was trying to follow that.
However I can see now that in general that observation was wrong. I'll
fix it.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced
2013-04-25 18:49 ` Anthony Liguori
@ 2013-04-26 9:53 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-26 9:53 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, mst
On 04/25/13 20:49, Anthony Liguori wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
>
>> This enables reuse when preparing per-table fw_cfg blobs later.
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> include/hw/acpi/acpi.h | 2 ++
>> hw/acpi/core.c | 39 ++++++++++++++++++++++++---------------
>> 2 files changed, 26 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
>> index 0e26f63..bc7e107 100644
>> --- a/include/hw/acpi/acpi.h
>> +++ b/include/hw/acpi/acpi.h
>> @@ -178,4 +178,6 @@ typedef struct acpi_table_std_header {
>> char asl_compiler_id[4]; /* ASL compiler vendor ID */
>> uint32_t asl_compiler_revision; /* ASL compiler revision number */
>> } QEMU_PACKED AcpiTableStdHdr;
>> +
>> +extern const AcpiTableStdHdr acpi_dfl_hdr;
>
> Can we do this without using a global variable? I assume this gets
> memcpy()'d or referenced as a pointer later on or something like that?
> Can we make a function that does that?
The data itself is reused between acpi_table_install() -- ie. the
-acpitable switch -- and acpi_table_fill_hdr(), to be added in 7/7. The
latter uses it in a structure assignment.
I can make a function ("acpi_table_fill_dfl_hdr()") that takes a target
buffer and a size, checks the size, and if it fits, copies the default
header into it.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-25 19:03 ` Anthony Liguori
2013-04-25 20:11 ` Eduardo Habkost
@ 2013-04-26 11:13 ` Laszlo Ersek
2013-04-29 8:20 ` Michael S. Tsirkin
1 sibling, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-26 11:13 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Kevin O'Connor, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-26 11:13 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
@ 2013-04-29 8:20 ` Michael S. Tsirkin
2013-04-29 12:39 ` Kevin O'Connor
0 siblings, 1 reply; 32+ messages in thread
From: Michael S. Tsirkin @ 2013-04-29 8:20 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: Kevin O'Connor, qemu-devel, Anthony Liguori
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.
--
MST
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-29 8:20 ` Michael S. Tsirkin
@ 2013-04-29 12:39 ` Kevin O'Connor
2013-04-29 13:21 ` Michael S. Tsirkin
2013-04-29 13:21 ` Laszlo Ersek
0 siblings, 2 replies; 32+ messages in thread
From: Kevin O'Connor @ 2013-04-29 12:39 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Laszlo Ersek, qemu-devel, Anthony Liguori
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-29 12:39 ` Kevin O'Connor
@ 2013-04-29 13:21 ` Michael S. Tsirkin
2013-04-29 13:21 ` Laszlo Ersek
1 sibling, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2013-04-29 13:21 UTC (permalink / raw)
To: Kevin O'Connor; +Cc: Laszlo Ersek, qemu-devel, Anthony Liguori
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.
--
MST
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients
2013-04-29 12:39 ` Kevin O'Connor
2013-04-29 13:21 ` Michael S. Tsirkin
@ 2013-04-29 13:21 ` Laszlo Ersek
1 sibling, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2013-04-29 13:21 UTC (permalink / raw)
To: Kevin O'Connor; +Cc: qemu-devel, Anthony Liguori, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2013-04-29 13:21 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-18 20:22 [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 1/7] refer to FWCfgState explicitly Laszlo Ersek
2013-04-25 18:44 ` Anthony Liguori
2013-04-25 21:04 ` Michael S. Tsirkin
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 2/7] acpi_table_install(): fix funcparam formatting in leading comment Laszlo Ersek
2013-04-25 18:44 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 3/7] hw/acpi: extract standard table headers as a standalone structure Laszlo Ersek
2013-04-25 18:47 ` Anthony Liguori
2013-04-26 9:32 ` Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 4/7] hw/acpi: export default ACPI headers using the type just introduced Laszlo Ersek
2013-04-25 18:49 ` Anthony Liguori
2013-04-26 9:53 ` Laszlo Ersek
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 5/7] hw/acpi: export acpi_checksum() Laszlo Ersek
2013-04-25 18:55 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 6/7] hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h Laszlo Ersek
2013-04-25 18:55 ` Anthony Liguori
2013-04-18 20:22 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
2013-04-18 20:30 ` Michael S. Tsirkin
2013-04-19 10:58 ` Laszlo Ersek
2013-04-24 9:42 ` Michael S. Tsirkin
2013-04-25 19:03 ` Anthony Liguori
2013-04-25 20:11 ` Eduardo Habkost
2013-04-25 20:45 ` Anthony Liguori
2013-04-25 20:57 ` [Qemu-devel] Purpose of qemu-common.h (was Re: [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients) Eduardo Habkost
2013-04-25 21:33 ` Michael S. Tsirkin
2013-04-26 11:13 ` [Qemu-devel] [PATCH v4 7/7] hw/i386: build ACPI MADT (APIC) for fw_cfg clients Laszlo Ersek
2013-04-29 8:20 ` Michael S. Tsirkin
2013-04-29 12:39 ` Kevin O'Connor
2013-04-29 13:21 ` Michael S. Tsirkin
2013-04-29 13:21 ` Laszlo Ersek
2013-04-24 9:39 ` [Qemu-devel] [PATCH v4 0/7] publish etc/acpi/APIC in fw_cfg Laszlo Ersek
2013-04-25 16:45 ` Anthony Liguori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).