* [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default
@ 2013-07-28 7:29 Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h Igor Mammedov
` (5 more replies)
0 siblings, 6 replies; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
It turns out that some 32 bit windows guests crash
if 64 bit PCI hole size is >2G.
Limit it to 2G for piix and q35 by default.
v1-v2:
* redone using QOM properties to pass value around
git-tree for testing:
https://github.com/imammedo/qemu/commits/pcihole64_fix_v2
Igor Mammedov (5):
pc: add I440FX QOM cast macro
utils: add range_size() wrapper
pc: replace i440fx_common_init() with i440fx_init() as it isn't used
by anywhere else
pc: add Q35 to QOM composition tree under /machine
pc: limit 64 bit hole to 2G by default
Michael S. Tsirkin (1):
pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h
hw/i386/pc.c | 61 ++++++++++++++++++-------------
hw/i386/pc_piix.c | 14 +-------
hw/i386/pc_q35.c | 2 +-
hw/pci-host/piix.c | 91 ++++++++++++++++++++++++++---------------------
hw/pci-host/q35.c | 32 ++++++++++-------
include/hw/i386/ioapic.h | 1 +
include/hw/i386/pc.h | 5 ++-
include/hw/pci-host/q35.h | 1 +
include/qemu/range.h | 6 ++++
9 files changed, 119 insertions(+), 94 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
@ 2013-07-28 7:29 ` Igor Mammedov
2013-07-28 9:54 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro Igor Mammedov
` (4 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
From: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/pc.c | 2 --
include/hw/i386/ioapic.h | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2a87563..b0b98a8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -75,8 +75,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 {
diff --git a/include/hw/i386/ioapic.h b/include/hw/i386/ioapic.h
index 86e63da..6245388 100644
--- a/include/hw/i386/ioapic.h
+++ b/include/hw/i386/ioapic.h
@@ -21,6 +21,7 @@
#define HW_IOAPIC_H
#define IOAPIC_NUM_PINS 24
+#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
void ioapic_eoi_broadcast(int vector);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h Igor Mammedov
@ 2013-07-28 7:29 ` Igor Mammedov
2013-07-28 9:57 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 3/6] utils: add range_size() wrapper Igor Mammedov
` (3 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/pci-host/piix.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 3908860..bf879e7 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -38,6 +38,10 @@
* http://download.intel.com/design/chipsets/datashts/29054901.pdf
*/
+#define TYPE_I440FX_PCI_HOST "i440FX-pcihost"
+#define I440FX_PCI_HOST(obj) \
+ OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST)
+
typedef struct I440FXState {
PCIHostState parent_obj;
} I440FXState;
@@ -257,7 +261,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
PCII440FXState *f;
unsigned i;
- dev = qdev_create(NULL, "i440FX-pcihost");
+ dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
s = PCI_HOST_BRIDGE(dev);
b = pci_bus_new(dev, NULL, pci_address_space,
address_space_io, 0, TYPE_PCI_BUS);
@@ -661,7 +665,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo i440fx_pcihost_info = {
- .name = "i440FX-pcihost",
+ .name = TYPE_I440FX_PCI_HOST,
.parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(I440FXState),
.instance_init = i440fx_pcihost_initfn,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 3/6] utils: add range_size() wrapper
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro Igor Mammedov
@ 2013-07-28 7:29 ` Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else Igor Mammedov
` (2 subsequent siblings)
5 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/qemu/range.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/qemu/range.h b/include/qemu/range.h
index b76cc0d..19b742c 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -42,4 +42,10 @@ static inline int ranges_overlap(uint64_t first1, uint64_t len1,
return !(last2 < first1 || last1 < first2);
}
+/* Get range size */
+static inline uint64_t range_size(Range r)
+{
+ return r.end - r.begin;
+}
+
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
` (2 preceding siblings ...)
2013-07-28 7:29 ` [Qemu-devel] [PATCH 3/6] utils: add range_size() wrapper Igor Mammedov
@ 2013-07-28 7:29 ` Igor Mammedov
2013-07-28 10:07 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default Igor Mammedov
5 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/pci-host/piix.c | 50 +++++++++++++-------------------------------------
1 file changed, 13 insertions(+), 37 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index bf879e7..4624d04 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -239,19 +239,18 @@ static int i440fx_initfn(PCIDevice *dev)
return 0;
}
-static PCIBus *i440fx_common_init(const char *device_name,
- PCII440FXState **pi440fx_state,
- int *piix3_devfn,
- ISABus **isa_bus, qemu_irq *pic,
- MemoryRegion *address_space_mem,
- MemoryRegion *address_space_io,
- ram_addr_t ram_size,
- hwaddr pci_hole_start,
- hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
- MemoryRegion *pci_address_space,
- MemoryRegion *ram_memory)
+PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
+ int *piix3_devfn,
+ ISABus **isa_bus, qemu_irq *pic,
+ MemoryRegion *address_space_mem,
+ MemoryRegion *address_space_io,
+ ram_addr_t ram_size,
+ hwaddr pci_hole_start,
+ hwaddr pci_hole_size,
+ hwaddr pci_hole64_start,
+ hwaddr pci_hole64_size,
+ MemoryRegion *pci_address_space,
+ MemoryRegion *ram_memory)
{
DeviceState *dev;
PCIBus *b;
@@ -269,7 +268,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
qdev_init_nofail(dev);
- d = pci_create_simple(b, 0, device_name);
+ d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE);
*pi440fx_state = I440FX_PCI_DEVICE(d);
f = *pi440fx_state;
f->system_memory = address_space_mem;
@@ -330,29 +329,6 @@ static PCIBus *i440fx_common_init(const char *device_name,
return b;
}
-PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
- ISABus **isa_bus, qemu_irq *pic,
- MemoryRegion *address_space_mem,
- MemoryRegion *address_space_io,
- ram_addr_t ram_size,
- hwaddr pci_hole_start,
- hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
- MemoryRegion *pci_memory, MemoryRegion *ram_memory)
-
-{
- PCIBus *b;
-
- b = i440fx_common_init(TYPE_I440FX_PCI_DEVICE, pi440fx_state,
- piix3_devfn, isa_bus, pic,
- address_space_mem, address_space_io, ram_size,
- pci_hole_start, pci_hole_size,
- pci_hole64_start, pci_hole64_size,
- pci_memory, ram_memory);
- return b;
-}
-
/* PIIX3 PCI to ISA bridge */
static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
{
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
` (3 preceding siblings ...)
2013-07-28 7:29 ` [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else Igor Mammedov
@ 2013-07-28 7:29 ` Igor Mammedov
2013-07-28 10:08 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default Igor Mammedov
5 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/pc_q35.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0b1d2e3..5140caf 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -130,7 +130,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
/* create pci host bus */
q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
-
+ object_property_add_child(qdev_get_machine(), "q35", OBJECT(q35_host), NULL);
q35_host->mch.ram_memory = ram_memory;
q35_host->mch.pci_address_space = pci_memory;
q35_host->mch.system_memory = get_system_memory();
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
` (4 preceding siblings ...)
2013-07-28 7:29 ` [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine Igor Mammedov
@ 2013-07-28 7:29 ` Igor Mammedov
2013-07-28 7:57 ` Michael S. Tsirkin
5 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 7:29 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
It turns out that some 32 bit windows guests crash
if 64 bit PCI hole size is >2G.
Limit it to 2G for piix and q35 by default.
User may override default boundaries by using
"pci_hole64_end " property.
Examples:
-global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
-global q35-pcihost.pci_hole64_end=0xffffffffffffffff
Reported-by: Igor Mammedov <imammedo@redhat.com>,
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
hw/i386/pc_piix.c | 14 +----------
hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
hw/pci-host/q35.c | 32 +++++++++++++++----------
include/hw/i386/pc.h | 5 ++--
include/hw/pci-host/q35.h | 1 +
6 files changed, 94 insertions(+), 54 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b0b98a8..acaeb6c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
{
PcRomPciInfo *info;
+ Object *pci_info;
+
if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
return;
}
+ pci_info = object_resolve_path("/machine/i440fx", NULL);
+ if (!pci_info) {
+ pci_info = object_resolve_path("/machine/q35", NULL);
+ if (!pci_info) {
+ return;
+ }
+ }
info = g_malloc(sizeof *info);
- info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
- info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
- info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
- info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
+ info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
+ "pci_hole_start", NULL));
+ info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
+ "pci_hole_end", NULL));
+ info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
+ "pci_hole64_start", NULL));
+ info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
+ "pci_hole64_end", NULL));
/* Pass PCI hole info to guest via a side channel.
* Required so guest PCI enumeration does the right thing. */
fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
@@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
PcGuestInfo *guest_info = &guest_info_state->info;
- guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
- if (sizeof(hwaddr) == 4) {
- guest_info->pci_info.w64.begin = 0;
- guest_info->pci_info.w64.end = 0;
- } else {
- /*
- * BIOS does not set MTRR entries for the 64 bit window, so no need to
- * align address to power of two. Align address at 1G, this makes sure
- * it can be exactly covered with a PAT entry even when using huge
- * pages.
- */
- guest_info->pci_info.w64.begin =
- ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
- guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
- (0x1ULL << 62);
- assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
- }
-
guest_info_state->machine_done.notify = pc_guest_info_machine_done;
qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
return guest_info;
}
+void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
+{
+ if (sizeof(hwaddr) == 4) {
+ memset(&pci_info->w64, sizeof(pci_info->w64), 0);
+ return;
+ }
+ /*
+ * BIOS does not set MTRR entries for the 64 bit window, so no need to
+ * align address to power of two. Align address at 1G, this makes sure
+ * it can be exactly covered with a PAT entry even when using huge
+ * pages.
+ */
+ pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
+ if (!pci_info->w64.end) {
+ /* set default end if it wasn't specified, + 2 Gb past start */
+ pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
+ }
+ assert(pci_info->w64.begin < pci_info->w64.end);
+}
+
void pc_acpi_init(const char *default_dsdt)
{
char *filename;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b58c255..ab25458 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
guest_info->has_pci_info = has_pci_info;
- /* Set PCI window size the way seabios has always done it. */
- /* Power of 2 so bios can cover it with a single MTRR */
- if (ram_size <= 0x80000000)
- guest_info->pci_info.w32.begin = 0x80000000;
- else if (ram_size <= 0xc0000000)
- guest_info->pci_info.w32.begin = 0xc0000000;
- else
- guest_info->pci_info.w32.begin = 0xe0000000;
-
/* allocate ram and load rom/bios */
if (!xen_enabled()) {
fw_cfg = pc_memory_init(system_memory,
@@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
system_memory, system_io, ram_size,
below_4g_mem_size,
0x100000000ULL - below_4g_mem_size,
- 0x100000000ULL + above_4g_mem_size,
- (sizeof(hwaddr) == 4
- ? 0
- : ((uint64_t)1 << 62)),
+ above_4g_mem_size,
pci_memory, ram_memory);
} else {
pci_bus = NULL;
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 4624d04..59a42c5 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -32,6 +32,7 @@
#include "hw/xen/xen.h"
#include "hw/pci-host/pam.h"
#include "sysemu/sysemu.h"
+#include "hw/i386/ioapic.h"
/*
* I440FX chipset data sheet.
@@ -44,6 +45,7 @@
typedef struct I440FXState {
PCIHostState parent_obj;
+ PcPciInfo pci_info;
} I440FXState;
#define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
@@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
ram_addr_t ram_size,
hwaddr pci_hole_start,
hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
+ ram_addr_t above_4g_mem_size,
MemoryRegion *pci_address_space,
MemoryRegion *ram_memory)
{
@@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
PIIX3State *piix3;
PCII440FXState *f;
unsigned i;
+ I440FXState *i440fx;
+ uint64_t pci_hole64_size;
dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
s = PCI_HOST_BRIDGE(dev);
@@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
f->system_memory = address_space_mem;
f->pci_address_space = pci_address_space;
f->ram_memory = ram_memory;
+
+ i440fx = I440FX_PCI_HOST(dev);
+ /* Set PCI window size the way seabios has always done it. */
+ /* Power of 2 so bios can cover it with a single MTRR */
+ if (ram_size <= 0x80000000) {
+ i440fx->pci_info.w32.begin = 0x80000000;
+ } else if (ram_size <= 0xc0000000) {
+ i440fx->pci_info.w32.begin = 0xc0000000;
+ } else {
+ i440fx->pci_info.w32.begin = 0xe0000000;
+ }
+
memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
pci_hole_start, pci_hole_size);
memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
+
+ pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
+ pci_hole64_size = range_size(i440fx->pci_info.w64);
memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
f->pci_address_space,
- pci_hole64_start, pci_hole64_size);
+ i440fx->pci_info.w64.begin, pci_hole64_size);
if (pci_hole64_size) {
- memory_region_add_subregion(f->system_memory, pci_hole64_start,
+ memory_region_add_subregion(f->system_memory,
+ i440fx->pci_info.w64.begin,
&f->pci_hole_64bit);
}
memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
@@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
return "0000";
}
+static Property i440fx_props[] = {
+ DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
+ DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
+ DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
+ DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
+ IO_APIC_DEFAULT_ADDRESS),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
dc->realize = i440fx_pcihost_realize;
dc->fw_name = "pci";
dc->no_user = 1;
+ dc->props = i440fx_props;
}
static const TypeInfo i440fx_pcihost_info = {
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 6b1b3b7..a6936e6 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
static Property mch_props[] = {
DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
+ DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
+ mch.pci_info.w64.begin, 0),
+ DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
+ mch.pci_info.w64.end, 0),
+ /* Leave enough space for the biggest MCFG BAR */
+ /* TODO: this matches current bios behaviour, but
+ * it's not a power of two, which means an MTRR
+ * can't cover it exactly.
+ */
+ DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
+ mch.pci_info.w32.begin,
+ MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
+ MCH_HOST_BRIDGE_PCIEXBAR_MAX),
+ DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
+ mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
DEFINE_PROP_END_OF_LIST(),
};
@@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
hwaddr pci_hole64_size;
MCHPCIState *mch = MCH_PCI_DEVICE(d);
- /* Leave enough space for the biggest MCFG BAR */
- /* TODO: this matches current bios behaviour, but
- * it's not a power of two, which means an MTRR
- * can't cover it exactly.
- */
- mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
- MCH_HOST_BRIDGE_PCIEXBAR_MAX;
-
/* setup pci memory regions */
memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
mch->pci_address_space,
@@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
0x100000000ULL - mch->below_4g_mem_size);
memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
&mch->pci_hole);
- pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
- ((uint64_t)1 << 62));
+
+ pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size);
+ pci_hole64_size = range_size(mch->pci_info.w64);
memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
mch->pci_address_space,
- 0x100000000ULL + mch->above_4g_mem_size,
+ mch->pci_info.w64.begin,
pci_hole64_size);
if (pci_hole64_size) {
memory_region_add_subregion(mch->system_memory,
- 0x100000000ULL + mch->above_4g_mem_size,
+ mch->pci_info.w64.begin,
&mch->pci_hole_64bit);
}
/* smram */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7fb97b0..ab747b7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -18,7 +18,6 @@ typedef struct PcPciInfo {
} PcPciInfo;
struct PcGuestInfo {
- PcPciInfo pci_info;
bool has_pci_info;
FWCfgState *fw_cfg;
};
@@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
ram_addr_t above_4g_mem_size);
+void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
FWCfgState *pc_memory_init(MemoryRegion *system_memory,
const char *kernel_filename,
@@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
ram_addr_t ram_size,
hwaddr pci_hole_start,
hwaddr pci_hole_size,
- hwaddr pci_hole64_start,
- hwaddr pci_hole64_size,
+ ram_addr_t above_4g_mem_size,
MemoryRegion *pci_memory,
MemoryRegion *ram_memory);
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 3cb631e..3a9e04b 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -55,6 +55,7 @@ typedef struct MCHPCIState {
MemoryRegion smram_region;
MemoryRegion pci_hole;
MemoryRegion pci_hole_64bit;
+ PcPciInfo pci_info;
uint8_t smm_enabled;
ram_addr_t below_4g_mem_size;
ram_addr_t above_4g_mem_size;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 7:29 ` [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default Igor Mammedov
@ 2013-07-28 7:57 ` Michael S. Tsirkin
2013-07-28 8:21 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Michael S. Tsirkin @ 2013-07-28 7:57 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> It turns out that some 32 bit windows guests crash
> if 64 bit PCI hole size is >2G.
> Limit it to 2G for piix and q35 by default.
> User may override default boundaries by using
> "pci_hole64_end " property.
>
> Examples:
> -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
>
> -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
IMO that's pretty bad as user interfaces go.
In particular if you are not careful you can make
end < start.
Why not set the size, and include a patch that
let people write "1G" or "2G" for convenience?
> Reported-by: Igor Mammedov <imammedo@redhat.com>,
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> hw/i386/pc_piix.c | 14 +----------
> hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> hw/pci-host/q35.c | 32 +++++++++++++++----------
> include/hw/i386/pc.h | 5 ++--
> include/hw/pci-host/q35.h | 1 +
> 6 files changed, 94 insertions(+), 54 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b0b98a8..acaeb6c 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> {
> PcRomPciInfo *info;
> + Object *pci_info;
> +
> if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> return;
> }
> + pci_info = object_resolve_path("/machine/i440fx", NULL);
> + if (!pci_info) {
> + pci_info = object_resolve_path("/machine/q35", NULL);
> + if (!pci_info) {
> + return;
> + }
> + }
So is the path /machine/i440fx? /machine/i440FX?
/machine/i440FX-pcihost?
There's no way to check this code is right without
actually running it.
How about i44fx_get_pci_info so we can have this
knowledge of paths localized in specific chipset code?
>
> info = g_malloc(sizeof *info);
> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> + "pci_hole_start", NULL));
> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> + "pci_hole_end", NULL));
Looks wrong.
object_property_get_int returns a signed int64.
w32 is unsigned.
Happens to work but I think we need an explicit API.
Property names are hard-coded string literals.
Please add macros to set and get them
so we can avoid duplicating code.
E.g.
#define PCI_HOST_PROPS...
static inline get_
> + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> + "pci_hole64_start", NULL));
> + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> + "pci_hole64_end", NULL));
> /* Pass PCI hole info to guest via a side channel.
> * Required so guest PCI enumeration does the right thing. */
> fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> @@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> PcGuestInfo *guest_info = &guest_info_state->info;
>
> - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> - if (sizeof(hwaddr) == 4) {
> - guest_info->pci_info.w64.begin = 0;
> - guest_info->pci_info.w64.end = 0;
> - } else {
> - /*
> - * BIOS does not set MTRR entries for the 64 bit window, so no need to
> - * align address to power of two. Align address at 1G, this makes sure
> - * it can be exactly covered with a PAT entry even when using huge
> - * pages.
> - */
> - guest_info->pci_info.w64.begin =
> - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> - (0x1ULL << 62);
> - assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
> - }
> -
> guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> return guest_info;
> }
>
> +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
> +{
> + if (sizeof(hwaddr) == 4) {
> + memset(&pci_info->w64, sizeof(pci_info->w64), 0);
> + return;
> + }
> + /*
> + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> + * align address to power of two. Align address at 1G, this makes sure
> + * it can be exactly covered with a PAT entry even when using huge
> + * pages.
> + */
> + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> + if (!pci_info->w64.end) {
> + /* set default end if it wasn't specified, + 2 Gb past start */
> + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
> + }
> + assert(pci_info->w64.begin < pci_info->w64.end);
> +}
> +
> void pc_acpi_init(const char *default_dsdt)
> {
> char *filename;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b58c255..ab25458 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> guest_info->has_pci_info = has_pci_info;
>
> - /* Set PCI window size the way seabios has always done it. */
> - /* Power of 2 so bios can cover it with a single MTRR */
> - if (ram_size <= 0x80000000)
> - guest_info->pci_info.w32.begin = 0x80000000;
> - else if (ram_size <= 0xc0000000)
> - guest_info->pci_info.w32.begin = 0xc0000000;
> - else
> - guest_info->pci_info.w32.begin = 0xe0000000;
> -
> /* allocate ram and load rom/bios */
> if (!xen_enabled()) {
> fw_cfg = pc_memory_init(system_memory,
> @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> system_memory, system_io, ram_size,
> below_4g_mem_size,
> 0x100000000ULL - below_4g_mem_size,
> - 0x100000000ULL + above_4g_mem_size,
> - (sizeof(hwaddr) == 4
> - ? 0
> - : ((uint64_t)1 << 62)),
> + above_4g_mem_size,
> pci_memory, ram_memory);
> } else {
> pci_bus = NULL;
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 4624d04..59a42c5 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -32,6 +32,7 @@
> #include "hw/xen/xen.h"
> #include "hw/pci-host/pam.h"
> #include "sysemu/sysemu.h"
> +#include "hw/i386/ioapic.h"
>
> /*
> * I440FX chipset data sheet.
> @@ -44,6 +45,7 @@
>
> typedef struct I440FXState {
> PCIHostState parent_obj;
> + PcPciInfo pci_info;
> } I440FXState;
>
> #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> @@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> ram_addr_t ram_size,
> hwaddr pci_hole_start,
> hwaddr pci_hole_size,
> - hwaddr pci_hole64_start,
> - hwaddr pci_hole64_size,
> + ram_addr_t above_4g_mem_size,
> MemoryRegion *pci_address_space,
> MemoryRegion *ram_memory)
> {
> @@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> PIIX3State *piix3;
> PCII440FXState *f;
> unsigned i;
> + I440FXState *i440fx;
> + uint64_t pci_hole64_size;
>
> dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> s = PCI_HOST_BRIDGE(dev);
> @@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> f->system_memory = address_space_mem;
> f->pci_address_space = pci_address_space;
> f->ram_memory = ram_memory;
> +
> + i440fx = I440FX_PCI_HOST(dev);
> + /* Set PCI window size the way seabios has always done it. */
> + /* Power of 2 so bios can cover it with a single MTRR */
> + if (ram_size <= 0x80000000) {
> + i440fx->pci_info.w32.begin = 0x80000000;
> + } else if (ram_size <= 0xc0000000) {
> + i440fx->pci_info.w32.begin = 0xc0000000;
> + } else {
> + i440fx->pci_info.w32.begin = 0xe0000000;
> + }
> +
> memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> pci_hole_start, pci_hole_size);
> memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> +
> + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
> + pci_hole64_size = range_size(i440fx->pci_info.w64);
> memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> f->pci_address_space,
> - pci_hole64_start, pci_hole64_size);
> + i440fx->pci_info.w64.begin, pci_hole64_size);
> if (pci_hole64_size) {
> - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> + memory_region_add_subregion(f->system_memory,
> + i440fx->pci_info.w64.begin,
> &f->pci_hole_64bit);
> }
> memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> return "0000";
> }
>
> +static Property i440fx_props[] = {
> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> + IO_APIC_DEFAULT_ADDRESS),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
So we have 4 properties. One of them pci_hole64_end
is supposed to be set to a value.
Others should not be touched under any circuimstances.
Of course if you query properties you have no way
to know which is which and what are the legal values.
Ouch.
> static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> dc->realize = i440fx_pcihost_realize;
> dc->fw_name = "pci";
> dc->no_user = 1;
> + dc->props = i440fx_props;
> }
>
> static const TypeInfo i440fx_pcihost_info = {
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 6b1b3b7..a6936e6 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
> static Property mch_props[] = {
> DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
> MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
> + mch.pci_info.w64.begin, 0),
> + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
> + mch.pci_info.w64.end, 0),
> + /* Leave enough space for the biggest MCFG BAR */
> + /* TODO: this matches current bios behaviour, but
> + * it's not a power of two, which means an MTRR
> + * can't cover it exactly.
> + */
> + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
> + mch.pci_info.w32.begin,
> + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> + MCH_HOST_BRIDGE_PCIEXBAR_MAX),
> + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
> + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
> hwaddr pci_hole64_size;
> MCHPCIState *mch = MCH_PCI_DEVICE(d);
>
> - /* Leave enough space for the biggest MCFG BAR */
> - /* TODO: this matches current bios behaviour, but
> - * it's not a power of two, which means an MTRR
> - * can't cover it exactly.
> - */
> - mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> - MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> -
> /* setup pci memory regions */
> memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
> mch->pci_address_space,
> @@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
> 0x100000000ULL - mch->below_4g_mem_size);
> memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
> &mch->pci_hole);
> - pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> - ((uint64_t)1 << 62));
> +
> + pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size);
> + pci_hole64_size = range_size(mch->pci_info.w64);
> memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
> mch->pci_address_space,
> - 0x100000000ULL + mch->above_4g_mem_size,
> + mch->pci_info.w64.begin,
> pci_hole64_size);
> if (pci_hole64_size) {
> memory_region_add_subregion(mch->system_memory,
> - 0x100000000ULL + mch->above_4g_mem_size,
> + mch->pci_info.w64.begin,
> &mch->pci_hole_64bit);
> }
> /* smram */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 7fb97b0..ab747b7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -18,7 +18,6 @@ typedef struct PcPciInfo {
> } PcPciInfo;
>
> struct PcGuestInfo {
> - PcPciInfo pci_info;
> bool has_pci_info;
> FWCfgState *fw_cfg;
> };
> @@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
>
> PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> ram_addr_t above_4g_mem_size);
> +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
>
> FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> const char *kernel_filename,
> @@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> ram_addr_t ram_size,
> hwaddr pci_hole_start,
> hwaddr pci_hole_size,
> - hwaddr pci_hole64_start,
> - hwaddr pci_hole64_size,
> + ram_addr_t above_4g_mem_size,
> MemoryRegion *pci_memory,
> MemoryRegion *ram_memory);
>
> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> index 3cb631e..3a9e04b 100644
> --- a/include/hw/pci-host/q35.h
> +++ b/include/hw/pci-host/q35.h
> @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
> MemoryRegion smram_region;
> MemoryRegion pci_hole;
> MemoryRegion pci_hole_64bit;
> + PcPciInfo pci_info;
> uint8_t smm_enabled;
> ram_addr_t below_4g_mem_size;
> ram_addr_t above_4g_mem_size;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 7:57 ` Michael S. Tsirkin
@ 2013-07-28 8:21 ` Igor Mammedov
2013-07-28 9:11 ` Michael S. Tsirkin
2013-07-28 9:13 ` Michael S. Tsirkin
0 siblings, 2 replies; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 8:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Sun, 28 Jul 2013 10:57:12 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > It turns out that some 32 bit windows guests crash
> > if 64 bit PCI hole size is >2G.
> > Limit it to 2G for piix and q35 by default.
> > User may override default boundaries by using
> > "pci_hole64_end " property.
> >
> > Examples:
> > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> >
> > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
>
> IMO that's pretty bad as user interfaces go.
> In particular if you are not careful you can make
> end < start.
> Why not set the size, and include a patch that
> let people write "1G" or "2G" for convenience?
sure as convenience why not, on top of this patches.
>
> > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > hw/i386/pc_piix.c | 14 +----------
> > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > include/hw/i386/pc.h | 5 ++--
> > include/hw/pci-host/q35.h | 1 +
> > 6 files changed, 94 insertions(+), 54 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b0b98a8..acaeb6c 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > {
> > PcRomPciInfo *info;
> > + Object *pci_info;
> > +
> > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > return;
> > }
> > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > + if (!pci_info) {
> > + pci_info = object_resolve_path("/machine/q35", NULL);
> > + if (!pci_info) {
> > + return;
> > + }
> > + }
>
>
> So is the path /machine/i440fx? /machine/i440FX?
> /machine/i440FX-pcihost?
> There's no way to check this code is right without
> actually running it.
that drawback of dynamic lookup,
QOM paths supposed to be stable.
>
> How about i44fx_get_pci_info so we can have this
> knowledge of paths localized in specific chipset code?
I've seen objections from afaerber about this approach, so dropped
this idea.
>
> >
> > info = g_malloc(sizeof *info);
> > - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> > - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> > - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> > - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> > + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> > + "pci_hole_start", NULL));
> > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> > + "pci_hole_end", NULL));
>
> Looks wrong.
> object_property_get_int returns a signed int64.
> w32 is unsigned.
> Happens to work but I think we need an explicit API.
I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
but not need for extra API, with fixed property definition
i.e. s/UINT64/UNIT32/ property set code will take care about limits.
>
> Property names are hard-coded string literals.
> Please add macros to set and get them
> so we can avoid duplicating code.
> E.g.
sure.
>
> #define PCI_HOST_PROPS...
> static inline get_
>
>
>
> > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> > + "pci_hole64_start", NULL));
> > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> > + "pci_hole64_end", NULL));
> > /* Pass PCI hole info to guest via a side channel.
> > * Required so guest PCI enumeration does the right thing. */
> > fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> > @@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> > PcGuestInfo *guest_info = &guest_info_state->info;
> >
> > - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> > - if (sizeof(hwaddr) == 4) {
> > - guest_info->pci_info.w64.begin = 0;
> > - guest_info->pci_info.w64.end = 0;
> > - } else {
> > - /*
> > - * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > - * align address to power of two. Align address at 1G, this makes sure
> > - * it can be exactly covered with a PAT entry even when using huge
> > - * pages.
> > - */
> > - guest_info->pci_info.w64.begin =
> > - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> > - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> > - (0x1ULL << 62);
> > - assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
> > - }
> > -
> > guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> > return guest_info;
> > }
> >
> > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
> > +{
> > + if (sizeof(hwaddr) == 4) {
> > + memset(&pci_info->w64, sizeof(pci_info->w64), 0);
> > + return;
> > + }
> > + /*
> > + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > + * align address to power of two. Align address at 1G, this makes sure
> > + * it can be exactly covered with a PAT entry even when using huge
> > + * pages.
> > + */
> > + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> > + if (!pci_info->w64.end) {
> > + /* set default end if it wasn't specified, + 2 Gb past start */
> > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
> > + }
> > + assert(pci_info->w64.begin < pci_info->w64.end);
> > +}
> > +
> > void pc_acpi_init(const char *default_dsdt)
> > {
> > char *filename;
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index b58c255..ab25458 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > guest_info->has_pci_info = has_pci_info;
> >
> > - /* Set PCI window size the way seabios has always done it. */
> > - /* Power of 2 so bios can cover it with a single MTRR */
> > - if (ram_size <= 0x80000000)
> > - guest_info->pci_info.w32.begin = 0x80000000;
> > - else if (ram_size <= 0xc0000000)
> > - guest_info->pci_info.w32.begin = 0xc0000000;
> > - else
> > - guest_info->pci_info.w32.begin = 0xe0000000;
> > -
> > /* allocate ram and load rom/bios */
> > if (!xen_enabled()) {
> > fw_cfg = pc_memory_init(system_memory,
> > @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > system_memory, system_io, ram_size,
> > below_4g_mem_size,
> > 0x100000000ULL - below_4g_mem_size,
> > - 0x100000000ULL + above_4g_mem_size,
> > - (sizeof(hwaddr) == 4
> > - ? 0
> > - : ((uint64_t)1 << 62)),
> > + above_4g_mem_size,
> > pci_memory, ram_memory);
> > } else {
> > pci_bus = NULL;
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index 4624d04..59a42c5 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -32,6 +32,7 @@
> > #include "hw/xen/xen.h"
> > #include "hw/pci-host/pam.h"
> > #include "sysemu/sysemu.h"
> > +#include "hw/i386/ioapic.h"
> >
> > /*
> > * I440FX chipset data sheet.
> > @@ -44,6 +45,7 @@
> >
> > typedef struct I440FXState {
> > PCIHostState parent_obj;
> > + PcPciInfo pci_info;
> > } I440FXState;
> >
> > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > @@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > ram_addr_t ram_size,
> > hwaddr pci_hole_start,
> > hwaddr pci_hole_size,
> > - hwaddr pci_hole64_start,
> > - hwaddr pci_hole64_size,
> > + ram_addr_t above_4g_mem_size,
> > MemoryRegion *pci_address_space,
> > MemoryRegion *ram_memory)
> > {
> > @@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > PIIX3State *piix3;
> > PCII440FXState *f;
> > unsigned i;
> > + I440FXState *i440fx;
> > + uint64_t pci_hole64_size;
> >
> > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> > s = PCI_HOST_BRIDGE(dev);
> > @@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > f->system_memory = address_space_mem;
> > f->pci_address_space = pci_address_space;
> > f->ram_memory = ram_memory;
> > +
> > + i440fx = I440FX_PCI_HOST(dev);
> > + /* Set PCI window size the way seabios has always done it. */
> > + /* Power of 2 so bios can cover it with a single MTRR */
> > + if (ram_size <= 0x80000000) {
> > + i440fx->pci_info.w32.begin = 0x80000000;
> > + } else if (ram_size <= 0xc0000000) {
> > + i440fx->pci_info.w32.begin = 0xc0000000;
> > + } else {
> > + i440fx->pci_info.w32.begin = 0xe0000000;
> > + }
> > +
> > memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > pci_hole_start, pci_hole_size);
> > memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > +
> > + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
> > + pci_hole64_size = range_size(i440fx->pci_info.w64);
> > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > f->pci_address_space,
> > - pci_hole64_start, pci_hole64_size);
> > + i440fx->pci_info.w64.begin, pci_hole64_size);
> > if (pci_hole64_size) {
> > - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> > + memory_region_add_subregion(f->system_memory,
> > + i440fx->pci_info.w64.begin,
> > &f->pci_hole_64bit);
> > }
> > memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> > @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> > return "0000";
> > }
> >
> > +static Property i440fx_props[] = {
> > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> > + IO_APIC_DEFAULT_ADDRESS),
> > + DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
>
> So we have 4 properties. One of them pci_hole64_end
> is supposed to be set to a value.
> Others should not be touched under any circuimstances.
> Of course if you query properties you have no way
> to know which is which and what are the legal values.
> Ouch.
read-only properties are possible but we would have to drop
usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
user better not to touch these properties since they are mostly internal API.
but if we say it's internal properties then enforcing read-only might be
overkill.
For user friendly property "pci_hole64_size" would be nice to have.
>
> > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > dc->realize = i440fx_pcihost_realize;
> > dc->fw_name = "pci";
> > dc->no_user = 1;
> > + dc->props = i440fx_props;
> > }
> >
> > static const TypeInfo i440fx_pcihost_info = {
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 6b1b3b7..a6936e6 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
> > static Property mch_props[] = {
> > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
> > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
> > + mch.pci_info.w64.begin, 0),
> > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
> > + mch.pci_info.w64.end, 0),
> > + /* Leave enough space for the biggest MCFG BAR */
> > + /* TODO: this matches current bios behaviour, but
> > + * it's not a power of two, which means an MTRR
> > + * can't cover it exactly.
> > + */
> > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
> > + mch.pci_info.w32.begin,
> > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > + MCH_HOST_BRIDGE_PCIEXBAR_MAX),
> > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
> > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
> > hwaddr pci_hole64_size;
> > MCHPCIState *mch = MCH_PCI_DEVICE(d);
> >
> > - /* Leave enough space for the biggest MCFG BAR */
> > - /* TODO: this matches current bios behaviour, but
> > - * it's not a power of two, which means an MTRR
> > - * can't cover it exactly.
> > - */
> > - mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > - MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> > -
> > /* setup pci memory regions */
> > memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
> > mch->pci_address_space,
> > @@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
> > 0x100000000ULL - mch->below_4g_mem_size);
> > memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
> > &mch->pci_hole);
> > - pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> > - ((uint64_t)1 << 62));
> > +
> > + pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size);
> > + pci_hole64_size = range_size(mch->pci_info.w64);
> > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
> > mch->pci_address_space,
> > - 0x100000000ULL + mch->above_4g_mem_size,
> > + mch->pci_info.w64.begin,
> > pci_hole64_size);
> > if (pci_hole64_size) {
> > memory_region_add_subregion(mch->system_memory,
> > - 0x100000000ULL + mch->above_4g_mem_size,
> > + mch->pci_info.w64.begin,
> > &mch->pci_hole_64bit);
> > }
> > /* smram */
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 7fb97b0..ab747b7 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -18,7 +18,6 @@ typedef struct PcPciInfo {
> > } PcPciInfo;
> >
> > struct PcGuestInfo {
> > - PcPciInfo pci_info;
> > bool has_pci_info;
> > FWCfgState *fw_cfg;
> > };
> > @@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
> >
> > PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > ram_addr_t above_4g_mem_size);
> > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
> >
> > FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > const char *kernel_filename,
> > @@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> > ram_addr_t ram_size,
> > hwaddr pci_hole_start,
> > hwaddr pci_hole_size,
> > - hwaddr pci_hole64_start,
> > - hwaddr pci_hole64_size,
> > + ram_addr_t above_4g_mem_size,
> > MemoryRegion *pci_memory,
> > MemoryRegion *ram_memory);
> >
> > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > index 3cb631e..3a9e04b 100644
> > --- a/include/hw/pci-host/q35.h
> > +++ b/include/hw/pci-host/q35.h
> > @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
> > MemoryRegion smram_region;
> > MemoryRegion pci_hole;
> > MemoryRegion pci_hole_64bit;
> > + PcPciInfo pci_info;
> > uint8_t smm_enabled;
> > ram_addr_t below_4g_mem_size;
> > ram_addr_t above_4g_mem_size;
> > --
> > 1.8.3.1
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 8:21 ` Igor Mammedov
@ 2013-07-28 9:11 ` Michael S. Tsirkin
2013-07-28 10:17 ` Andreas Färber
2013-07-28 17:33 ` Igor Mammedov
2013-07-28 9:13 ` Michael S. Tsirkin
1 sibling, 2 replies; 29+ messages in thread
From: Michael S. Tsirkin @ 2013-07-28 9:11 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> On Sun, 28 Jul 2013 10:57:12 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > It turns out that some 32 bit windows guests crash
> > > if 64 bit PCI hole size is >2G.
> > > Limit it to 2G for piix and q35 by default.
> > > User may override default boundaries by using
> > > "pci_hole64_end " property.
> > >
> > > Examples:
> > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > >
> > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> >
> > IMO that's pretty bad as user interfaces go.
> > In particular if you are not careful you can make
> > end < start.
> > Why not set the size, and include a patch that
> > let people write "1G" or "2G" for convenience?
> sure as convenience why not, on top of this patches.
Well because you are specifying end as 0xffffffffffffffff
so it's not a multiple of 1G?
> >
> > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > > hw/i386/pc_piix.c | 14 +----------
> > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > > include/hw/i386/pc.h | 5 ++--
> > > include/hw/pci-host/q35.h | 1 +
> > > 6 files changed, 94 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index b0b98a8..acaeb6c 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > {
> > > PcRomPciInfo *info;
> > > + Object *pci_info;
> > > +
> > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > return;
> > > }
> > > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > + if (!pci_info) {
> > > + pci_info = object_resolve_path("/machine/q35", NULL);
> > > + if (!pci_info) {
> > > + return;
> > > + }
> > > + }
> >
> >
> > So is the path /machine/i440fx? /machine/i440FX?
> > /machine/i440FX-pcihost?
> > There's no way to check this code is right without
> > actually running it.
> that drawback of dynamic lookup,
> QOM paths supposed to be stable.
>
> >
> > How about i44fx_get_pci_info so we can have this
> > knowledge of paths localized in specific chipset code?
> I've seen objections from afaerber about this approach, so dropped
> this idea.
>
> >
> > >
> > > info = g_malloc(sizeof *info);
> > > - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> > > - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> > > - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> > > - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> > > + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> > > + "pci_hole_start", NULL));
> > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> > > + "pci_hole_end", NULL));
> >
> > Looks wrong.
> > object_property_get_int returns a signed int64.
> > w32 is unsigned.
> > Happens to work but I think we need an explicit API.
> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
Not these are 64 bit values, but they need to be
unsigned not signed.
> but not need for extra API, with fixed property definition
> i.e. s/UINT64/UNIT32/ property set code will take care about limits.
If you replace these with UINT32 you won't be able to
specify values >4G.
> >
> > Property names are hard-coded string literals.
> > Please add macros to set and get them
> > so we can avoid duplicating code.
> > E.g.
> sure.
>
> >
> > #define PCI_HOST_PROPS...
> > static inline get_
> >
> >
> >
> > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> > > + "pci_hole64_start", NULL));
> > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> > > + "pci_hole64_end", NULL));
> > > /* Pass PCI hole info to guest via a side channel.
> > > * Required so guest PCI enumeration does the right thing. */
> > > fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> > > @@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> > > PcGuestInfo *guest_info = &guest_info_state->info;
> > >
> > > - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> > > - if (sizeof(hwaddr) == 4) {
> > > - guest_info->pci_info.w64.begin = 0;
> > > - guest_info->pci_info.w64.end = 0;
> > > - } else {
> > > - /*
> > > - * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > - * align address to power of two. Align address at 1G, this makes sure
> > > - * it can be exactly covered with a PAT entry even when using huge
> > > - * pages.
> > > - */
> > > - guest_info->pci_info.w64.begin =
> > > - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> > > - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> > > - (0x1ULL << 62);
> > > - assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
> > > - }
> > > -
> > > guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> > > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> > > return guest_info;
> > > }
> > >
> > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
> > > +{
> > > + if (sizeof(hwaddr) == 4) {
> > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0);
> > > + return;
> > > + }
> > > + /*
> > > + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > + * align address to power of two. Align address at 1G, this makes sure
> > > + * it can be exactly covered with a PAT entry even when using huge
> > > + * pages.
> > > + */
> > > + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> > > + if (!pci_info->w64.end) {
> > > + /* set default end if it wasn't specified, + 2 Gb past start */
> > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
> > > + }
> > > + assert(pci_info->w64.begin < pci_info->w64.end);
> > > +}
> > > +
> > > void pc_acpi_init(const char *default_dsdt)
> > > {
> > > char *filename;
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index b58c255..ab25458 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> > > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > > guest_info->has_pci_info = has_pci_info;
> > >
> > > - /* Set PCI window size the way seabios has always done it. */
> > > - /* Power of 2 so bios can cover it with a single MTRR */
> > > - if (ram_size <= 0x80000000)
> > > - guest_info->pci_info.w32.begin = 0x80000000;
> > > - else if (ram_size <= 0xc0000000)
> > > - guest_info->pci_info.w32.begin = 0xc0000000;
> > > - else
> > > - guest_info->pci_info.w32.begin = 0xe0000000;
> > > -
> > > /* allocate ram and load rom/bios */
> > > if (!xen_enabled()) {
> > > fw_cfg = pc_memory_init(system_memory,
> > > @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > > system_memory, system_io, ram_size,
> > > below_4g_mem_size,
> > > 0x100000000ULL - below_4g_mem_size,
> > > - 0x100000000ULL + above_4g_mem_size,
> > > - (sizeof(hwaddr) == 4
> > > - ? 0
> > > - : ((uint64_t)1 << 62)),
> > > + above_4g_mem_size,
> > > pci_memory, ram_memory);
> > > } else {
> > > pci_bus = NULL;
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index 4624d04..59a42c5 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -32,6 +32,7 @@
> > > #include "hw/xen/xen.h"
> > > #include "hw/pci-host/pam.h"
> > > #include "sysemu/sysemu.h"
> > > +#include "hw/i386/ioapic.h"
> > >
> > > /*
> > > * I440FX chipset data sheet.
> > > @@ -44,6 +45,7 @@
> > >
> > > typedef struct I440FXState {
> > > PCIHostState parent_obj;
> > > + PcPciInfo pci_info;
> > > } I440FXState;
> > >
> > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > > @@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > ram_addr_t ram_size,
> > > hwaddr pci_hole_start,
> > > hwaddr pci_hole_size,
> > > - hwaddr pci_hole64_start,
> > > - hwaddr pci_hole64_size,
> > > + ram_addr_t above_4g_mem_size,
> > > MemoryRegion *pci_address_space,
> > > MemoryRegion *ram_memory)
> > > {
> > > @@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > PIIX3State *piix3;
> > > PCII440FXState *f;
> > > unsigned i;
> > > + I440FXState *i440fx;
> > > + uint64_t pci_hole64_size;
> > >
> > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> > > s = PCI_HOST_BRIDGE(dev);
> > > @@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > f->system_memory = address_space_mem;
> > > f->pci_address_space = pci_address_space;
> > > f->ram_memory = ram_memory;
> > > +
> > > + i440fx = I440FX_PCI_HOST(dev);
> > > + /* Set PCI window size the way seabios has always done it. */
> > > + /* Power of 2 so bios can cover it with a single MTRR */
> > > + if (ram_size <= 0x80000000) {
> > > + i440fx->pci_info.w32.begin = 0x80000000;
> > > + } else if (ram_size <= 0xc0000000) {
> > > + i440fx->pci_info.w32.begin = 0xc0000000;
> > > + } else {
> > > + i440fx->pci_info.w32.begin = 0xe0000000;
> > > + }
> > > +
> > > memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > > pci_hole_start, pci_hole_size);
> > > memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > > +
> > > + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
> > > + pci_hole64_size = range_size(i440fx->pci_info.w64);
> > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > > f->pci_address_space,
> > > - pci_hole64_start, pci_hole64_size);
> > > + i440fx->pci_info.w64.begin, pci_hole64_size);
> > > if (pci_hole64_size) {
> > > - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> > > + memory_region_add_subregion(f->system_memory,
> > > + i440fx->pci_info.w64.begin,
> > > &f->pci_hole_64bit);
> > > }
> > > memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> > > @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> > > return "0000";
> > > }
> > >
> > > +static Property i440fx_props[] = {
> > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> > > + IO_APIC_DEFAULT_ADDRESS),
> > > + DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> >
> > So we have 4 properties. One of them pci_hole64_end
> > is supposed to be set to a value.
> > Others should not be touched under any circuimstances.
> > Of course if you query properties you have no way
> > to know which is which and what are the legal values.
> > Ouch.
> read-only properties are possible but we would have to drop
> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
Or add DEFINE_PROP_UINT64_RO for this?
> user better not to touch these properties since they are mostly internal API.
> but if we say it's internal properties then enforcing read-only might be
> overkill.
> For user friendly property "pci_hole64_size" would be nice to have.
So at the moment I do
qemu -device i440FX-pcihost,help
and this will get all properties.
If we add some properties that user can not set
they should not appear in this output.
> >
> > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > > {
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > > dc->realize = i440fx_pcihost_realize;
> > > dc->fw_name = "pci";
> > > dc->no_user = 1;
> > > + dc->props = i440fx_props;
> > > }
> > >
> > > static const TypeInfo i440fx_pcihost_info = {
> > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > index 6b1b3b7..a6936e6 100644
> > > --- a/hw/pci-host/q35.c
> > > +++ b/hw/pci-host/q35.c
> > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
> > > static Property mch_props[] = {
> > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
> > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
> > > + mch.pci_info.w64.begin, 0),
> > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
> > > + mch.pci_info.w64.end, 0),
> > > + /* Leave enough space for the biggest MCFG BAR */
> > > + /* TODO: this matches current bios behaviour, but
> > > + * it's not a power of two, which means an MTRR
> > > + * can't cover it exactly.
> > > + */
> > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
> > > + mch.pci_info.w32.begin,
> > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX),
> > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
> > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > >
> > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
> > > hwaddr pci_hole64_size;
> > > MCHPCIState *mch = MCH_PCI_DEVICE(d);
> > >
> > > - /* Leave enough space for the biggest MCFG BAR */
> > > - /* TODO: this matches current bios behaviour, but
> > > - * it's not a power of two, which means an MTRR
> > > - * can't cover it exactly.
> > > - */
> > > - mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > > - MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> > > -
> > > /* setup pci memory regions */
> > > memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
> > > mch->pci_address_space,
> > > @@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
> > > 0x100000000ULL - mch->below_4g_mem_size);
> > > memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
> > > &mch->pci_hole);
> > > - pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> > > - ((uint64_t)1 << 62));
> > > +
> > > + pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size);
> > > + pci_hole64_size = range_size(mch->pci_info.w64);
> > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
> > > mch->pci_address_space,
> > > - 0x100000000ULL + mch->above_4g_mem_size,
> > > + mch->pci_info.w64.begin,
> > > pci_hole64_size);
> > > if (pci_hole64_size) {
> > > memory_region_add_subregion(mch->system_memory,
> > > - 0x100000000ULL + mch->above_4g_mem_size,
> > > + mch->pci_info.w64.begin,
> > > &mch->pci_hole_64bit);
> > > }
> > > /* smram */
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 7fb97b0..ab747b7 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -18,7 +18,6 @@ typedef struct PcPciInfo {
> > > } PcPciInfo;
> > >
> > > struct PcGuestInfo {
> > > - PcPciInfo pci_info;
> > > bool has_pci_info;
> > > FWCfgState *fw_cfg;
> > > };
> > > @@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
> > >
> > > PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > ram_addr_t above_4g_mem_size);
> > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
> > >
> > > FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > > const char *kernel_filename,
> > > @@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> > > ram_addr_t ram_size,
> > > hwaddr pci_hole_start,
> > > hwaddr pci_hole_size,
> > > - hwaddr pci_hole64_start,
> > > - hwaddr pci_hole64_size,
> > > + ram_addr_t above_4g_mem_size,
> > > MemoryRegion *pci_memory,
> > > MemoryRegion *ram_memory);
> > >
> > > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > > index 3cb631e..3a9e04b 100644
> > > --- a/include/hw/pci-host/q35.h
> > > +++ b/include/hw/pci-host/q35.h
> > > @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
> > > MemoryRegion smram_region;
> > > MemoryRegion pci_hole;
> > > MemoryRegion pci_hole_64bit;
> > > + PcPciInfo pci_info;
> > > uint8_t smm_enabled;
> > > ram_addr_t below_4g_mem_size;
> > > ram_addr_t above_4g_mem_size;
> > > --
> > > 1.8.3.1
> >
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 8:21 ` Igor Mammedov
2013-07-28 9:11 ` Michael S. Tsirkin
@ 2013-07-28 9:13 ` Michael S. Tsirkin
2013-07-28 17:34 ` Igor Mammedov
1 sibling, 1 reply; 29+ messages in thread
From: Michael S. Tsirkin @ 2013-07-28 9:13 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> On Sun, 28 Jul 2013 10:57:12 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > It turns out that some 32 bit windows guests crash
> > > if 64 bit PCI hole size is >2G.
> > > Limit it to 2G for piix and q35 by default.
> > > User may override default boundaries by using
> > > "pci_hole64_end " property.
> > >
> > > Examples:
> > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > >
> > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> >
> > IMO that's pretty bad as user interfaces go.
> > In particular if you are not careful you can make
> > end < start.
> > Why not set the size, and include a patch that
> > let people write "1G" or "2G" for convenience?
> sure as convenience why not, on top of this patches.
>
> >
> > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > > hw/i386/pc_piix.c | 14 +----------
> > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > > include/hw/i386/pc.h | 5 ++--
> > > include/hw/pci-host/q35.h | 1 +
> > > 6 files changed, 94 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index b0b98a8..acaeb6c 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > {
> > > PcRomPciInfo *info;
> > > + Object *pci_info;
> > > +
> > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > return;
> > > }
> > > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > + if (!pci_info) {
> > > + pci_info = object_resolve_path("/machine/q35", NULL);
> > > + if (!pci_info) {
> > > + return;
> > > + }
> > > + }
> >
> >
> > So is the path /machine/i440fx? /machine/i440FX?
> > /machine/i440FX-pcihost?
> > There's no way to check this code is right without
> > actually running it.
> that drawback of dynamic lookup,
> QOM paths supposed to be stable.
>
> >
> > How about i44fx_get_pci_info so we can have this
> > knowledge of paths localized in specific chipset code?
> I've seen objections from afaerber about this approach, so dropped
> this idea.
Could we lookup TYPE_PCI_HOST? This will make pc code
independent of specific machine.
--
MST
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h
2013-07-28 7:29 ` [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h Igor Mammedov
@ 2013-07-28 9:54 ` Andreas Färber
2013-07-28 17:19 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 9:54 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Am 28.07.2013 09:29, schrieb Igor Mammedov:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Missing Reviewed-bys from Gerd and me.
Andreas
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/pc.c | 2 --
> include/hw/i386/ioapic.h | 1 +
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2a87563..b0b98a8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -75,8 +75,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 {
> diff --git a/include/hw/i386/ioapic.h b/include/hw/i386/ioapic.h
> index 86e63da..6245388 100644
> --- a/include/hw/i386/ioapic.h
> +++ b/include/hw/i386/ioapic.h
> @@ -21,6 +21,7 @@
> #define HW_IOAPIC_H
>
> #define IOAPIC_NUM_PINS 24
> +#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
>
> void ioapic_eoi_broadcast(int vector);
>
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro
2013-07-28 7:29 ` [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro Igor Mammedov
@ 2013-07-28 9:57 ` Andreas Färber
2013-07-28 17:21 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 9:57 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Am 28.07.2013 09:29, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/pci-host/piix.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 3908860..bf879e7 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -38,6 +38,10 @@
> * http://download.intel.com/design/chipsets/datashts/29054901.pdf
> */
>
> +#define TYPE_I440FX_PCI_HOST "i440FX-pcihost"
> +#define I440FX_PCI_HOST(obj) \
> + OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST)
Either Anthony or mst had insisted on PCI_HOST_BRIDGE rather than
PCI_HOST. Other than that looks good, thanks!
Andreas
> +
> typedef struct I440FXState {
> PCIHostState parent_obj;
> } I440FXState;
> @@ -257,7 +261,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
> PCII440FXState *f;
> unsigned i;
>
> - dev = qdev_create(NULL, "i440FX-pcihost");
> + dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> s = PCI_HOST_BRIDGE(dev);
> b = pci_bus_new(dev, NULL, pci_address_space,
> address_space_io, 0, TYPE_PCI_BUS);
> @@ -661,7 +665,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> }
>
> static const TypeInfo i440fx_pcihost_info = {
> - .name = "i440FX-pcihost",
> + .name = TYPE_I440FX_PCI_HOST,
> .parent = TYPE_PCI_HOST_BRIDGE,
> .instance_size = sizeof(I440FXState),
> .instance_init = i440fx_pcihost_initfn,
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else
2013-07-28 7:29 ` [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else Igor Mammedov
@ 2013-07-28 10:07 ` Andreas Färber
0 siblings, 0 replies; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 10:07 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Am 28.07.2013 09:29, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/pci-host/piix.c | 50 +++++++++++++-------------------------------------
> 1 file changed, 13 insertions(+), 37 deletions(-)
Subject should probably be rebroken to have "It isn't ..." in the body.
I've confirmed that it's not used anywhere else currently, so
Reviewed-by: Andreas Färber <afaerber@suse.de>
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine
2013-07-28 7:29 ` [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine Igor Mammedov
@ 2013-07-28 10:08 ` Andreas Färber
0 siblings, 0 replies; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 10:08 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Am 28.07.2013 09:29, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/i386/pc_q35.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0b1d2e3..5140caf 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -130,7 +130,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>
> /* create pci host bus */
> q35_host = Q35_HOST_DEVICE(qdev_create(NULL, TYPE_Q35_HOST_DEVICE));
> -
> + object_property_add_child(qdev_get_machine(), "q35", OBJECT(q35_host), NULL);
> q35_host->mch.ram_memory = ram_memory;
> q35_host->mch.pci_address_space = pci_memory;
> q35_host->mch.system_memory = get_system_memory();
Reviewed-by: Andreas Färber <afaerber@suse.de>
Thanks a lot, I was about to send the equivalent patch!
This is a Last Minute guest-visible change, but it matches what we have
for i440fx, so I consider this fine.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 9:11 ` Michael S. Tsirkin
@ 2013-07-28 10:17 ` Andreas Färber
2013-07-28 17:40 ` Igor Mammedov
2013-07-30 21:34 ` Michael Roth
2013-07-28 17:33 ` Igor Mammedov
1 sibling, 2 replies; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 10:17 UTC (permalink / raw)
To: Michael S. Tsirkin, Igor Mammedov; +Cc: qemu-devel
Am 28.07.2013 11:11, schrieb Michael S. Tsirkin:
> On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
>> On Sun, 28 Jul 2013 10:57:12 +0300
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
>>>>
>>>> info = g_malloc(sizeof *info);
>>>> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
>>>> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
>>>> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
>>>> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
>>>> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
>>>> + "pci_hole_start", NULL));
>>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
>>>> + "pci_hole_end", NULL));
>>>
>>> Looks wrong.
>>> object_property_get_int returns a signed int64.
>>> w32 is unsigned.
>>> Happens to work but I think we need an explicit API.
That's how QAPI works internally today for any uint64 visitor/property.
uint64_t is cast to int64_t and back in visitors.
So I'd hope something like
uint64_t val = (uint64_t) object_property_get_int()
would work equally well - CC'ing Michael.
>> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
>
> Not these are 64 bit values, but they need to be
> unsigned not signed.
>
>> but not need for extra API, with fixed property definition
>> i.e. s/UINT64/UNIT32/ property set code will take care about limits.
>
> If you replace these with UINT32 you won't be able to
> specify values >4G.
>
>>> Property names are hard-coded string literals.
>>> Please add macros to set and get them
>>> so we can avoid duplicating code.
>>> E.g.
>> sure.
>>
>>>
>>> #define PCI_HOST_PROPS...
>>> static inline get_
[...]
>>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>>>> return "0000";
>>>> }
>>>>
>>>> +static Property i440fx_props[] = {
>>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
>>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
>>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
>>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
>>>> + IO_APIC_DEFAULT_ADDRESS),
>>>> + DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>
>>> So we have 4 properties. One of them pci_hole64_end
>>> is supposed to be set to a value.
>>> Others should not be touched under any circuimstances.
>>> Of course if you query properties you have no way
>>> to know which is which and what are the legal values.
>>> Ouch.
>> read-only properties are possible but we would have to drop
>> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
>
> Or add DEFINE_PROP_UINT64_RO for this?
>
>> user better not to touch these properties since they are mostly internal API.
>> but if we say it's internal properties then enforcing read-only might be
>> overkill.
>> For user friendly property "pci_hole64_size" would be nice to have.
>
> So at the moment I do
>
> qemu -device i440FX-pcihost,help
>
> and this will get all properties.
>
> If we add some properties that user can not set
> they should not appear in this output.
[snip]
Igor, you can simply use dynamic properties with NULL as setter argument
for object_property_add*() to achieve that effect.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h
2013-07-28 9:54 ` Andreas Färber
@ 2013-07-28 17:19 ` Igor Mammedov
2013-07-28 17:37 ` Andreas Färber
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 17:19 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, mst
On Sun, 28 Jul 2013 11:54:46 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 28.07.2013 09:29, schrieb Igor Mammedov:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> Missing Reviewed-bys from Gerd and me.
it changed file form apic.h to ioapic.h, so I haven't felt comfortble adding
yours Reviewed-bys.
I sure can add it if it's ok.
>
> Andreas
>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/i386/pc.c | 2 --
> > include/hw/i386/ioapic.h | 1 +
> > 2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 2a87563..b0b98a8 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -75,8 +75,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 {
> > diff --git a/include/hw/i386/ioapic.h b/include/hw/i386/ioapic.h
> > index 86e63da..6245388 100644
> > --- a/include/hw/i386/ioapic.h
> > +++ b/include/hw/i386/ioapic.h
> > @@ -21,6 +21,7 @@
> > #define HW_IOAPIC_H
> >
> > #define IOAPIC_NUM_PINS 24
> > +#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
> >
> > void ioapic_eoi_broadcast(int vector);
> >
> >
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro
2013-07-28 9:57 ` Andreas Färber
@ 2013-07-28 17:21 ` Igor Mammedov
2013-07-28 17:24 ` Andreas Färber
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 17:21 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, mst
On Sun, 28 Jul 2013 11:57:14 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 28.07.2013 09:29, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/pci-host/piix.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index 3908860..bf879e7 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -38,6 +38,10 @@
> > * http://download.intel.com/design/chipsets/datashts/29054901.pdf
> > */
> >
> > +#define TYPE_I440FX_PCI_HOST "i440FX-pcihost"
> > +#define I440FX_PCI_HOST(obj) \
> > + OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST)
>
> Either Anthony or mst had insisted on PCI_HOST_BRIDGE rather than
> PCI_HOST. Other than that looks good, thanks!
it's the type cast macro that is missing, so adding it shouldn't hurt,
and some day in future we might any way need to add it even if we don't use it
now.
> Andreas
>
> > +
> > typedef struct I440FXState {
> > PCIHostState parent_obj;
> > } I440FXState;
> > @@ -257,7 +261,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
> > PCII440FXState *f;
> > unsigned i;
> >
> > - dev = qdev_create(NULL, "i440FX-pcihost");
> > + dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> > s = PCI_HOST_BRIDGE(dev);
> > b = pci_bus_new(dev, NULL, pci_address_space,
> > address_space_io, 0, TYPE_PCI_BUS);
> > @@ -661,7 +665,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > }
> >
> > static const TypeInfo i440fx_pcihost_info = {
> > - .name = "i440FX-pcihost",
> > + .name = TYPE_I440FX_PCI_HOST,
> > .parent = TYPE_PCI_HOST_BRIDGE,
> > .instance_size = sizeof(I440FXState),
> > .instance_init = i440fx_pcihost_initfn,
> >
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro
2013-07-28 17:21 ` Igor Mammedov
@ 2013-07-28 17:24 ` Andreas Färber
2013-07-28 18:05 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 17:24 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
Am 28.07.2013 19:21, schrieb Igor Mammedov:
> On Sun, 28 Jul 2013 11:57:14 +0200
> Andreas Färber <afaerber@suse.de> wrote:
>
>> Am 28.07.2013 09:29, schrieb Igor Mammedov:
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> hw/pci-host/piix.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index 3908860..bf879e7 100644
>>> --- a/hw/pci-host/piix.c
>>> +++ b/hw/pci-host/piix.c
>>> @@ -38,6 +38,10 @@
>>> * http://download.intel.com/design/chipsets/datashts/29054901.pdf
>>> */
>>>
>>> +#define TYPE_I440FX_PCI_HOST "i440FX-pcihost"
>>> +#define I440FX_PCI_HOST(obj) \
>>> + OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST)
>>
>> Either Anthony or mst had insisted on PCI_HOST_BRIDGE rather than
>> PCI_HOST. Other than that looks good, thanks!
>
> it's the type cast macro that is missing, so adding it shouldn't hurt,
> and some day in future we might any way need to add it even if we don't use it
> now.
Maybe you misunderstood? Please add _BRIDGE to your macros. :)
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 9:11 ` Michael S. Tsirkin
2013-07-28 10:17 ` Andreas Färber
@ 2013-07-28 17:33 ` Igor Mammedov
2013-07-28 19:51 ` Michael S. Tsirkin
1 sibling, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 17:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Sun, 28 Jul 2013 12:11:42 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> > On Sun, 28 Jul 2013 10:57:12 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > > It turns out that some 32 bit windows guests crash
> > > > if 64 bit PCI hole size is >2G.
> > > > Limit it to 2G for piix and q35 by default.
> > > > User may override default boundaries by using
> > > > "pci_hole64_end " property.
> > > >
> > > > Examples:
> > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > > >
> > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> > >
> > > IMO that's pretty bad as user interfaces go.
> > > In particular if you are not careful you can make
> > > end < start.
> > > Why not set the size, and include a patch that
> > > let people write "1G" or "2G" for convenience?
> > sure as convenience why not, on top of this patches.
>
> Well because you are specifying end as 0xffffffffffffffff
> so it's not a multiple of 1G?
>
> > >
> > > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > > > hw/i386/pc_piix.c | 14 +----------
> > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > > > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > > > include/hw/i386/pc.h | 5 ++--
> > > > include/hw/pci-host/q35.h | 1 +
> > > > 6 files changed, 94 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index b0b98a8..acaeb6c 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > > {
> > > > PcRomPciInfo *info;
> > > > + Object *pci_info;
> > > > +
> > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > > return;
> > > > }
> > > > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > > + if (!pci_info) {
> > > > + pci_info = object_resolve_path("/machine/q35", NULL);
> > > > + if (!pci_info) {
> > > > + return;
> > > > + }
> > > > + }
> > >
> > >
> > > So is the path /machine/i440fx? /machine/i440FX?
> > > /machine/i440FX-pcihost?
> > > There's no way to check this code is right without
> > > actually running it.
> > that drawback of dynamic lookup,
> > QOM paths supposed to be stable.
> >
> > >
> > > How about i44fx_get_pci_info so we can have this
> > > knowledge of paths localized in specific chipset code?
> > I've seen objections from afaerber about this approach, so dropped
> > this idea.
> >
> > >
> > > >
> > > > info = g_malloc(sizeof *info);
> > > > - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> > > > - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> > > > - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> > > > - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> > > > + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> > > > + "pci_hole_start", NULL));
> > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> > > > + "pci_hole_end", NULL));
> > >
> > > Looks wrong.
> > > object_property_get_int returns a signed int64.
> > > w32 is unsigned.
> > > Happens to work but I think we need an explicit API.
> > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
>
> Not these are 64 bit values, but they need to be
> unsigned not signed.
>
> > but not need for extra API, with fixed property definition
> > i.e. s/UINT64/UNIT32/ property set code will take care about limits.
>
> If you replace these with UINT32 you won't be able to
> specify values >4G.
does 32 bit PCI hole has right to be more than 4Gb?
>
> > >
> > > Property names are hard-coded string literals.
> > > Please add macros to set and get them
> > > so we can avoid duplicating code.
> > > E.g.
> > sure.
> >
> > >
> > > #define PCI_HOST_PROPS...
> > > static inline get_
> > >
> > >
> > >
> > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> > > > + "pci_hole64_start", NULL));
> > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> > > > + "pci_hole64_end", NULL));
> > > > /* Pass PCI hole info to guest via a side channel.
> > > > * Required so guest PCI enumeration does the right thing. */
> > > > fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> > > > @@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > > PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> > > > PcGuestInfo *guest_info = &guest_info_state->info;
> > > >
> > > > - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> > > > - if (sizeof(hwaddr) == 4) {
> > > > - guest_info->pci_info.w64.begin = 0;
> > > > - guest_info->pci_info.w64.end = 0;
> > > > - } else {
> > > > - /*
> > > > - * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > > - * align address to power of two. Align address at 1G, this makes sure
> > > > - * it can be exactly covered with a PAT entry even when using huge
> > > > - * pages.
> > > > - */
> > > > - guest_info->pci_info.w64.begin =
> > > > - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> > > > - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> > > > - (0x1ULL << 62);
> > > > - assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
> > > > - }
> > > > -
> > > > guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> > > > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> > > > return guest_info;
> > > > }
> > > >
> > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
> > > > +{
> > > > + if (sizeof(hwaddr) == 4) {
> > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0);
> > > > + return;
> > > > + }
> > > > + /*
> > > > + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > > + * align address to power of two. Align address at 1G, this makes sure
> > > > + * it can be exactly covered with a PAT entry even when using huge
> > > > + * pages.
> > > > + */
> > > > + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> > > > + if (!pci_info->w64.end) {
> > > > + /* set default end if it wasn't specified, + 2 Gb past start */
> > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
> > > > + }
> > > > + assert(pci_info->w64.begin < pci_info->w64.end);
> > > > +}
> > > > +
> > > > void pc_acpi_init(const char *default_dsdt)
> > > > {
> > > > char *filename;
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index b58c255..ab25458 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> > > > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > > > guest_info->has_pci_info = has_pci_info;
> > > >
> > > > - /* Set PCI window size the way seabios has always done it. */
> > > > - /* Power of 2 so bios can cover it with a single MTRR */
> > > > - if (ram_size <= 0x80000000)
> > > > - guest_info->pci_info.w32.begin = 0x80000000;
> > > > - else if (ram_size <= 0xc0000000)
> > > > - guest_info->pci_info.w32.begin = 0xc0000000;
> > > > - else
> > > > - guest_info->pci_info.w32.begin = 0xe0000000;
> > > > -
> > > > /* allocate ram and load rom/bios */
> > > > if (!xen_enabled()) {
> > > > fw_cfg = pc_memory_init(system_memory,
> > > > @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > > > system_memory, system_io, ram_size,
> > > > below_4g_mem_size,
> > > > 0x100000000ULL - below_4g_mem_size,
> > > > - 0x100000000ULL + above_4g_mem_size,
> > > > - (sizeof(hwaddr) == 4
> > > > - ? 0
> > > > - : ((uint64_t)1 << 62)),
> > > > + above_4g_mem_size,
> > > > pci_memory, ram_memory);
> > > > } else {
> > > > pci_bus = NULL;
> > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > > index 4624d04..59a42c5 100644
> > > > --- a/hw/pci-host/piix.c
> > > > +++ b/hw/pci-host/piix.c
> > > > @@ -32,6 +32,7 @@
> > > > #include "hw/xen/xen.h"
> > > > #include "hw/pci-host/pam.h"
> > > > #include "sysemu/sysemu.h"
> > > > +#include "hw/i386/ioapic.h"
> > > >
> > > > /*
> > > > * I440FX chipset data sheet.
> > > > @@ -44,6 +45,7 @@
> > > >
> > > > typedef struct I440FXState {
> > > > PCIHostState parent_obj;
> > > > + PcPciInfo pci_info;
> > > > } I440FXState;
> > > >
> > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > > > @@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > ram_addr_t ram_size,
> > > > hwaddr pci_hole_start,
> > > > hwaddr pci_hole_size,
> > > > - hwaddr pci_hole64_start,
> > > > - hwaddr pci_hole64_size,
> > > > + ram_addr_t above_4g_mem_size,
> > > > MemoryRegion *pci_address_space,
> > > > MemoryRegion *ram_memory)
> > > > {
> > > > @@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > PIIX3State *piix3;
> > > > PCII440FXState *f;
> > > > unsigned i;
> > > > + I440FXState *i440fx;
> > > > + uint64_t pci_hole64_size;
> > > >
> > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> > > > s = PCI_HOST_BRIDGE(dev);
> > > > @@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > f->system_memory = address_space_mem;
> > > > f->pci_address_space = pci_address_space;
> > > > f->ram_memory = ram_memory;
> > > > +
> > > > + i440fx = I440FX_PCI_HOST(dev);
> > > > + /* Set PCI window size the way seabios has always done it. */
> > > > + /* Power of 2 so bios can cover it with a single MTRR */
> > > > + if (ram_size <= 0x80000000) {
> > > > + i440fx->pci_info.w32.begin = 0x80000000;
> > > > + } else if (ram_size <= 0xc0000000) {
> > > > + i440fx->pci_info.w32.begin = 0xc0000000;
> > > > + } else {
> > > > + i440fx->pci_info.w32.begin = 0xe0000000;
> > > > + }
> > > > +
> > > > memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > > > pci_hole_start, pci_hole_size);
> > > > memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > > > +
> > > > + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
> > > > + pci_hole64_size = range_size(i440fx->pci_info.w64);
> > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > > > f->pci_address_space,
> > > > - pci_hole64_start, pci_hole64_size);
> > > > + i440fx->pci_info.w64.begin, pci_hole64_size);
> > > > if (pci_hole64_size) {
> > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> > > > + memory_region_add_subregion(f->system_memory,
> > > > + i440fx->pci_info.w64.begin,
> > > > &f->pci_hole_64bit);
> > > > }
> > > > memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> > > > @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> > > > return "0000";
> > > > }
> > > >
> > > > +static Property i440fx_props[] = {
> > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> > > > + IO_APIC_DEFAULT_ADDRESS),
> > > > + DEFINE_PROP_END_OF_LIST(),
> > > > +};
> > > > +
> > >
> > > So we have 4 properties. One of them pci_hole64_end
> > > is supposed to be set to a value.
> > > Others should not be touched under any circuimstances.
> > > Of course if you query properties you have no way
> > > to know which is which and what are the legal values.
> > > Ouch.
> > read-only properties are possible but we would have to drop
> > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
>
> Or add DEFINE_PROP_UINT64_RO for this?
it might be the way to do it.
>
> > user better not to touch these properties since they are mostly internal API.
> > but if we say it's internal properties then enforcing read-only might be
> > overkill.
> > For user friendly property "pci_hole64_size" would be nice to have.
>
> So at the moment I do
>
> qemu -device i440FX-pcihost,help
>
> and this will get all properties.
>
> If we add some properties that user can not set
> they should not appear in this output.
with QOM all around I wouldn't say so, it could be property only for internal
purposes and QOM properties do not care about whether they are visible or
not to user so far. I guess we could document it in code like do not
touch /internal or ...
>
>
> > >
> > > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > > > {
> > > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > > @@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > > > dc->realize = i440fx_pcihost_realize;
> > > > dc->fw_name = "pci";
> > > > dc->no_user = 1;
> > > > + dc->props = i440fx_props;
> > > > }
> > > >
> > > > static const TypeInfo i440fx_pcihost_info = {
> > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > index 6b1b3b7..a6936e6 100644
> > > > --- a/hw/pci-host/q35.c
> > > > +++ b/hw/pci-host/q35.c
> > > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
> > > > static Property mch_props[] = {
> > > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
> > > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> > > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
> > > > + mch.pci_info.w64.begin, 0),
> > > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
> > > > + mch.pci_info.w64.end, 0),
> > > > + /* Leave enough space for the biggest MCFG BAR */
> > > > + /* TODO: this matches current bios behaviour, but
> > > > + * it's not a power of two, which means an MTRR
> > > > + * can't cover it exactly.
> > > > + */
> > > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
> > > > + mch.pci_info.w32.begin,
> > > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX),
> > > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
> > > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > >
> > > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
> > > > hwaddr pci_hole64_size;
> > > > MCHPCIState *mch = MCH_PCI_DEVICE(d);
> > > >
> > > > - /* Leave enough space for the biggest MCFG BAR */
> > > > - /* TODO: this matches current bios behaviour, but
> > > > - * it's not a power of two, which means an MTRR
> > > > - * can't cover it exactly.
> > > > - */
> > > > - mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > > > - MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> > > > -
> > > > /* setup pci memory regions */
> > > > memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
> > > > mch->pci_address_space,
> > > > @@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
> > > > 0x100000000ULL - mch->below_4g_mem_size);
> > > > memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
> > > > &mch->pci_hole);
> > > > - pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> > > > - ((uint64_t)1 << 62));
> > > > +
> > > > + pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size);
> > > > + pci_hole64_size = range_size(mch->pci_info.w64);
> > > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
> > > > mch->pci_address_space,
> > > > - 0x100000000ULL + mch->above_4g_mem_size,
> > > > + mch->pci_info.w64.begin,
> > > > pci_hole64_size);
> > > > if (pci_hole64_size) {
> > > > memory_region_add_subregion(mch->system_memory,
> > > > - 0x100000000ULL + mch->above_4g_mem_size,
> > > > + mch->pci_info.w64.begin,
> > > > &mch->pci_hole_64bit);
> > > > }
> > > > /* smram */
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index 7fb97b0..ab747b7 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -18,7 +18,6 @@ typedef struct PcPciInfo {
> > > > } PcPciInfo;
> > > >
> > > > struct PcGuestInfo {
> > > > - PcPciInfo pci_info;
> > > > bool has_pci_info;
> > > > FWCfgState *fw_cfg;
> > > > };
> > > > @@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
> > > >
> > > > PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > > ram_addr_t above_4g_mem_size);
> > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
> > > >
> > > > FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > > > const char *kernel_filename,
> > > > @@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> > > > ram_addr_t ram_size,
> > > > hwaddr pci_hole_start,
> > > > hwaddr pci_hole_size,
> > > > - hwaddr pci_hole64_start,
> > > > - hwaddr pci_hole64_size,
> > > > + ram_addr_t above_4g_mem_size,
> > > > MemoryRegion *pci_memory,
> > > > MemoryRegion *ram_memory);
> > > >
> > > > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > > > index 3cb631e..3a9e04b 100644
> > > > --- a/include/hw/pci-host/q35.h
> > > > +++ b/include/hw/pci-host/q35.h
> > > > @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
> > > > MemoryRegion smram_region;
> > > > MemoryRegion pci_hole;
> > > > MemoryRegion pci_hole_64bit;
> > > > + PcPciInfo pci_info;
> > > > uint8_t smm_enabled;
> > > > ram_addr_t below_4g_mem_size;
> > > > ram_addr_t above_4g_mem_size;
> > > > --
> > > > 1.8.3.1
> > >
> >
> >
> > --
> > Regards,
> > Igor
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 9:13 ` Michael S. Tsirkin
@ 2013-07-28 17:34 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 17:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Sun, 28 Jul 2013 12:13:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> > On Sun, 28 Jul 2013 10:57:12 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > > It turns out that some 32 bit windows guests crash
> > > > if 64 bit PCI hole size is >2G.
> > > > Limit it to 2G for piix and q35 by default.
> > > > User may override default boundaries by using
> > > > "pci_hole64_end " property.
> > > >
> > > > Examples:
> > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > > >
> > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> > >
> > > IMO that's pretty bad as user interfaces go.
> > > In particular if you are not careful you can make
> > > end < start.
> > > Why not set the size, and include a patch that
> > > let people write "1G" or "2G" for convenience?
> > sure as convenience why not, on top of this patches.
> >
> > >
> > > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > > > hw/i386/pc_piix.c | 14 +----------
> > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > > > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > > > include/hw/i386/pc.h | 5 ++--
> > > > include/hw/pci-host/q35.h | 1 +
> > > > 6 files changed, 94 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index b0b98a8..acaeb6c 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > > {
> > > > PcRomPciInfo *info;
> > > > + Object *pci_info;
> > > > +
> > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > > return;
> > > > }
> > > > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > > + if (!pci_info) {
> > > > + pci_info = object_resolve_path("/machine/q35", NULL);
> > > > + if (!pci_info) {
> > > > + return;
> > > > + }
> > > > + }
> > >
> > >
> > > So is the path /machine/i440fx? /machine/i440FX?
> > > /machine/i440FX-pcihost?
> > > There's no way to check this code is right without
> > > actually running it.
> > that drawback of dynamic lookup,
> > QOM paths supposed to be stable.
> >
> > >
> > > How about i44fx_get_pci_info so we can have this
> > > knowledge of paths localized in specific chipset code?
> > I've seen objections from afaerber about this approach, so dropped
> > this idea.
>
> Could we lookup TYPE_PCI_HOST? This will make pc code
> independent of specific machine.
sure, thanks for idea.
>
> --
> MST
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h
2013-07-28 17:19 ` Igor Mammedov
@ 2013-07-28 17:37 ` Andreas Färber
0 siblings, 0 replies; 29+ messages in thread
From: Andreas Färber @ 2013-07-28 17:37 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Gerd Hoffmann, qemu-devel, mst
Am 28.07.2013 19:19, schrieb Igor Mammedov:
> On Sun, 28 Jul 2013 11:54:46 +0200
> Andreas Färber <afaerber@suse.de> wrote:
>
>> Am 28.07.2013 09:29, schrieb Igor Mammedov:
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Missing Reviewed-bys from Gerd and me.
> it changed file form apic.h to ioapic.h, so I haven't felt comfortble adding
> yours Reviewed-bys.
> I sure can add it if it's ok.
Oh, missed that! Still looks fine to me,
Reviewed-by: Andreas Färber <afaerber@suse.de>
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 10:17 ` Andreas Färber
@ 2013-07-28 17:40 ` Igor Mammedov
2013-07-28 19:48 ` Michael S. Tsirkin
2013-07-30 21:34 ` Michael Roth
1 sibling, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 17:40 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, Michael S. Tsirkin
On Sun, 28 Jul 2013 12:17:47 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 28.07.2013 11:11, schrieb Michael S. Tsirkin:
> > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> >> On Sun, 28 Jul 2013 10:57:12 +0300
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>
> >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> >>>>
> >>>> info = g_malloc(sizeof *info);
> >>>> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> >>>> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> >>>> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> >>>> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> >>>> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> >>>> + "pci_hole_start", NULL));
> >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> >>>> + "pci_hole_end", NULL));
> >>>
> >>> Looks wrong.
> >>> object_property_get_int returns a signed int64.
> >>> w32 is unsigned.
> >>> Happens to work but I think we need an explicit API.
>
> That's how QAPI works internally today for any uint64 visitor/property.
> uint64_t is cast to int64_t and back in visitors.
>
> So I'd hope something like
> uint64_t val = (uint64_t) object_property_get_int()
> would work equally well - CC'ing Michael.
>
> >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
> >
> > Not these are 64 bit values, but they need to be
> > unsigned not signed.
> >
> >> but not need for extra API, with fixed property definition
> >> i.e. s/UINT64/UNIT32/ property set code will take care about limits.
> >
> > If you replace these with UINT32 you won't be able to
> > specify values >4G.
> >
> >>> Property names are hard-coded string literals.
> >>> Please add macros to set and get them
> >>> so we can avoid duplicating code.
> >>> E.g.
> >> sure.
> >>
> >>>
> >>> #define PCI_HOST_PROPS...
> >>> static inline get_
> [...]
> >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> >>>> return "0000";
> >>>> }
> >>>>
> >>>> +static Property i440fx_props[] = {
> >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> >>>> + IO_APIC_DEFAULT_ADDRESS),
> >>>> + DEFINE_PROP_END_OF_LIST(),
> >>>> +};
> >>>> +
> >>>
> >>> So we have 4 properties. One of them pci_hole64_end
> >>> is supposed to be set to a value.
> >>> Others should not be touched under any circuimstances.
> >>> Of course if you query properties you have no way
> >>> to know which is which and what are the legal values.
> >>> Ouch.
> >> read-only properties are possible but we would have to drop
> >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
> >
> > Or add DEFINE_PROP_UINT64_RO for this?
> >
> >> user better not to touch these properties since they are mostly internal API.
> >> but if we say it's internal properties then enforcing read-only might be
> >> overkill.
> >> For user friendly property "pci_hole64_size" would be nice to have.
> >
> > So at the moment I do
> >
> > qemu -device i440FX-pcihost,help
> >
> > and this will get all properties.
> >
> > If we add some properties that user can not set
> > they should not appear in this output.
> [snip]
>
> Igor, you can simply use dynamic properties with NULL as setter argument
> for object_property_add*() to achieve that effect.
Yes, I can but it's more boiler plate code just for restricting single
property. And if we have "pci_hole64_size"/default then user set
"pci_hole64_end" would not have any effect, since "pci_hole64_size"
would override it.
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro
2013-07-28 17:24 ` Andreas Färber
@ 2013-07-28 18:05 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2013-07-28 18:05 UTC (permalink / raw)
To: Andreas Färber; +Cc: qemu-devel, mst
On Sun, 28 Jul 2013 19:24:03 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 28.07.2013 19:21, schrieb Igor Mammedov:
> > On Sun, 28 Jul 2013 11:57:14 +0200
> > Andreas Färber <afaerber@suse.de> wrote:
> >
> >> Am 28.07.2013 09:29, schrieb Igor Mammedov:
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>> hw/pci-host/piix.c | 8 ++++++--
> >>> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >>> index 3908860..bf879e7 100644
> >>> --- a/hw/pci-host/piix.c
> >>> +++ b/hw/pci-host/piix.c
> >>> @@ -38,6 +38,10 @@
> >>> * http://download.intel.com/design/chipsets/datashts/29054901.pdf
> >>> */
> >>>
> >>> +#define TYPE_I440FX_PCI_HOST "i440FX-pcihost"
> >>> +#define I440FX_PCI_HOST(obj) \
> >>> + OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST)
> >>
> >> Either Anthony or mst had insisted on PCI_HOST_BRIDGE rather than
> >> PCI_HOST. Other than that looks good, thanks!
> >
> > it's the type cast macro that is missing, so adding it shouldn't hurt,
> > and some day in future we might any way need to add it even if we don't use it
> > now.
>
> Maybe you misunderstood? Please add _BRIDGE to your macros. :)
Yep, sure I'll do it.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 17:40 ` Igor Mammedov
@ 2013-07-28 19:48 ` Michael S. Tsirkin
0 siblings, 0 replies; 29+ messages in thread
From: Michael S. Tsirkin @ 2013-07-28 19:48 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Andreas Färber, qemu-devel
On Sun, Jul 28, 2013 at 07:40:32PM +0200, Igor Mammedov wrote:
> On Sun, 28 Jul 2013 12:17:47 +0200
> Andreas Färber <afaerber@suse.de> wrote:
>
> > Am 28.07.2013 11:11, schrieb Michael S. Tsirkin:
> > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> > >> On Sun, 28 Jul 2013 10:57:12 +0300
> > >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >>
> > >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > >>>>
> > >>>> info = g_malloc(sizeof *info);
> > >>>> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> > >>>> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> > >>>> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> > >>>> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> > >>>> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> > >>>> + "pci_hole_start", NULL));
> > >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> > >>>> + "pci_hole_end", NULL));
> > >>>
> > >>> Looks wrong.
> > >>> object_property_get_int returns a signed int64.
> > >>> w32 is unsigned.
> > >>> Happens to work but I think we need an explicit API.
> >
> > That's how QAPI works internally today for any uint64 visitor/property.
> > uint64_t is cast to int64_t and back in visitors.
> >
> > So I'd hope something like
> > uint64_t val = (uint64_t) object_property_get_int()
> > would work equally well - CC'ing Michael.
> >
> > >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
> > >
> > > Not these are 64 bit values, but they need to be
> > > unsigned not signed.
> > >
> > >> but not need for extra API, with fixed property definition
> > >> i.e. s/UINT64/UNIT32/ property set code will take care about limits.
> > >
> > > If you replace these with UINT32 you won't be able to
> > > specify values >4G.
> > >
> > >>> Property names are hard-coded string literals.
> > >>> Please add macros to set and get them
> > >>> so we can avoid duplicating code.
> > >>> E.g.
> > >> sure.
> > >>
> > >>>
> > >>> #define PCI_HOST_PROPS...
> > >>> static inline get_
> > [...]
> > >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> > >>>> return "0000";
> > >>>> }
> > >>>>
> > >>>> +static Property i440fx_props[] = {
> > >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> > >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> > >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> > >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> > >>>> + IO_APIC_DEFAULT_ADDRESS),
> > >>>> + DEFINE_PROP_END_OF_LIST(),
> > >>>> +};
> > >>>> +
> > >>>
> > >>> So we have 4 properties. One of them pci_hole64_end
> > >>> is supposed to be set to a value.
> > >>> Others should not be touched under any circuimstances.
> > >>> Of course if you query properties you have no way
> > >>> to know which is which and what are the legal values.
> > >>> Ouch.
> > >> read-only properties are possible but we would have to drop
> > >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
> > >
> > > Or add DEFINE_PROP_UINT64_RO for this?
> > >
> > >> user better not to touch these properties since they are mostly internal API.
> > >> but if we say it's internal properties then enforcing read-only might be
> > >> overkill.
> > >> For user friendly property "pci_hole64_size" would be nice to have.
> > >
> > > So at the moment I do
> > >
> > > qemu -device i440FX-pcihost,help
> > >
> > > and this will get all properties.
> > >
> > > If we add some properties that user can not set
> > > they should not appear in this output.
> > [snip]
> >
> > Igor, you can simply use dynamic properties with NULL as setter argument
> > for object_property_add*() to achieve that effect.
> Yes, I can but it's more boiler plate code just for restricting single
> property. And if we have "pci_hole64_size"/default then user set
> "pci_hole64_end" would not have any effect, since "pci_hole64_size"
> would override it.
I don't think we want user to control low level properties such and
_start and _end. _size might be unavoidable but let's limit
it to that.
> > Andreas
> >
> > --
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 17:33 ` Igor Mammedov
@ 2013-07-28 19:51 ` Michael S. Tsirkin
2013-07-29 7:55 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Michael S. Tsirkin @ 2013-07-28 19:51 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Sun, Jul 28, 2013 at 07:33:27PM +0200, Igor Mammedov wrote:
> On Sun, 28 Jul 2013 12:11:42 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> > > On Sun, 28 Jul 2013 10:57:12 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > > > It turns out that some 32 bit windows guests crash
> > > > > if 64 bit PCI hole size is >2G.
> > > > > Limit it to 2G for piix and q35 by default.
> > > > > User may override default boundaries by using
> > > > > "pci_hole64_end " property.
> > > > >
> > > > > Examples:
> > > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > > > >
> > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> > > >
> > > > IMO that's pretty bad as user interfaces go.
> > > > In particular if you are not careful you can make
> > > > end < start.
> > > > Why not set the size, and include a patch that
> > > > let people write "1G" or "2G" for convenience?
> > > sure as convenience why not, on top of this patches.
> >
> > Well because you are specifying end as 0xffffffffffffffff
> > so it's not a multiple of 1G?
> >
> > > >
> > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > > > > hw/i386/pc_piix.c | 14 +----------
> > > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > > > > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > > > > include/hw/i386/pc.h | 5 ++--
> > > > > include/hw/pci-host/q35.h | 1 +
> > > > > 6 files changed, 94 insertions(+), 54 deletions(-)
> > > > >
> > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > > index b0b98a8..acaeb6c 100644
> > > > > --- a/hw/i386/pc.c
> > > > > +++ b/hw/i386/pc.c
> > > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > > > {
> > > > > PcRomPciInfo *info;
> > > > > + Object *pci_info;
> > > > > +
> > > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > > > return;
> > > > > }
> > > > > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > > > + if (!pci_info) {
> > > > > + pci_info = object_resolve_path("/machine/q35", NULL);
> > > > > + if (!pci_info) {
> > > > > + return;
> > > > > + }
> > > > > + }
> > > >
> > > >
> > > > So is the path /machine/i440fx? /machine/i440FX?
> > > > /machine/i440FX-pcihost?
> > > > There's no way to check this code is right without
> > > > actually running it.
> > > that drawback of dynamic lookup,
> > > QOM paths supposed to be stable.
> > >
> > > >
> > > > How about i44fx_get_pci_info so we can have this
> > > > knowledge of paths localized in specific chipset code?
> > > I've seen objections from afaerber about this approach, so dropped
> > > this idea.
> > >
> > > >
> > > > >
> > > > > info = g_malloc(sizeof *info);
> > > > > - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> > > > > - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> > > > > - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> > > > > - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> > > > > + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> > > > > + "pci_hole_start", NULL));
> > > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> > > > > + "pci_hole_end", NULL));
> > > >
> > > > Looks wrong.
> > > > object_property_get_int returns a signed int64.
> > > > w32 is unsigned.
> > > > Happens to work but I think we need an explicit API.
> > > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
> >
> > Not these are 64 bit values, but they need to be
> > unsigned not signed.
> >
> > > but not need for extra API, with fixed property definition
> > > i.e. s/UINT64/UNIT32/ property set code will take care about limits.
> >
> > If you replace these with UINT32 you won't be able to
> > specify values >4G.
> does 32 bit PCI hole has right to be more than 4Gb?
No but the 64 bit one does. 32 one shouldn't be user
controllable at all.
> >
> > > >
> > > > Property names are hard-coded string literals.
> > > > Please add macros to set and get them
> > > > so we can avoid duplicating code.
> > > > E.g.
> > > sure.
> > >
> > > >
> > > > #define PCI_HOST_PROPS...
> > > > static inline get_
> > > >
> > > >
> > > >
> > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> > > > > + "pci_hole64_start", NULL));
> > > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> > > > > + "pci_hole64_end", NULL));
> > > > > /* Pass PCI hole info to guest via a side channel.
> > > > > * Required so guest PCI enumeration does the right thing. */
> > > > > fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> > > > > @@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > > > PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> > > > > PcGuestInfo *guest_info = &guest_info_state->info;
> > > > >
> > > > > - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> > > > > - if (sizeof(hwaddr) == 4) {
> > > > > - guest_info->pci_info.w64.begin = 0;
> > > > > - guest_info->pci_info.w64.end = 0;
> > > > > - } else {
> > > > > - /*
> > > > > - * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > > > - * align address to power of two. Align address at 1G, this makes sure
> > > > > - * it can be exactly covered with a PAT entry even when using huge
> > > > > - * pages.
> > > > > - */
> > > > > - guest_info->pci_info.w64.begin =
> > > > > - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> > > > > - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> > > > > - (0x1ULL << 62);
> > > > > - assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
> > > > > - }
> > > > > -
> > > > > guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> > > > > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> > > > > return guest_info;
> > > > > }
> > > > >
> > > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
> > > > > +{
> > > > > + if (sizeof(hwaddr) == 4) {
> > > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0);
> > > > > + return;
> > > > > + }
> > > > > + /*
> > > > > + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > > > + * align address to power of two. Align address at 1G, this makes sure
> > > > > + * it can be exactly covered with a PAT entry even when using huge
> > > > > + * pages.
> > > > > + */
> > > > > + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> > > > > + if (!pci_info->w64.end) {
> > > > > + /* set default end if it wasn't specified, + 2 Gb past start */
> > > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
> > > > > + }
> > > > > + assert(pci_info->w64.begin < pci_info->w64.end);
> > > > > +}
> > > > > +
> > > > > void pc_acpi_init(const char *default_dsdt)
> > > > > {
> > > > > char *filename;
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index b58c255..ab25458 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> > > > > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > > > > guest_info->has_pci_info = has_pci_info;
> > > > >
> > > > > - /* Set PCI window size the way seabios has always done it. */
> > > > > - /* Power of 2 so bios can cover it with a single MTRR */
> > > > > - if (ram_size <= 0x80000000)
> > > > > - guest_info->pci_info.w32.begin = 0x80000000;
> > > > > - else if (ram_size <= 0xc0000000)
> > > > > - guest_info->pci_info.w32.begin = 0xc0000000;
> > > > > - else
> > > > > - guest_info->pci_info.w32.begin = 0xe0000000;
> > > > > -
> > > > > /* allocate ram and load rom/bios */
> > > > > if (!xen_enabled()) {
> > > > > fw_cfg = pc_memory_init(system_memory,
> > > > > @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > > > > system_memory, system_io, ram_size,
> > > > > below_4g_mem_size,
> > > > > 0x100000000ULL - below_4g_mem_size,
> > > > > - 0x100000000ULL + above_4g_mem_size,
> > > > > - (sizeof(hwaddr) == 4
> > > > > - ? 0
> > > > > - : ((uint64_t)1 << 62)),
> > > > > + above_4g_mem_size,
> > > > > pci_memory, ram_memory);
> > > > > } else {
> > > > > pci_bus = NULL;
> > > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > > > index 4624d04..59a42c5 100644
> > > > > --- a/hw/pci-host/piix.c
> > > > > +++ b/hw/pci-host/piix.c
> > > > > @@ -32,6 +32,7 @@
> > > > > #include "hw/xen/xen.h"
> > > > > #include "hw/pci-host/pam.h"
> > > > > #include "sysemu/sysemu.h"
> > > > > +#include "hw/i386/ioapic.h"
> > > > >
> > > > > /*
> > > > > * I440FX chipset data sheet.
> > > > > @@ -44,6 +45,7 @@
> > > > >
> > > > > typedef struct I440FXState {
> > > > > PCIHostState parent_obj;
> > > > > + PcPciInfo pci_info;
> > > > > } I440FXState;
> > > > >
> > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > > > > @@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > > ram_addr_t ram_size,
> > > > > hwaddr pci_hole_start,
> > > > > hwaddr pci_hole_size,
> > > > > - hwaddr pci_hole64_start,
> > > > > - hwaddr pci_hole64_size,
> > > > > + ram_addr_t above_4g_mem_size,
> > > > > MemoryRegion *pci_address_space,
> > > > > MemoryRegion *ram_memory)
> > > > > {
> > > > > @@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > > PIIX3State *piix3;
> > > > > PCII440FXState *f;
> > > > > unsigned i;
> > > > > + I440FXState *i440fx;
> > > > > + uint64_t pci_hole64_size;
> > > > >
> > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> > > > > s = PCI_HOST_BRIDGE(dev);
> > > > > @@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > > f->system_memory = address_space_mem;
> > > > > f->pci_address_space = pci_address_space;
> > > > > f->ram_memory = ram_memory;
> > > > > +
> > > > > + i440fx = I440FX_PCI_HOST(dev);
> > > > > + /* Set PCI window size the way seabios has always done it. */
> > > > > + /* Power of 2 so bios can cover it with a single MTRR */
> > > > > + if (ram_size <= 0x80000000) {
> > > > > + i440fx->pci_info.w32.begin = 0x80000000;
> > > > > + } else if (ram_size <= 0xc0000000) {
> > > > > + i440fx->pci_info.w32.begin = 0xc0000000;
> > > > > + } else {
> > > > > + i440fx->pci_info.w32.begin = 0xe0000000;
> > > > > + }
> > > > > +
> > > > > memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > > > > pci_hole_start, pci_hole_size);
> > > > > memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > > > > +
> > > > > + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
> > > > > + pci_hole64_size = range_size(i440fx->pci_info.w64);
> > > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > > > > f->pci_address_space,
> > > > > - pci_hole64_start, pci_hole64_size);
> > > > > + i440fx->pci_info.w64.begin, pci_hole64_size);
> > > > > if (pci_hole64_size) {
> > > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> > > > > + memory_region_add_subregion(f->system_memory,
> > > > > + i440fx->pci_info.w64.begin,
> > > > > &f->pci_hole_64bit);
> > > > > }
> > > > > memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> > > > > @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> > > > > return "0000";
> > > > > }
> > > > >
> > > > > +static Property i440fx_props[] = {
> > > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> > > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> > > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> > > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> > > > > + IO_APIC_DEFAULT_ADDRESS),
> > > > > + DEFINE_PROP_END_OF_LIST(),
> > > > > +};
> > > > > +
> > > >
> > > > So we have 4 properties. One of them pci_hole64_end
> > > > is supposed to be set to a value.
> > > > Others should not be touched under any circuimstances.
> > > > Of course if you query properties you have no way
> > > > to know which is which and what are the legal values.
> > > > Ouch.
> > > read-only properties are possible but we would have to drop
> > > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
> >
> > Or add DEFINE_PROP_UINT64_RO for this?
> it might be the way to do it.
>
> >
> > > user better not to touch these properties since they are mostly internal API.
> > > but if we say it's internal properties then enforcing read-only might be
> > > overkill.
> > > For user friendly property "pci_hole64_size" would be nice to have.
> >
> > So at the moment I do
> >
> > qemu -device i440FX-pcihost,help
> >
> > and this will get all properties.
> >
> > If we add some properties that user can not set
> > they should not appear in this output.
> with QOM all around I wouldn't say so, it could be property only for internal
> purposes and QOM properties do not care about whether they are visible or
> not to user so far. I guess we could document it in code like do not
> touch /internal or ...
>
>
> >
> >
> > > >
> > > > > static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > > > > {
> > > > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > > > @@ -638,6 +666,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> > > > > dc->realize = i440fx_pcihost_realize;
> > > > > dc->fw_name = "pci";
> > > > > dc->no_user = 1;
> > > > > + dc->props = i440fx_props;
> > > > > }
> > > > >
> > > > > static const TypeInfo i440fx_pcihost_info = {
> > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > > index 6b1b3b7..a6936e6 100644
> > > > > --- a/hw/pci-host/q35.c
> > > > > +++ b/hw/pci-host/q35.c
> > > > > @@ -67,6 +67,21 @@ static const char *q35_host_root_bus_path(PCIHostState *host_bridge,
> > > > > static Property mch_props[] = {
> > > > > DEFINE_PROP_UINT64("MCFG", Q35PCIHost, parent_obj.base_addr,
> > > > > MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> > > > > + DEFINE_PROP_UINT64("pci_hole64_start", Q35PCIHost,
> > > > > + mch.pci_info.w64.begin, 0),
> > > > > + DEFINE_PROP_UINT64("pci_hole64_end", Q35PCIHost,
> > > > > + mch.pci_info.w64.end, 0),
> > > > > + /* Leave enough space for the biggest MCFG BAR */
> > > > > + /* TODO: this matches current bios behaviour, but
> > > > > + * it's not a power of two, which means an MTRR
> > > > > + * can't cover it exactly.
> > > > > + */
> > > > > + DEFINE_PROP_UINT64("pci_hole_start", Q35PCIHost,
> > > > > + mch.pci_info.w32.begin,
> > > > > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > > > > + MCH_HOST_BRIDGE_PCIEXBAR_MAX),
> > > > > + DEFINE_PROP_UINT64("pci_hole_end", Q35PCIHost,
> > > > > + mch.pci_info.w32.end, IO_APIC_DEFAULT_ADDRESS),
> > > > > DEFINE_PROP_END_OF_LIST(),
> > > > > };
> > > > >
> > > > > @@ -255,14 +270,6 @@ static int mch_init(PCIDevice *d)
> > > > > hwaddr pci_hole64_size;
> > > > > MCHPCIState *mch = MCH_PCI_DEVICE(d);
> > > > >
> > > > > - /* Leave enough space for the biggest MCFG BAR */
> > > > > - /* TODO: this matches current bios behaviour, but
> > > > > - * it's not a power of two, which means an MTRR
> > > > > - * can't cover it exactly.
> > > > > - */
> > > > > - mch->guest_info->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> > > > > - MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> > > > > -
> > > > > /* setup pci memory regions */
> > > > > memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
> > > > > mch->pci_address_space,
> > > > > @@ -270,15 +277,16 @@ static int mch_init(PCIDevice *d)
> > > > > 0x100000000ULL - mch->below_4g_mem_size);
> > > > > memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
> > > > > &mch->pci_hole);
> > > > > - pci_hole64_size = (sizeof(hwaddr) == 4 ? 0 :
> > > > > - ((uint64_t)1 << 62));
> > > > > +
> > > > > + pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size);
> > > > > + pci_hole64_size = range_size(mch->pci_info.w64);
> > > > > memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
> > > > > mch->pci_address_space,
> > > > > - 0x100000000ULL + mch->above_4g_mem_size,
> > > > > + mch->pci_info.w64.begin,
> > > > > pci_hole64_size);
> > > > > if (pci_hole64_size) {
> > > > > memory_region_add_subregion(mch->system_memory,
> > > > > - 0x100000000ULL + mch->above_4g_mem_size,
> > > > > + mch->pci_info.w64.begin,
> > > > > &mch->pci_hole_64bit);
> > > > > }
> > > > > /* smram */
> > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > index 7fb97b0..ab747b7 100644
> > > > > --- a/include/hw/i386/pc.h
> > > > > +++ b/include/hw/i386/pc.h
> > > > > @@ -18,7 +18,6 @@ typedef struct PcPciInfo {
> > > > > } PcPciInfo;
> > > > >
> > > > > struct PcGuestInfo {
> > > > > - PcPciInfo pci_info;
> > > > > bool has_pci_info;
> > > > > FWCfgState *fw_cfg;
> > > > > };
> > > > > @@ -100,6 +99,7 @@ void pc_acpi_init(const char *default_dsdt);
> > > > >
> > > > > PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > > > ram_addr_t above_4g_mem_size);
> > > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start);
> > > > >
> > > > > FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> > > > > const char *kernel_filename,
> > > > > @@ -150,8 +150,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> > > > > ram_addr_t ram_size,
> > > > > hwaddr pci_hole_start,
> > > > > hwaddr pci_hole_size,
> > > > > - hwaddr pci_hole64_start,
> > > > > - hwaddr pci_hole64_size,
> > > > > + ram_addr_t above_4g_mem_size,
> > > > > MemoryRegion *pci_memory,
> > > > > MemoryRegion *ram_memory);
> > > > >
> > > > > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
> > > > > index 3cb631e..3a9e04b 100644
> > > > > --- a/include/hw/pci-host/q35.h
> > > > > +++ b/include/hw/pci-host/q35.h
> > > > > @@ -55,6 +55,7 @@ typedef struct MCHPCIState {
> > > > > MemoryRegion smram_region;
> > > > > MemoryRegion pci_hole;
> > > > > MemoryRegion pci_hole_64bit;
> > > > > + PcPciInfo pci_info;
> > > > > uint8_t smm_enabled;
> > > > > ram_addr_t below_4g_mem_size;
> > > > > ram_addr_t above_4g_mem_size;
> > > > > --
> > > > > 1.8.3.1
> > > >
> > >
> > >
> > > --
> > > Regards,
> > > Igor
> >
>
>
> --
> Regards,
> Igor
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 19:51 ` Michael S. Tsirkin
@ 2013-07-29 7:55 ` Igor Mammedov
2013-07-29 8:16 ` Michael S. Tsirkin
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2013-07-29 7:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Sun, 28 Jul 2013 22:51:57 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Jul 28, 2013 at 07:33:27PM +0200, Igor Mammedov wrote:
> > On Sun, 28 Jul 2013 12:11:42 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> > > > On Sun, 28 Jul 2013 10:57:12 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> > > > > > It turns out that some 32 bit windows guests crash
> > > > > > if 64 bit PCI hole size is >2G.
> > > > > > Limit it to 2G for piix and q35 by default.
> > > > > > User may override default boundaries by using
> > > > > > "pci_hole64_end " property.
> > > > > >
> > > > > > Examples:
> > > > > > -global i440FX-pcihost.pci_hole64_end=0xffffffffffffffff
> > > > > >
> > > > > > -global q35-pcihost.pci_hole64_end=0xffffffffffffffff
> > > > >
> > > > > IMO that's pretty bad as user interfaces go.
> > > > > In particular if you are not careful you can make
> > > > > end < start.
> > > > > Why not set the size, and include a patch that
> > > > > let people write "1G" or "2G" for convenience?
> > > > sure as convenience why not, on top of this patches.
> > >
> > > Well because you are specifying end as 0xffffffffffffffff
> > > so it's not a multiple of 1G?
> > >
> > > > >
> > > > > > Reported-by: Igor Mammedov <imammedo@redhat.com>,
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > > hw/i386/pc.c | 59 +++++++++++++++++++++++++++++------------------
> > > > > > hw/i386/pc_piix.c | 14 +----------
> > > > > > hw/pci-host/piix.c | 37 +++++++++++++++++++++++++----
> > > > > > hw/pci-host/q35.c | 32 +++++++++++++++----------
> > > > > > include/hw/i386/pc.h | 5 ++--
> > > > > > include/hw/pci-host/q35.h | 1 +
> > > > > > 6 files changed, 94 insertions(+), 54 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > > > index b0b98a8..acaeb6c 100644
> > > > > > --- a/hw/i386/pc.c
> > > > > > +++ b/hw/i386/pc.c
> > > > > > @@ -1003,15 +1003,28 @@ typedef struct PcRomPciInfo {
> > > > > > static void pc_fw_cfg_guest_info(PcGuestInfo *guest_info)
> > > > > > {
> > > > > > PcRomPciInfo *info;
> > > > > > + Object *pci_info;
> > > > > > +
> > > > > > if (!guest_info->has_pci_info || !guest_info->fw_cfg) {
> > > > > > return;
> > > > > > }
> > > > > > + pci_info = object_resolve_path("/machine/i440fx", NULL);
> > > > > > + if (!pci_info) {
> > > > > > + pci_info = object_resolve_path("/machine/q35", NULL);
> > > > > > + if (!pci_info) {
> > > > > > + return;
> > > > > > + }
> > > > > > + }
> > > > >
> > > > >
> > > > > So is the path /machine/i440fx? /machine/i440FX?
> > > > > /machine/i440FX-pcihost?
> > > > > There's no way to check this code is right without
> > > > > actually running it.
> > > > that drawback of dynamic lookup,
> > > > QOM paths supposed to be stable.
> > > >
> > > > >
> > > > > How about i44fx_get_pci_info so we can have this
> > > > > knowledge of paths localized in specific chipset code?
> > > > I've seen objections from afaerber about this approach, so dropped
> > > > this idea.
> > > >
> > > > >
> > > > > >
> > > > > > info = g_malloc(sizeof *info);
> > > > > > - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> > > > > > - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> > > > > > - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> > > > > > - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> > > > > > + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> > > > > > + "pci_hole_start", NULL));
> > > > > > + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> > > > > > + "pci_hole_end", NULL));
> > > > >
> > > > > Looks wrong.
> > > > > object_property_get_int returns a signed int64.
> > > > > w32 is unsigned.
> > > > > Happens to work but I think we need an explicit API.
> > > > I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
> > >
> > > Not these are 64 bit values, but they need to be
> > > unsigned not signed.
> > >
> > > > but not need for extra API, with fixed property definition
> > > > i.e. s/UINT64/UNIT32/ property set code will take care about limits.
> > >
> > > If you replace these with UINT32 you won't be able to
> > > specify values >4G.
> > does 32 bit PCI hole has right to be more than 4Gb?
>
> No but the 64 bit one does. 32 one shouldn't be user
> controllable at all.
>
> > >
> > > > >
> > > > > Property names are hard-coded string literals.
> > > > > Please add macros to set and get them
> > > > > so we can avoid duplicating code.
> > > > > E.g.
> > > > sure.
> > > >
> > > > >
> > > > > #define PCI_HOST_PROPS...
> > > > > static inline get_
> > > > >
> > > > >
> > > > >
> > > > > > + info->w64_min = cpu_to_le64(object_property_get_int(pci_info,
> > > > > > + "pci_hole64_start", NULL));
> > > > > > + info->w64_max = cpu_to_le64(object_property_get_int(pci_info,
> > > > > > + "pci_hole64_end", NULL));
> > > > > > /* Pass PCI hole info to guest via a side channel.
> > > > > > * Required so guest PCI enumeration does the right thing. */
> > > > > > fw_cfg_add_file(guest_info->fw_cfg, "etc/pci-info", info, sizeof *info);
> > > > > > @@ -1037,29 +1050,31 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > > > > > PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> > > > > > PcGuestInfo *guest_info = &guest_info_state->info;
> > > > > >
> > > > > > - guest_info->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> > > > > > - if (sizeof(hwaddr) == 4) {
> > > > > > - guest_info->pci_info.w64.begin = 0;
> > > > > > - guest_info->pci_info.w64.end = 0;
> > > > > > - } else {
> > > > > > - /*
> > > > > > - * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > > > > - * align address to power of two. Align address at 1G, this makes sure
> > > > > > - * it can be exactly covered with a PAT entry even when using huge
> > > > > > - * pages.
> > > > > > - */
> > > > > > - guest_info->pci_info.w64.begin =
> > > > > > - ROUND_UP((0x1ULL << 32) + above_4g_mem_size, 0x1ULL << 30);
> > > > > > - guest_info->pci_info.w64.end = guest_info->pci_info.w64.begin +
> > > > > > - (0x1ULL << 62);
> > > > > > - assert(guest_info->pci_info.w64.begin <= guest_info->pci_info.w64.end);
> > > > > > - }
> > > > > > -
> > > > > > guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> > > > > > qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> > > > > > return guest_info;
> > > > > > }
> > > > > >
> > > > > > +void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start)
> > > > > > +{
> > > > > > + if (sizeof(hwaddr) == 4) {
> > > > > > + memset(&pci_info->w64, sizeof(pci_info->w64), 0);
> > > > > > + return;
> > > > > > + }
> > > > > > + /*
> > > > > > + * BIOS does not set MTRR entries for the 64 bit window, so no need to
> > > > > > + * align address to power of two. Align address at 1G, this makes sure
> > > > > > + * it can be exactly covered with a PAT entry even when using huge
> > > > > > + * pages.
> > > > > > + */
> > > > > > + pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
> > > > > > + if (!pci_info->w64.end) {
> > > > > > + /* set default end if it wasn't specified, + 2 Gb past start */
> > > > > > + pci_info->w64.end = pci_info->w64.begin + (0x1ULL << 31);
> > > > > > + }
> > > > > > + assert(pci_info->w64.begin < pci_info->w64.end);
> > > > > > +}
> > > > > > +
> > > > > > void pc_acpi_init(const char *default_dsdt)
> > > > > > {
> > > > > > char *filename;
> > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > > index b58c255..ab25458 100644
> > > > > > --- a/hw/i386/pc_piix.c
> > > > > > +++ b/hw/i386/pc_piix.c
> > > > > > @@ -129,15 +129,6 @@ static void pc_init1(MemoryRegion *system_memory,
> > > > > > guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
> > > > > > guest_info->has_pci_info = has_pci_info;
> > > > > >
> > > > > > - /* Set PCI window size the way seabios has always done it. */
> > > > > > - /* Power of 2 so bios can cover it with a single MTRR */
> > > > > > - if (ram_size <= 0x80000000)
> > > > > > - guest_info->pci_info.w32.begin = 0x80000000;
> > > > > > - else if (ram_size <= 0xc0000000)
> > > > > > - guest_info->pci_info.w32.begin = 0xc0000000;
> > > > > > - else
> > > > > > - guest_info->pci_info.w32.begin = 0xe0000000;
> > > > > > -
> > > > > > /* allocate ram and load rom/bios */
> > > > > > if (!xen_enabled()) {
> > > > > > fw_cfg = pc_memory_init(system_memory,
> > > > > > @@ -160,10 +151,7 @@ static void pc_init1(MemoryRegion *system_memory,
> > > > > > system_memory, system_io, ram_size,
> > > > > > below_4g_mem_size,
> > > > > > 0x100000000ULL - below_4g_mem_size,
> > > > > > - 0x100000000ULL + above_4g_mem_size,
> > > > > > - (sizeof(hwaddr) == 4
> > > > > > - ? 0
> > > > > > - : ((uint64_t)1 << 62)),
> > > > > > + above_4g_mem_size,
> > > > > > pci_memory, ram_memory);
> > > > > > } else {
> > > > > > pci_bus = NULL;
> > > > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > > > > index 4624d04..59a42c5 100644
> > > > > > --- a/hw/pci-host/piix.c
> > > > > > +++ b/hw/pci-host/piix.c
> > > > > > @@ -32,6 +32,7 @@
> > > > > > #include "hw/xen/xen.h"
> > > > > > #include "hw/pci-host/pam.h"
> > > > > > #include "sysemu/sysemu.h"
> > > > > > +#include "hw/i386/ioapic.h"
> > > > > >
> > > > > > /*
> > > > > > * I440FX chipset data sheet.
> > > > > > @@ -44,6 +45,7 @@
> > > > > >
> > > > > > typedef struct I440FXState {
> > > > > > PCIHostState parent_obj;
> > > > > > + PcPciInfo pci_info;
> > > > > > } I440FXState;
> > > > > >
> > > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */
> > > > > > @@ -247,8 +249,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > > > ram_addr_t ram_size,
> > > > > > hwaddr pci_hole_start,
> > > > > > hwaddr pci_hole_size,
> > > > > > - hwaddr pci_hole64_start,
> > > > > > - hwaddr pci_hole64_size,
> > > > > > + ram_addr_t above_4g_mem_size,
> > > > > > MemoryRegion *pci_address_space,
> > > > > > MemoryRegion *ram_memory)
> > > > > > {
> > > > > > @@ -259,6 +260,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > > > PIIX3State *piix3;
> > > > > > PCII440FXState *f;
> > > > > > unsigned i;
> > > > > > + I440FXState *i440fx;
> > > > > > + uint64_t pci_hole64_size;
> > > > > >
> > > > > > dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST);
> > > > > > s = PCI_HOST_BRIDGE(dev);
> > > > > > @@ -274,14 +277,30 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > > > > > f->system_memory = address_space_mem;
> > > > > > f->pci_address_space = pci_address_space;
> > > > > > f->ram_memory = ram_memory;
> > > > > > +
> > > > > > + i440fx = I440FX_PCI_HOST(dev);
> > > > > > + /* Set PCI window size the way seabios has always done it. */
> > > > > > + /* Power of 2 so bios can cover it with a single MTRR */
> > > > > > + if (ram_size <= 0x80000000) {
> > > > > > + i440fx->pci_info.w32.begin = 0x80000000;
> > > > > > + } else if (ram_size <= 0xc0000000) {
> > > > > > + i440fx->pci_info.w32.begin = 0xc0000000;
> > > > > > + } else {
> > > > > > + i440fx->pci_info.w32.begin = 0xe0000000;
> > > > > > + }
> > > > > > +
> > > > > > memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > > > > > pci_hole_start, pci_hole_size);
> > > > > > memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > > > > > +
> > > > > > + pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size);
> > > > > > + pci_hole64_size = range_size(i440fx->pci_info.w64);
> > > > > > memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > > > > > f->pci_address_space,
> > > > > > - pci_hole64_start, pci_hole64_size);
> > > > > > + i440fx->pci_info.w64.begin, pci_hole64_size);
> > > > > > if (pci_hole64_size) {
> > > > > > - memory_region_add_subregion(f->system_memory, pci_hole64_start,
> > > > > > + memory_region_add_subregion(f->system_memory,
> > > > > > + i440fx->pci_info.w64.begin,
> > > > > > &f->pci_hole_64bit);
> > > > > > }
> > > > > > memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> > > > > > @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> > > > > > return "0000";
> > > > > > }
> > > > > >
> > > > > > +static Property i440fx_props[] = {
> > > > > > + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> > > > > > + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> > > > > > + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> > > > > > + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> > > > > > + IO_APIC_DEFAULT_ADDRESS),
> > > > > > + DEFINE_PROP_END_OF_LIST(),
> > > > > > +};
> > > > > > +
> > > > >
> > > > > So we have 4 properties. One of them pci_hole64_end
> > > > > is supposed to be set to a value.
> > > > > Others should not be touched under any circuimstances.
> > > > > Of course if you query properties you have no way
> > > > > to know which is which and what are the legal values.
> > > > > Ouch.
> > > > read-only properties are possible but we would have to drop
> > > > usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
> > >
> > > Or add DEFINE_PROP_UINT64_RO for this?
> > it might be the way to do it.
I've meant to do it only for 32-bit PCI hole, I'm sorry for being unclear.
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-29 7:55 ` Igor Mammedov
@ 2013-07-29 8:16 ` Michael S. Tsirkin
0 siblings, 0 replies; 29+ messages in thread
From: Michael S. Tsirkin @ 2013-07-29 8:16 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Mon, Jul 29, 2013 at 09:55:32AM +0200, Igor Mammedov wrote:
> > > > > but not need for extra API, with fixed property definition
> > > > > i.e. s/UINT64/UNIT32/ property set code will take care about limits.
> > > >
> > > > If you replace these with UINT32 you won't be able to
> > > > specify values >4G.
> > > does 32 bit PCI hole has right to be more than 4Gb?
> >
> > No but the 64 bit one does. 32 one shouldn't be user
> > controllable at all.
...
> I've meant to do it only for 32-bit PCI hole, I'm sorry for being unclear.
>
> [...]
That's OK then.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
2013-07-28 10:17 ` Andreas Färber
2013-07-28 17:40 ` Igor Mammedov
@ 2013-07-30 21:34 ` Michael Roth
1 sibling, 0 replies; 29+ messages in thread
From: Michael Roth @ 2013-07-30 21:34 UTC (permalink / raw)
To: Andreas Färber, Michael S. Tsirkin, Igor Mammedov; +Cc: qemu-devel
Quoting Andreas Färber (2013-07-28 05:17:47)
> Am 28.07.2013 11:11, schrieb Michael S. Tsirkin:
> > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote:
> >> On Sun, 28 Jul 2013 10:57:12 +0300
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>
> >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> >>>>
> >>>> info = g_malloc(sizeof *info);
> >>>> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> >>>> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> >>>> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> >>>> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> >>>> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> >>>> + "pci_hole_start", NULL));
> >>>> + info->w32_max = cpu_to_le64(object_property_get_int(pci_info,
> >>>> + "pci_hole_end", NULL));
> >>>
> >>> Looks wrong.
> >>> object_property_get_int returns a signed int64.
> >>> w32 is unsigned.
> >>> Happens to work but I think we need an explicit API.
>
> That's how QAPI works internally today for any uint64 visitor/property.
> uint64_t is cast to int64_t and back in visitors.
>
> So I'd hope something like
> uint64_t val = (uint64_t) object_property_get_int()
> would work equally well - CC'ing Michael.
It actually depends on the 'wire'/'serialized' encoding of the underlying
visitor implementation. In this case we're 'serializing' into a QObject
via QmpOutputVisitor, where all integers are actually stored as a
QInt/int64 anyway, so this cast is unavoidable in this case regardless
of what QAPI interface we use: we'll always end up storing as an int64,
requiring us to re-cast to get back to original value. Best we can
achieve is burying the cast deeper (or significantly re-working how
QObject/QInt works)
There is additional bounds checking performed prior to serialization
if the serialized type is less than 64 bits though, so we'd probably
want to add fixed-width accessors if we found ourselves in a situation
where we needed to cast to a smaller datatype than the original.
It's also worth noting that visiting uint64 types using int64 visitor
interfaces isn't universally guaranteed to work: for certain visitor
implementations the serialized encodings may not be compatible with
one another. But in this case we should be good.
>
> >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/
> >
> > Not these are 64 bit values, but they need to be
> > unsigned not signed.
> > > >> but not need for extra API, with fixed property definition
> >> i.e. s/UINT64/UNIT32/ property set code will take care about limits.
> >
> > If you replace these with UINT32 you won't be able to
> > specify values >4G.
> >
> >>> Property names are hard-coded string literals.
> >>> Please add macros to set and get them
> >>> so we can avoid duplicating code.
> >>> E.g.
> >> sure.
> >>
> >>>
> >>> #define PCI_HOST_PROPS...
> >>> static inline get_
> [...]
> >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
> >>>> return "0000";
> >>>> }
> >>>>
> >>>> +static Property i440fx_props[] = {
> >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w64.begin, 0),
> >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.end, 0),
> >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.begin, 0),
> >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.end,
> >>>> + IO_APIC_DEFAULT_ADDRESS),
> >>>> + DEFINE_PROP_END_OF_LIST(),
> >>>> +};
> >>>> +
> >>>
> >>> So we have 4 properties. One of them pci_hole64_end
> >>> is supposed to be set to a value.
> >>> Others should not be touched under any circuimstances.
> >>> Of course if you query properties you have no way
> >>> to know which is which and what are the legal values.
> >>> Ouch.
> >> read-only properties are possible but we would have to drop
> >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in PropertyInfo,
> >
> > Or add DEFINE_PROP_UINT64_RO for this?
> >
> >> user better not to touch these properties since they are mostly internal API.
> >> but if we say it's internal properties then enforcing read-only might be
> >> overkill.
> >> For user friendly property "pci_hole64_size" would be nice to have.
> >
> > So at the moment I do
> >
> > qemu -device i440FX-pcihost,help
> >
> > and this will get all properties.
> >
> > If we add some properties that user can not set
> > they should not appear in this output.
> [snip]
>
> Igor, you can simply use dynamic properties with NULL as setter argument
> for object_property_add*() to achieve that effect.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2013-07-30 21:34 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-28 7:29 [Qemu-devel] [PATCH 0/6 v2 for-1.6] pc: limit 64 bit hole to 2G by default Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 1/6] pc: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/ioapic.h Igor Mammedov
2013-07-28 9:54 ` Andreas Färber
2013-07-28 17:19 ` Igor Mammedov
2013-07-28 17:37 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 2/6] pc: add I440FX QOM cast macro Igor Mammedov
2013-07-28 9:57 ` Andreas Färber
2013-07-28 17:21 ` Igor Mammedov
2013-07-28 17:24 ` Andreas Färber
2013-07-28 18:05 ` Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 3/6] utils: add range_size() wrapper Igor Mammedov
2013-07-28 7:29 ` [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else Igor Mammedov
2013-07-28 10:07 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine Igor Mammedov
2013-07-28 10:08 ` Andreas Färber
2013-07-28 7:29 ` [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default Igor Mammedov
2013-07-28 7:57 ` Michael S. Tsirkin
2013-07-28 8:21 ` Igor Mammedov
2013-07-28 9:11 ` Michael S. Tsirkin
2013-07-28 10:17 ` Andreas Färber
2013-07-28 17:40 ` Igor Mammedov
2013-07-28 19:48 ` Michael S. Tsirkin
2013-07-30 21:34 ` Michael Roth
2013-07-28 17:33 ` Igor Mammedov
2013-07-28 19:51 ` Michael S. Tsirkin
2013-07-29 7:55 ` Igor Mammedov
2013-07-29 8:16 ` Michael S. Tsirkin
2013-07-28 9:13 ` Michael S. Tsirkin
2013-07-28 17:34 ` Igor Mammedov
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).