Beginning implementing the AMD IOMMU emulation
cover Beginning implementing the AMD IOMMU emulation
Hi everybody,
This patchset is intended to provide a start for implementing the emulation of the AMD IOMMU. For those who aren't aware yet, I intend to participate as a student in GSoC 2010.
The patches are meant to be applied on top of qemu-kvm.
In short, this demonstrates a mechanism of inserting ACPI tables without modifying SeaBIOS or other BIOS implementations. I also have a SeaBIOS equivalent, but I think this approach is better, at least at the moment.
To test, simply boot a Linux kernel inside KVM and look in dmesg for the IVRS table.
I wouldn't merge this patchset yet, at least stuff after the first patch, until it accumulates more work. I also didn't test loading ACPI tables from the command line after these modifications.
I'd appreciate comments on these patches.
Cheers,
Eduard
Eduard - Gabriel Munteanu (7):
acpi: qemu_realloc() might return a different pointer
acpi: split and rename acpi_table_add()
acpi: move table header definition into pc.h
sparc: rename hw/iommu.c
x86-64: AMD IOMMU stub
acpi: cleanup acpi_checksum()
acpi: fix bug in acpi_checksum() caused by garbage in checksum field
Makefile.target | 3 +-
hw/acpi.c | 83 +++++++++++++++++++-------------
hw/amd_iommu.c | 103 +++++++++++++++++++++++++++++++++++++++++
hw/pc.c | 2 +
hw/pc.h | 20 ++++++++-
hw/{iommu.c => sparc_iommu.c} | 0
hw/sun4m.h | 2 +-
vl.c | 2 +-
8 files changed, 177 insertions(+), 38 deletions(-)
create mode 100644 hw/amd_iommu.c
rename hw/{iommu.c => sparc_iommu.c} (100%)RFC PATCH 1/7 acpi: qemu_realloc() might return a different pointer
We mustn't assume qemu_realloc() returns the same pointer in acpi_table_add(). Therefore, 'p' might be invalid if it's relative to the old value of acpi_tables.
--- hw/acpi.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
hw/acpi.c +4 -3
@@ -857,7 +857,7 @@ int acpi_table_add(const char *t)char buf[1024 ], *p, *f;struct acpi_table_header acpi_hdr;unsigned long val; -size_t off; +size_t newlen, off;memset (&acpi_hdr,0 ,sizeof (acpi_hdr));
@@ -938,9 +938,10 @@ int acpi_table_add(const char *t) acpi_tables_len =sizeof (uint16_t ); acpi_tables =qemu_mallocz (acpi_tables_len); } + newlen = acpi_tables_len +sizeof (uint16_t ) + acpi_hdr.length; + acpi_tables =qemu_realloc (acpi_tables, newlen); p = acpi_tables + acpi_tables_len; - acpi_tables_len +=sizeof (uint16_t ) + acpi_hdr.length; - acpi_tables =qemu_realloc (acpi_tables, acpi_tables_len); + acpi_tables_len = newlen; acpi_hdr.length =cpu_to_le32 (acpi_hdr.length); *(uint16_t *)p = acpi_hdr.length;
Signature · 1 line
1.6.4.4
RFC PATCH 2/7 acpi: split and rename acpi_table_add()
In reply to Eduard - Gabriel MunteanuWe'd like to let emulation code build and insert ACPI tables at bootup, without depending on hacking the BIOS code. This will be used to provide an IVRS table for emulating the AMD IOMMU, for instance.
This splits acpi_table_add(), retaining the old behavior of inserting cmdline-supplied tables under the name of acpi_table_cmdline_add(). The other two resulting functions can be used for the aforementioned purpose.
--- hw/acpi.c | 64 ++++++++++++++++++++++++++++++++++++++++--------------------- hw/pc.h | 4 ++- vl.c | 2 +- 3 files changed, 46 insertions(+), 24 deletions(-)
hw/acpi.c +42 -22
@@ -840,7 +840,7 @@ struct acpi_table_header } __attribute__((packed));char *acpi_tables; -size_t acpi_tables_len; +size_t acpi_tables_len, acpi_tables_prev_len;static int acpi_checksum (const uint8_t *data,int len) {
@@ -851,13 +851,44 @@ static int acpi_checksum(const uint8_t *data, int len)return (-sum) &0xff ; } -int acpi_table_add (const char *t) +void *acpi_alloc_table (size_t size) +{ +void *ptr; + +if (!acpi_tables) { + acpi_tables_len =sizeof (uint16_t ); + acpi_tables =qemu_mallocz (acpi_tables_len); + } + acpi_tables_prev_len = acpi_tables_len; + acpi_tables_len +=sizeof (uint16_t ) + size; + acpi_tables =qemu_realloc (acpi_tables, acpi_tables_len); + ptr = acpi_tables + acpi_tables_prev_len; + + *(uint16_t *) ptr = size; + +return ptr +sizeof (uint16_t ); +} + +void acpi_commit_table (void *buf) +{ +struct acpi_table_header *acpi_hdr = buf; +size_t size = acpi_tables_len - acpi_tables_prev_len -sizeof (uint16_t ); + + acpi_hdr->length =cpu_to_le32 (size); + acpi_hdr->checksum =acpi_checksum (buf, size); + +/* increase number of tables */ + (*(uint16_t *) acpi_tables) = +cpu_to_le32 (le32_to_cpu (*(uint16_t *) acpi_tables) +1 ); +} + +int acpi_table_cmdline_add (const char *t) {static const char *dfl_id ="QEMUQEMU" ;char buf[1024 ], *p, *f;struct acpi_table_header acpi_hdr;unsigned long val; -size_t newlen, off; +size_t size, off;memset (&acpi_hdr,0 ,sizeof (acpi_hdr));
@@ -915,7 +946,7 @@ int acpi_table_add(const char *t) buf[0 ] ='\0' ; } - acpi_hdr.length =sizeof (acpi_hdr); + size =sizeof (acpi_hdr); f = buf;while (buf[0 ]) {
@@ -927,27 +958,17 @@ int acpi_table_add(const char *t)fprintf (stderr,"Can't stat file '%s': %s\n" , f,strerror (errno)); goto out; } - acpi_hdr.length += s.st_size; + size += s.st_size;if (!n)break ; *n =':' ; f = n +1 ; } -if (!acpi_tables) { - acpi_tables_len =sizeof (uint16_t ); - acpi_tables =qemu_mallocz (acpi_tables_len); - } - newlen = acpi_tables_len +sizeof (uint16_t ) + acpi_hdr.length; - acpi_tables =qemu_realloc (acpi_tables, newlen); - p = acpi_tables + acpi_tables_len; - acpi_tables_len = newlen; + p =acpi_alloc_table (size); + off =sizeof (struct acpi_table_header ); - acpi_hdr.length =cpu_to_le32 (acpi_hdr.length); - *(uint16_t *)p = acpi_hdr.length; - p +=sizeof (uint16_t ); -memcpy (p, &acpi_hdr,sizeof (acpi_hdr)); - off =sizeof (acpi_hdr); +memcpy (p, &acpi_hdr, off); f = buf;while (buf[0 ]) {
@@ -983,10 +1004,8 @@ int acpi_table_add(const char *t) f = n +1 ; } - ((struct acpi_table_header *)p)->checksum =acpi_checksum ((uint8_t *)p, off); -/* increase number of tables */ - (*(uint16_t *)acpi_tables) = -cpu_to_le32 (le32_to_cpu (*(uint16_t *)acpi_tables) +1 ); +acpi_commit_table (p); +return 0 ; out:if (acpi_tables) {
@@ -995,3 +1014,4 @@ out: }return -1 ; } +
hw/pc.h +3 -1
@@ -108,7 +108,9 @@ extern char *acpi_tables;extern size_t acpi_tables_len;void acpi_bios_init (void ); -int acpi_table_add (const char *table_desc); +void *acpi_alloc_table (size_t size); +void acpi_commit_table (void *buf); +int acpi_table_cmdline_add (const char *table_desc);/* acpi_piix.c */ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
vl.c +1 -1
@@ -5492,7 +5492,7 @@ int main(int argc, char **argv, char **envp) rtc_td_hack =1 ;break ;case QEMU_OPTION_acpitable: -if (acpi_table_add (optarg) <0 ) { +if (acpi_table_cmdline_add (optarg) <0 ) {fprintf (stderr,"Wrong acpi table provided\n" );exit (1 ); }
Signature · 1 line
1.6.4.4
RFC PATCH 3/7 acpi: move table header definition into pc.h
In reply to Eduard - Gabriel MunteanuThis moves the table header definition into pc.h to allow other code to build ACPI tables.
--- hw/acpi.c | 13 ------------- hw/pc.h | 13 +++++++++++++ 2 files changed, 13 insertions(+), 13 deletions(-)
hw/acpi.c +0 -13
@@ -826,19 +826,6 @@ static int piix4_device_hotplug(PCIDevice *dev, int state)return 0 ; } -struct acpi_table_header -{ -char signature [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 */ -} __attribute__((packed)); -char *acpi_tables;size_t acpi_tables_len, acpi_tables_prev_len;
hw/pc.h +13 -0
@@ -103,6 +103,19 @@ int ioport_get_a20(void);CPUState *pc_new_cpu (const char *cpu_model);/* acpi.c */ +struct acpi_table_header +{ +char signature [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 */ +} __attribute__((packed)); +extern int acpi_enabled;extern char *acpi_tables;extern size_t acpi_tables_len;
Signature · 1 line
1.6.4.4
RFC PATCH 4/7 sparc: rename hw/iommu.c
In reply to Eduard - Gabriel Munteanuhw/iommu.c concerns the SPARC IOMMU. However we intend to implement the AMD IOMMU, which could lead to confusion unless we rename the former.
---
Makefile.target | 2 +-
hw/{iommu.c => sparc_iommu.c} | 0
hw/sun4m.h | 2 +-
3 files changed, 2 insertions(+), 2 deletions(-)
rename hw/{iommu.c => sparc_iommu.c} (100%)Makefile.target +1 -1
@@ -305,7 +305,7 @@ obj-sparc-y += vga.o vga-pci.o obj-sparc-y += fdc.o mc146818rtc.o serial.o obj-sparc-y += cirrus_vga.o parallel.o else -obj-sparc-y = sun4m.o lance.o tcx.o iommu.o slavio_intctl.o +obj-sparc-y = sun4m.o lance.o tcx.o sparc_iommu.o slavio_intctl.o obj-sparc-y += slavio_timer.o slavio_misc.o fdc.o sparc32_dma.o obj-sparc-y += cs4231.o eccmemctl.o sbi.o sun4c_intctl.o endif
hw/sparc_iommu.c renamed from hw/iommu.c +0 -0
hw/sun4m.h +1 -1
@@ -5,7 +5,7 @@/* Devices used by sparc32 system. */ -/* iommu.c */ +/* sparc_iommu.c */ void sparc_iommu_memory_rw (void *opaque,target_phys_addr_t addr,uint8_t *buf,int len,int is_write); static inline void sparc_iommu_memory_read(void *opaque,
Signature · 1 line
1.6.4.4
Re: [Qemu-devel] [RFC PATCH 4/7] sparc: rename hw/iommu.c
Quoted 2 lines · Eduard - Gabriel Munteanu
hw/iommu.cconcerns the SPARC IOMMU. However we intend to implement the
AMD IOMMU, which could lead to confusion unless we rename the former.
I was also thinking of renaming the file some time ago. The correct name would be "sun4m_iommu.c". Sun4c (while still Sparc based) had a different architecture (IIRC CPU MMU doubled as IOMMU) and Sun4d had several IO-UNITs instead. All Sun4m machines had an IOMMU.
But the qdev name of the device is still "iommu" and we can't change that. So I'm not so sure it's worth renaming. Can't AMD IOMMU reside in amd_iommu.c?
Re: [Qemu-devel] [RFC PATCH 4/7] sparc: rename hw/iommu.c
Quoted 4 lines · Blue Swirl
Quoted 2 lines · Eduard - Gabriel Munteanu
hw/iommu.cconcerns the SPARC IOMMU. However we intend to implement the
AMD IOMMU, which could lead to confusion unless we rename the former.
Quoted 8 lines
I was also thinking of renaming the file some time ago. The correct name would be "
sun4m_iommu.c". Sun4c (while still Sparc based) had a different architecture (IIRC CPU MMU doubled as IOMMU) and Sun4d had several IO-UNITs instead. All Sun4m machines had an IOMMU.But the qdev name of the device is still "iommu" and we can't change that. So I'm not so sure it's worth renaming. Can't AMD IOMMU reside in
amd_iommu.c?
Keeping the plain name 'iommu' will likely cause confusion when more iommu implementations are added. It is better to rename it so that the name better describes what the file implements. So this change makes sense for me.
Joerg
Re: [Qemu-devel] [RFC PATCH 4/7] sparc: rename hw/iommu.c
Quoted 15 lines · Joerg Roedel
Quoted 4 lines · Blue Swirl
Quoted 2 lines · Eduard - Gabriel Munteanu
hw/iommu.cconcerns the SPARC IOMMU. However we intend to implement the
AMD IOMMU, which could lead to confusion unless we rename the former.Quoted 8 lines
I was also thinking of renaming the file some time ago. The correct name would be "
sun4m_iommu.c". Sun4c (while still Sparc based) had a different architecture (IIRC CPU MMU doubled as IOMMU) and Sun4d had several IO-UNITs instead. All Sun4m machines had an IOMMU.But the qdev name of the device is still "iommu" and we can't change that. So I'm not so sure it's worth renaming. Can't AMD IOMMU reside in
amd_iommu.c?
Quoted 4 lines
Keeping the plain name 'iommu' will likely cause confusion when more
iommu implementations are added. It is better to rename it so that the name better describes what the file implements. So this change makes sense for me.
I see. I'm OK then with "sun4m_iommu.c".
Re: [Qemu-devel] [RFC PATCH 4/7] sparc: rename hw/iommu.c
Quoted 16 lines · Blue Swirl
Quoted 15 lines · Joerg Roedel
Quoted 12 lines · Blue Swirl
Quoted 2 lines · Eduard - Gabriel Munteanu
hw/iommu.cconcerns the SPARC IOMMU. However we intend to implement the
AMD IOMMU, which could lead to confusion unless we rename the former.I was also thinking of renaming the file some time ago. The correct name would be "
sun4m_iommu.c". Sun4c (while still Sparc based) had a different architecture (IIRC CPU MMU doubled as IOMMU) and Sun4d had several IO-UNITs instead. All Sun4m machines had an IOMMU.But the qdev name of the device is still "iommu" and we can't change that. So I'm not so sure it's worth renaming. Can't AMD IOMMU reside in
amd_iommu.c?
Quoted 6 lines
Quoted 4 lines
Keeping the plain name 'iommu' will likely cause confusion when more
iommu implementations are added. It is better to rename it so that the name better describes what the file implements. So this change makes sense for me.I see. I'm OK then with "
sun4m_iommu.c".
Yes, I think it's enough to just change the filename, since multiple such devices aren't likely to conflict, in any configuration.
Then "sun4m_iommu.c" it is. Will resubmit with the next patchset.
Eduard
Re: [Qemu-devel] [RFC PATCH 4/7] sparc: rename hw/iommu.c
In reply to Blue SwirlQuoted 4 lines · Blue Swirl
Quoted 2 lines · Eduard - Gabriel Munteanu
hw/iommu.cconcerns the SPARC IOMMU. However we intend to implement the
AMD IOMMU, which could lead to confusion unless we rename the former.
Quoted 8 lines
I was also thinking of renaming the file some time ago. The correct name would be "
sun4m_iommu.c". Sun4c (while still Sparc based) had a different architecture (IIRC CPU MMU doubled as IOMMU) and Sun4d had several IO-UNITs instead. All Sun4m machines had an IOMMU.But the qdev name of the device is still "iommu" and we can't change that. So I'm not so sure it's worth renaming. Can't AMD IOMMU reside in
amd_iommu.c?
I'd go for the (filename) rename. The qdev name shouldn't cause conflicts due to the different targets.
cheers,
Gerd
RFC PATCH 5/7 x86-64: AMD IOMMU stub
In reply to Eduard - Gabriel MunteanuThis currently loads a non-functional IVRS ACPI table and provides a skeleton for initializing the AMD IOMMU.
--- Makefile.target | 1 + hw/amd_iommu.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/pc.c | 2 + hw/pc.h | 3 ++ 4 files changed, 109 insertions(+), 0 deletions(-) create mode 100644 hw/amd_iommu.c
Makefile.target +1 -0
@@ -230,6 +230,7 @@ obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o obj-i386-y += extboot.o obj-i386-y += ne2000-isa.o debugcon.o multiboot.o obj-i386-y += testdev.o +obj-i386-y += amd_iommu.o obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o obj-i386-$(CONFIG_KVM_DEVICE_ASSIGNMENT) += device-assignment.o
hw/amd_iommu.c new file +103 -0
103 lines not on this page · raw message
hw/pc.c +2 -0
@@ -906,6 +906,8 @@ static void pc_init1(ram_addr_t ram_size,cpu_register_physical_memory ((uint32_t )(-bios_size), bios_size, bios_offset |IO_MEM_ROM ); +amd_iommu_init (); + fw_cfg =bochs_bios_init ();rom_set_fw (fw_cfg);
hw/pc.h +3 -0
@@ -194,4 +194,7 @@ int cpu_is_bsp(CPUState *env);int e820_add_entry (uint64_t ,uint64_t ,uint32_t ); +/* amd_iommu.c */ +int amd_iommu_init (void ); +#endif
Signature · 1 line
1.6.4.4
Re: [RFC PATCH 5/7] x86-64: AMD IOMMU stub
Quoted 106 lines · Eduard - Gabriel Munteanu
This currently loads a non-functional IVRS ACPI table and provides a
skeleton for initializing the AMD IOMMU.Signed-off-by Eduard - Gabriel Munteanu <[email protected]>---
Makefile.target | 1 + hw/amd_iommu.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/pc.c | 2 + hw/pc.h | 3 ++ 4 files changed, 109 insertions(+), 0 deletions(-)create mode 100644
hw/amd_iommu.cdiff --git a/Makefile.target b/Makefile.target
index cbe19a6..dfa4652 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -230,6 +230,7 @@ obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
obj-i386-y += extboot.o
obj-i386-y += ne2000-isa.o debugcon.o multiboot.o
obj-i386-y += testdev.o
+obj-i386-y += amd_iommu.oobj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o obj-i386-$(CONFIG_KVM_DEVICE_ASSIGNMENT) += device-assignment.o
diff --gita/hw/amd_iommu.cb/hw/amd_iommu.c
new file mode 100644
index 0000000..b502430
---/dev/null
+++b/hw/amd_iommu.c
@@ -0,0 +1,103 @@
+/*
+ * AMD IOMMU emulation
+ *
+ * Copyright (c) 2010 Eduard - Gabriel Munteanu <[email protected]>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * 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.
+ */
+
+/*
+ * IVRS (I/O Virtualization Reporting Structure) table.
+ *
+ * Describes the AMD IOMMU, as per:
+ * "AMD I/O Virtualization Technology (IOMMU) Specification", rev 1.26
+ */
+
+#include <stdint.h>
+
+#include "pc.h"
+
+structivrs_ivhd
+{+ uint8_t type; + uint8_t flags; + uint16_t length; + uint16_t devid; + uint16_t capab_off; + uint64_t iommu_base_addr; + uint16_t pci_seg_group; + uint16_t iommu_info; + uint32_t reserved; + uint32_t entry; +} __attribute__ ((__packed__)); + +struct ivrs_table +{ + struct acpi_table_header acpi_hdr; + uint32_t iv_info; + uint32_t reserved[2]; + struct ivrs_ivhd ivhd; +} __attribute__ ((__packed__)); + +static const char ivrs_sig[] = "IVRS"; +static const char dfl_id[] = "QEMUQEMU"; + +static void amd_iommu_init_ivrs(void) +{ + int ivrs_size = sizeof(struct ivrs_table); + struct ivrs_table *ivrs; + struct ivrs_ivhd *ivhd; + struct acpi_table_header *acpi_hdr; + + ivrs = acpi_alloc_table(ivrs_size); + acpi_hdr = &ivrs->acpi_hdr; + ivhd = &ivrs->ivhd; + + ivrs->iv_info = (64 << 15) | /* Virtual address space size. */ + (48 << 8); /* Physical address space size. */ + + ivhd->type = 0x10; + ivhd->flags = 0;
Quoted 8 lines
+ ivhd->length = sizeof(struct ivrs_ivhd); + ivhd->devid = 0; + ivhd->capab_off = 0; + ivhd->iommu_base_addr = 0; + ivhd->pci_seg_group = 0; + ivhd->iommu_info = 0; + ivhd->reserved = 0; + ivhd->entry = 0;
The table would be filled with wrong data on big endian host. Please use cpu_to_le64/32/16.
Quoted 50 lines
+ strncpy(acpi_hdr->signature, ivrs_sig, 4); + acpi_hdr->revision = 1; + strncpy(acpi_hdr->oem_id, dfl_id, 6); + strncpy(acpi_hdr->oem_table_id, dfl_id, 8); + acpi_hdr->oem_revision = 1; + strncpy(acpi_hdr->asl_compiler_id, dfl_id, 4); + acpi_hdr->asl_compiler_revision = 1; + + acpi_commit_table(ivrs); +} + +int amd_iommu_init(void) +{ + amd_iommu_init_ivrs(); + + return 0; +} + diff --git a/hw/pc.c b/hw/pc.c index 0aebae9..89a7a30 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -906,6 +906,8 @@ static void pc_init1(ram_addr_t ram_size, cpu_register_physical_memory((uint32_t)(-bios_size), bios_size, bios_offset | IO_MEM_ROM);+
amd_iommu_init();
+fw_cfg = bochs_bios_init(); rom_set_fw(fw_cfg);diff --git
a/hw/pc.hb/hw/pc.h
index 92954db..b91300e 100644
---a/hw/pc.h
+++b/hw/pc.h
@@ -194,4 +194,7 @@ intcpu_is_bsp(CPUState *env);int e820_add_entry(uint64_t, uint64_t, uint32_t);+/*
amd_iommu.c*/
+intamd_iommu_init(void);
+
#endif--
1.6.4.4
RFC PATCH 6/7 acpi: cleanup acpi_checksum()
In reply to Eduard - Gabriel MunteanuThis adds newlines in acpi_checksum() to separate the declarations, the body and the return statement.
--- hw/acpi.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
hw/acpi.c +2 -0
@@ -832,9 +832,11 @@ size_t acpi_tables_len, acpi_tables_prev_len;static int acpi_checksum (const uint8_t *data,int len) {int sum, i; + sum =0 ;for (i =0 ; i < len; i++) sum += data[i]; +return (-sum) &0xff ; }
Signature · 1 line
1.6.4.4
RFC PATCH 7/7 acpi: fix bug in acpi_checksum() caused by garbage in checksum field
In reply to Eduard - Gabriel MunteanuThe whole table must sum to zero. We need to ignore garbage in the checksum field (i.e. consider it zero) when checksumming. It is legitimate to have garbage there, as the checksum makes sense only when the table has been filled.
--- hw/acpi.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
hw/acpi.c +5 -0
@@ -832,11 +832,16 @@ size_t acpi_tables_len, acpi_tables_prev_len;static int acpi_checksum (const uint8_t *data,int len) {int sum, i; +struct acpi_table_header *acpi_hdr; sum =0 ;for (i =0 ; i < len; i++) sum += data[i]; +/* Ignore preexisting garbage in checksum. */ + acpi_hdr = (struct acpi_table_header *) data; + sum -= acpi_hdr->checksum; +return (-sum) &0xff ; }
Signature · 1 line
1.6.4.4
Re: [Qemu-devel] [RFC PATCH 7/7] acpi: fix bug in acpi_checksum() caused by garbage in checksum field
hw/acpi.c size_t acpi_tables_len, acpi_tables_prev_len; · line 845
Quoted 5 lines · Eduard - Gabriel Munteanu
hw/acpi.c +4
-0+/* Ignore preexisting garbage in checksum. */ + acpi_hdr = (struct acpi_table_header *) data; + sum -= acpi_hdr->checksum; +return (-sum) &0xff ;
Wouldn't it be cleaner to adjust the acpi_checksum definition to take and acpi_table_header operand instead of uint8_t? And given it's only usage, perhaps update the checksum instead of returning it? E.g.
- ((struct acpi_table_header*)p)->checksum = acpi_checksum((uint8_t*)p, off);
+ acpi_update_checksum((struct acpi_table_header *)p, off);
r~
Hello Eduard,
Quoted 3 lines · Eduard - Gabriel Munteanu
This patchset is intended to provide a start for implementing the emulation of the AMD IOMMU. For those who aren't aware yet, I intend to participate as a student in GSoC 2010.
Great. This is a good start.
Quoted 3 lines
In short, this demonstrates a mechanism of inserting ACPI tables without modifying SeaBIOS or other BIOS implementations. I also have a SeaBIOS equivalent, but I think this approach is better, at least at the moment.
I like the approach implemented in this patchset because of its simplicity. The right place for building acpi tables is the bios, though. I am fine with both ways. Anthony, Avi, what do you think about it?
Quoted 3 lines
I wouldn't merge this patchset yet, at least stuff after the first patch, until it accumulates more work. I also didn't test loading ACPI tables from the command line after these modifications.
When Linux finds an IVRS table it expects that there is a working AMD IOMMU so you should change this patchset so that the ACPI table is enabled later when the hardware emulation is working. That will keep this work bisectable.
As the next step I suggest you to implement an AMD IOMMU pci device with the config space, capability and the mmio register space.
Regards,
Joerg
Quoted 10 lines · Joerg Roedel
Quoted 5 lines
In short, this demonstrates a mechanism of inserting ACPI tables without modifying SeaBIOS or other BIOS implementations. I also have a SeaBIOS equivalent, but I think this approach is better, at least at the moment.
I like the approach implemented in this patchset because of its simplicity. The right place for building acpi tables is the bios, though. I am fine with both ways. Anthony, Avi, what do you think about it?
I agree. Let qemu emulate the hardware, build the tables in the BIOS.
Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.