[Qemu-devel] [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Qemu-devel] [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Don Slutz]

Changes v2 to v3:
  Stefano Stabellini:
    Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
    Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
       Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
       below_4g_mem_size.

Changes v1 to v2:
  Michael S. Tsirkin:
    Rename option.
    Only add it to machine types that support it.
  Split into 4 parts.

1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout

  This looks to be a possible bug that has yet to be found.
  below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
  (pc_guest_info_init) which are currently not "correct".  This and
  4/4 change the same lines.

2/4 -- GlobalProperty: Display warning about unused -global

    My testing showed that setting a global property on an object
    that is not used is not reported at all.  This is added to help
    when the new global is set but not used.  The negative not_used
    was picked so that all static objects are assumed to be used
    even when they are not.

3/4 -- pc & q35: Add new object pc-memory-layout

  The objects that it might make sense to add this property to all
  get created too late.  So add a new object just to hold this
  property.  Name it so that it is expected that only pc (and q35)
  machine types support it.

4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init

  Seprate the xen only part of the change.  Currectly based on patch 1/4

Don Slutz (4):
  xen-all: Fix xen_hvm_init() to adjust pc memory layout.
  GlobalProperty: Display warning about unused -global
  pc & q35: Add new object pc-memory-layout.
  xen-all: Pass max_ram_below_4g to xen_hvm_init.

 hw/core/qdev-properties-system.c |  1 +
 hw/core/qdev-properties.c        | 15 ++++++++++++
 hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
 hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
 hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
 include/hw/i386/pc.h             |  2 ++
 include/hw/qdev-core.h           |  1 +
 include/hw/qdev-properties.h     |  1 +
 include/hw/xen/xen.h             |  3 ++-
 vl.c                             |  2 ++
 xen-all.c                        | 46 +++++++++++++++++++----------------
 xen-stub.c                       |  3 ++-
 12 files changed, 165 insertions(+), 53 deletions(-)

-- 
1.8.4

[PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Don Slutz]

Changes v2 to v3:
  Stefano Stabellini:
    Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
    Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
       Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
       below_4g_mem_size.

Changes v1 to v2:
  Michael S. Tsirkin:
    Rename option.
    Only add it to machine types that support it.
  Split into 4 parts.

1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout

  This looks to be a possible bug that has yet to be found.
  below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
  (pc_guest_info_init) which are currently not "correct".  This and
  4/4 change the same lines.

2/4 -- GlobalProperty: Display warning about unused -global

    My testing showed that setting a global property on an object
    that is not used is not reported at all.  This is added to help
    when the new global is set but not used.  The negative not_used
    was picked so that all static objects are assumed to be used
    even when they are not.

3/4 -- pc & q35: Add new object pc-memory-layout

  The objects that it might make sense to add this property to all
  get created too late.  So add a new object just to hold this
  property.  Name it so that it is expected that only pc (and q35)
  machine types support it.

4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init

  Seprate the xen only part of the change.  Currectly based on patch 1/4

Don Slutz (4):
  xen-all: Fix xen_hvm_init() to adjust pc memory layout.
  GlobalProperty: Display warning about unused -global
  pc & q35: Add new object pc-memory-layout.
  xen-all: Pass max_ram_below_4g to xen_hvm_init.

 hw/core/qdev-properties-system.c |  1 +
 hw/core/qdev-properties.c        | 15 ++++++++++++
 hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
 hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
 hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
 include/hw/i386/pc.h             |  2 ++
 include/hw/qdev-core.h           |  1 +
 include/hw/qdev-properties.h     |  1 +
 include/hw/xen/xen.h             |  3 ++-
 vl.c                             |  2 ++
 xen-all.c                        | 46 +++++++++++++++++++----------------
 xen-stub.c                       |  3 ++-
 12 files changed, 165 insertions(+), 53 deletions(-)

-- 
1.8.4

[Qemu-devel] [PATCH v3 1/4] xen-all: Fix xen_hvm_init() to adjust pc memory layout.

[Don Slutz]

This is just below_4g_mem_size and above_4g_mem_size which is used later in QEMU.

Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/i386/pc_piix.c    | 31 ++++++++++++++++---------------
 hw/i386/pc_q35.c     | 29 +++++++++++++++--------------
 include/hw/xen/xen.h |  3 ++-
 xen-all.c            | 24 ++++++++++++++----------
 xen-stub.c           |  3 ++-
 5 files changed, 49 insertions(+), 41 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7930a26..bd52f6e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -96,21 +96,6 @@ static void pc_init1(QEMUMachineInitArgs *args,
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
 
-    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
-        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
-        exit(1);
-    }
-
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(args->cpu_model, icc_bridge);
-
-    if (kvm_enabled() && kvmclock_enabled) {
-        kvmclock_create();
-    }
-
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
      * If it doesn't, we need to split it in chunks below and above 4G.
      * In any case, try to make sure that guest addresses aligned at
@@ -127,6 +112,22 @@ static void pc_init1(QEMUMachineInitArgs *args,
         below_4g_mem_size = args->ram_size;
     }
 
+    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
+                                      &ram_memory) != 0) {
+        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
+        exit(1);
+    }
+
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
+
+    pc_cpus_init(args->cpu_model, icc_bridge);
+
+    if (kvm_enabled() && kvmclock_enabled) {
+        kvmclock_create();
+    }
+
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c844dc2..6e34fe1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -83,20 +83,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
 
-    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
-        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
-        exit(1);
-    }
-
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(args->cpu_model, icc_bridge);
-    pc_acpi_init("q35-acpi-dsdt.aml");
-
-    kvmclock_create();
-
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
      * also known as MMCFG).
@@ -115,6 +101,21 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
         below_4g_mem_size = args->ram_size;
     }
 
+    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
+                                      &ram_memory) != 0) {
+        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
+        exit(1);
+    }
+
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
+
+    pc_cpus_init(args->cpu_model, icc_bridge);
+    pc_acpi_init("q35-acpi-dsdt.aml");
+
+    kvmclock_create();
+
     /* pci enabled */
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 9d549fc..0f3942e 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -37,10 +37,11 @@ void xen_cmos_set_s3_resume(void *opaque, int irq, int level);
 qemu_irq *xen_interrupt_controller_init(void);
 
 int xen_init(QEMUMachine *machine);
-int xen_hvm_init(MemoryRegion **ram_memory);
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
+int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
+                 MemoryRegion **ram_memory);
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
                    struct MemoryRegion *mr);
 void xen_modified_memory(ram_addr_t start, ram_addr_t length);
diff --git a/xen-all.c b/xen-all.c
index ba34739..c64300c 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -155,10 +155,11 @@ qemu_irq *xen_interrupt_controller_init(void)
 
 /* Memory Ops */
 
-static void xen_ram_init(ram_addr_t ram_size, MemoryRegion **ram_memory_p)
+static void xen_ram_init(ram_addr_t *below_4g_mem_size,
+                         ram_addr_t *above_4g_mem_size,
+                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
 {
     MemoryRegion *sysmem = get_system_memory();
-    ram_addr_t below_4g_mem_size, above_4g_mem_size = 0;
     ram_addr_t block_len;
 
     block_len = ram_size;
@@ -173,10 +174,11 @@ static void xen_ram_init(ram_addr_t ram_size, MemoryRegion **ram_memory_p)
     vmstate_register_ram_global(&ram_memory);
 
     if (ram_size >= HVM_BELOW_4G_RAM_END) {
-        above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
-        below_4g_mem_size = HVM_BELOW_4G_RAM_END;
+        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
+        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
     } else {
-        below_4g_mem_size = ram_size;
+        *above_4g_mem_size = 0;
+        *below_4g_mem_size = ram_size;
     }
 
     memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
@@ -189,12 +191,13 @@ static void xen_ram_init(ram_addr_t ram_size, MemoryRegion **ram_memory_p)
      * the Options ROM, so it is registered here as RAM.
      */
     memory_region_init_alias(&ram_lo, NULL, "xen.ram.lo",
-                             &ram_memory, 0xc0000, below_4g_mem_size - 0xc0000);
+                             &ram_memory, 0xc0000,
+                             *below_4g_mem_size - 0xc0000);
     memory_region_add_subregion(sysmem, 0xc0000, &ram_lo);
-    if (above_4g_mem_size > 0) {
+    if (*above_4g_mem_size > 0) {
         memory_region_init_alias(&ram_hi, NULL, "xen.ram.hi",
                                  &ram_memory, 0x100000000ULL,
-                                 above_4g_mem_size);
+                                 *above_4g_mem_size);
         memory_region_add_subregion(sysmem, 0x100000000ULL, &ram_hi);
     }
 }
@@ -1066,7 +1069,8 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
-int xen_hvm_init(MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
+                 MemoryRegion **ram_memory)
 {
     int i, rc;
     unsigned long ioreq_pfn;
@@ -1144,7 +1148,7 @@ int xen_hvm_init(MemoryRegion **ram_memory)
 
     /* Init RAM management */
     xen_map_cache_init(xen_phys_offset_to_gaddr, state);
-    xen_ram_init(ram_size, ram_memory);
+    xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
diff --git a/xen-stub.c b/xen-stub.c
index 59927cb..0302dff 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -64,7 +64,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
 {
 }
 
-int xen_hvm_init(MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
+                 MemoryRegion **ram_memory)
 {
     return 0;
 }
-- 
1.8.4

[PATCH v3 1/4] xen-all: Fix xen_hvm_init() to adjust pc memory layout.

[Don Slutz]

This is just below_4g_mem_size and above_4g_mem_size which is used later in QEMU.

Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/i386/pc_piix.c    | 31 ++++++++++++++++---------------
 hw/i386/pc_q35.c     | 29 +++++++++++++++--------------
 include/hw/xen/xen.h |  3 ++-
 xen-all.c            | 24 ++++++++++++++----------
 xen-stub.c           |  3 ++-
 5 files changed, 49 insertions(+), 41 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7930a26..bd52f6e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -96,21 +96,6 @@ static void pc_init1(QEMUMachineInitArgs *args,
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
 
-    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
-        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
-        exit(1);
-    }
-
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(args->cpu_model, icc_bridge);
-
-    if (kvm_enabled() && kvmclock_enabled) {
-        kvmclock_create();
-    }
-
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
      * If it doesn't, we need to split it in chunks below and above 4G.
      * In any case, try to make sure that guest addresses aligned at
@@ -127,6 +112,22 @@ static void pc_init1(QEMUMachineInitArgs *args,
         below_4g_mem_size = args->ram_size;
     }
 
+    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
+                                      &ram_memory) != 0) {
+        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
+        exit(1);
+    }
+
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
+
+    pc_cpus_init(args->cpu_model, icc_bridge);
+
+    if (kvm_enabled() && kvmclock_enabled) {
+        kvmclock_create();
+    }
+
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
         memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c844dc2..6e34fe1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -83,20 +83,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
 
-    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
-        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
-        exit(1);
-    }
-
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(args->cpu_model, icc_bridge);
-    pc_acpi_init("q35-acpi-dsdt.aml");
-
-    kvmclock_create();
-
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
      * also known as MMCFG).
@@ -115,6 +101,21 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
         below_4g_mem_size = args->ram_size;
     }
 
+    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
+                                      &ram_memory) != 0) {
+        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
+        exit(1);
+    }
+
+    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
+    object_property_add_child(qdev_get_machine(), "icc-bridge",
+                              OBJECT(icc_bridge), NULL);
+
+    pc_cpus_init(args->cpu_model, icc_bridge);
+    pc_acpi_init("q35-acpi-dsdt.aml");
+
+    kvmclock_create();
+
     /* pci enabled */
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 9d549fc..0f3942e 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -37,10 +37,11 @@ void xen_cmos_set_s3_resume(void *opaque, int irq, int level);
 qemu_irq *xen_interrupt_controller_init(void);
 
 int xen_init(QEMUMachine *machine);
-int xen_hvm_init(MemoryRegion **ram_memory);
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
+int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
+                 MemoryRegion **ram_memory);
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
                    struct MemoryRegion *mr);
 void xen_modified_memory(ram_addr_t start, ram_addr_t length);
diff --git a/xen-all.c b/xen-all.c
index ba34739..c64300c 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -155,10 +155,11 @@ qemu_irq *xen_interrupt_controller_init(void)
 
 /* Memory Ops */
 
-static void xen_ram_init(ram_addr_t ram_size, MemoryRegion **ram_memory_p)
+static void xen_ram_init(ram_addr_t *below_4g_mem_size,
+                         ram_addr_t *above_4g_mem_size,
+                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
 {
     MemoryRegion *sysmem = get_system_memory();
-    ram_addr_t below_4g_mem_size, above_4g_mem_size = 0;
     ram_addr_t block_len;
 
     block_len = ram_size;
@@ -173,10 +174,11 @@ static void xen_ram_init(ram_addr_t ram_size, MemoryRegion **ram_memory_p)
     vmstate_register_ram_global(&ram_memory);
 
     if (ram_size >= HVM_BELOW_4G_RAM_END) {
-        above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
-        below_4g_mem_size = HVM_BELOW_4G_RAM_END;
+        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
+        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
     } else {
-        below_4g_mem_size = ram_size;
+        *above_4g_mem_size = 0;
+        *below_4g_mem_size = ram_size;
     }
 
     memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
@@ -189,12 +191,13 @@ static void xen_ram_init(ram_addr_t ram_size, MemoryRegion **ram_memory_p)
      * the Options ROM, so it is registered here as RAM.
      */
     memory_region_init_alias(&ram_lo, NULL, "xen.ram.lo",
-                             &ram_memory, 0xc0000, below_4g_mem_size - 0xc0000);
+                             &ram_memory, 0xc0000,
+                             *below_4g_mem_size - 0xc0000);
     memory_region_add_subregion(sysmem, 0xc0000, &ram_lo);
-    if (above_4g_mem_size > 0) {
+    if (*above_4g_mem_size > 0) {
         memory_region_init_alias(&ram_hi, NULL, "xen.ram.hi",
                                  &ram_memory, 0x100000000ULL,
-                                 above_4g_mem_size);
+                                 *above_4g_mem_size);
         memory_region_add_subregion(sysmem, 0x100000000ULL, &ram_hi);
     }
 }
@@ -1066,7 +1069,8 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
-int xen_hvm_init(MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
+                 MemoryRegion **ram_memory)
 {
     int i, rc;
     unsigned long ioreq_pfn;
@@ -1144,7 +1148,7 @@ int xen_hvm_init(MemoryRegion **ram_memory)
 
     /* Init RAM management */
     xen_map_cache_init(xen_phys_offset_to_gaddr, state);
-    xen_ram_init(ram_size, ram_memory);
+    xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
diff --git a/xen-stub.c b/xen-stub.c
index 59927cb..0302dff 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -64,7 +64,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
 {
 }
 
-int xen_hvm_init(MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
+                 MemoryRegion **ram_memory)
 {
     return 0;
 }
-- 
1.8.4

Re: [PATCH v3 1/4] xen-all: Fix xen_hvm_init() to adjust pc memory layout.

[Andreas Färber]

Am 25.03.2014 00:55, schrieb Don Slutz:
> This is just below_4g_mem_size and above_4g_mem_size which is used later in QEMU.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Please remember to place your Signed-off-by last. In theory you would
place another Signed-off-by last, but in practice we usually drop the
first one if there are no other Sobs. Think of someone taking your
patch, making changes and applying it to some downstream; then Stefano
did not ack the modified patch signed off by someone else, but rather
you are asserting that Stefano acked the patch in the form you are
sending it.

> ---
>  hw/i386/pc_piix.c    | 31 ++++++++++++++++---------------
>  hw/i386/pc_q35.c     | 29 +++++++++++++++--------------
>  include/hw/xen/xen.h |  3 ++-
>  xen-all.c            | 24 ++++++++++++++----------
>  xen-stub.c           |  3 ++-
>  5 files changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7930a26..bd52f6e 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -96,21 +96,6 @@ static void pc_init1(QEMUMachineInitArgs *args,
>      FWCfgState *fw_cfg = NULL;
>      PcGuestInfo *guest_info;
>  
> -    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
> -        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> -        exit(1);
> -    }
> -
> -    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> -    object_property_add_child(qdev_get_machine(), "icc-bridge",
> -                              OBJECT(icc_bridge), NULL);
> -
> -    pc_cpus_init(args->cpu_model, icc_bridge);
> -
> -    if (kvm_enabled() && kvmclock_enabled) {
> -        kvmclock_create();
> -    }
> -
>      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
>       * If it doesn't, we need to split it in chunks below and above 4G.
>       * In any case, try to make sure that guest addresses aligned at
> @@ -127,6 +112,22 @@ static void pc_init1(QEMUMachineInitArgs *args,
>          below_4g_mem_size = args->ram_size;
>      }
>  
> +    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> +                                      &ram_memory) != 0) {
> +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> +        exit(1);
> +    }
> +
> +    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), "icc-bridge",
> +                              OBJECT(icc_bridge), NULL);
> +
> +    pc_cpus_init(args->cpu_model, icc_bridge);
> +
> +    if (kvm_enabled() && kvmclock_enabled) {
> +        kvmclock_create();
> +    }
> +
>      if (pci_enabled) {
>          pci_memory = g_new(MemoryRegion, 1);
>          memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);

Movement looks okay to me, CC'ing Igor. Did you test KVM or only Xen?

It would be nice to follow up replacing fprintf() with error_report()
though in a separate patch.

> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index c844dc2..6e34fe1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -83,20 +83,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      DeviceState *icc_bridge;
>      PcGuestInfo *guest_info;
>  
> -    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
> -        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> -        exit(1);
> -    }
> -
> -    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> -    object_property_add_child(qdev_get_machine(), "icc-bridge",
> -                              OBJECT(icc_bridge), NULL);
> -
> -    pc_cpus_init(args->cpu_model, icc_bridge);
> -    pc_acpi_init("q35-acpi-dsdt.aml");
> -
> -    kvmclock_create();
> -
>      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>       * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>       * also known as MMCFG).
> @@ -115,6 +101,21 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>          below_4g_mem_size = args->ram_size;
>      }
>  
> +    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> +                                      &ram_memory) != 0) {
> +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> +        exit(1);
> +    }
> +
> +    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), "icc-bridge",
> +                              OBJECT(icc_bridge), NULL);
> +
> +    pc_cpus_init(args->cpu_model, icc_bridge);
> +    pc_acpi_init("q35-acpi-dsdt.aml");
> +
> +    kvmclock_create();
> +
>      /* pci enabled */
>      if (pci_enabled) {
>          pci_memory = g_new(MemoryRegion, 1);
[snip]

Dito here.

Xen parts look sane to me.

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v3 1/4] xen-all: Fix xen_hvm_init() to adjust pc memory layout.

[Don Slutz]

On 04/18/14 10:23, Andreas Färber wrote:
> Am 25.03.2014 00:55, schrieb Don Slutz:
>> This is just below_4g_mem_size and above_4g_mem_size which is used later in QEMU.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Please remember to place your Signed-off-by last. In theory you would
> place another Signed-off-by last, but in practice we usually drop the
> first one if there are no other Sobs. Think of someone taking your
> patch, making changes and applying it to some downstream; then Stefano
> did not ack the modified patch signed off by someone else, but rather
> you are asserting that Stefano acked the patch in the form you are
> sending it.

I will try to remember this.  Not sure Xen follows the same
rules.

>> ---
>>   hw/i386/pc_piix.c    | 31 ++++++++++++++++---------------
>>   hw/i386/pc_q35.c     | 29 +++++++++++++++--------------
>>   include/hw/xen/xen.h |  3 ++-
>>   xen-all.c            | 24 ++++++++++++++----------
>>   xen-stub.c           |  3 ++-
>>   5 files changed, 49 insertions(+), 41 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 7930a26..bd52f6e 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -96,21 +96,6 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>       FWCfgState *fw_cfg = NULL;
>>       PcGuestInfo *guest_info;
>>   
>> -    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
>> -        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>> -        exit(1);
>> -    }
>> -
>> -    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
>> -    object_property_add_child(qdev_get_machine(), "icc-bridge",
>> -                              OBJECT(icc_bridge), NULL);
>> -
>> -    pc_cpus_init(args->cpu_model, icc_bridge);
>> -
>> -    if (kvm_enabled() && kvmclock_enabled) {
>> -        kvmclock_create();
>> -    }
>> -
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
>>        * If it doesn't, we need to split it in chunks below and above 4G.
>>        * In any case, try to make sure that guest addresses aligned at
>> @@ -127,6 +112,22 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>           below_4g_mem_size = args->ram_size;
>>       }
>>   
>> +    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
>> +                                      &ram_memory) != 0) {
>> +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>> +        exit(1);
>> +    }
>> +
>> +    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
>> +    object_property_add_child(qdev_get_machine(), "icc-bridge",
>> +                              OBJECT(icc_bridge), NULL);
>> +
>> +    pc_cpus_init(args->cpu_model, icc_bridge);
>> +
>> +    if (kvm_enabled() && kvmclock_enabled) {
>> +        kvmclock_create();
>> +    }
>> +
>>       if (pci_enabled) {
>>           pci_memory = g_new(MemoryRegion, 1);
>>           memory_region_init(pci_memory, NULL, "pci", UINT64_MAX);
> Movement looks okay to me, CC'ing Igor. Did you test KVM or only Xen?

I have tested on one version of KVM (Fedora 17).

> It would be nice to follow up replacing fprintf() with error_report()
> though in a separate patch.

Will do.
    -Don Slutz

>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index c844dc2..6e34fe1 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -83,20 +83,6 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>       DeviceState *icc_bridge;
>>       PcGuestInfo *guest_info;
>>   
>> -    if (xen_enabled() && xen_hvm_init(&ram_memory) != 0) {
>> -        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>> -        exit(1);
>> -    }
>> -
>> -    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
>> -    object_property_add_child(qdev_get_machine(), "icc-bridge",
>> -                              OBJECT(icc_bridge), NULL);
>> -
>> -    pc_cpus_init(args->cpu_model, icc_bridge);
>> -    pc_acpi_init("q35-acpi-dsdt.aml");
>> -
>> -    kvmclock_create();
>> -
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>>        * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>>        * also known as MMCFG).
>> @@ -115,6 +101,21 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>           below_4g_mem_size = args->ram_size;
>>       }
>>   
>> +    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
>> +                                      &ram_memory) != 0) {
>> +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>> +        exit(1);
>> +    }
>> +
>> +    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
>> +    object_property_add_child(qdev_get_machine(), "icc-bridge",
>> +                              OBJECT(icc_bridge), NULL);
>> +
>> +    pc_cpus_init(args->cpu_model, icc_bridge);
>> +    pc_acpi_init("q35-acpi-dsdt.aml");
>> +
>> +    kvmclock_create();
>> +
>>       /* pci enabled */
>>       if (pci_enabled) {
>>           pci_memory = g_new(MemoryRegion, 1);
> [snip]
>
> Dito here.
>
> Xen parts look sane to me.
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> Regards,
> Andreas
>

[Qemu-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

This can help a user understand why -global was ignored.

For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
ignored when "-global cirrus-vga.vgamem_mb=16" is not.

This is currently clear when the wrong property is provided:

out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
char device redirected to /dev/pts/20 (label compat_monitor0)
qemu-system-x86_64: Property '.vram_size_mb' not found
Aborted (core dumped)

vs

out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16 -monitor pty -vga cirrus
char device redirected to /dev/pts/20 (label compat_monitor0)
VNC server running on `::1:5900'
^Cqemu: terminating on signal 2

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/core/qdev-properties-system.c |  1 +
 hw/core/qdev-properties.c        | 15 +++++++++++++++
 include/hw/qdev-core.h           |  1 +
 include/hw/qdev-properties.h     |  1 +
 vl.c                             |  2 ++
 5 files changed, 20 insertions(+)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index de83561..9c742ca 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -444,6 +444,7 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
     g->driver   = qemu_opt_get(opts, "driver");
     g->property = qemu_opt_get(opts, "property");
     g->value    = qemu_opt_get(opts, "value");
+    g->not_used = true;
     qdev_prop_register_global(g);
     return 0;
 }
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c67acf5..437c008 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -951,6 +951,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
+void qdev_prop_check_global(void)
+{
+    GlobalProperty *prop;
+
+    QTAILQ_FOREACH(prop, &global_props, next) {
+        if (!prop->not_used) {
+            continue;
+        }
+        fprintf(stderr, "Warning: \"-global %s.%s=%s\" not used\n",
+                prop->driver, prop->property, prop->value);
+
+    }
+}
+
 void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp)
 {
@@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
         if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
+        prop->not_used = false;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
             error_propagate(errp, err);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dbe473c..131fb49 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -235,6 +235,7 @@ typedef struct GlobalProperty {
     const char *driver;
     const char *property;
     const char *value;
+    bool not_used;
     QTAILQ_ENTRY(GlobalProperty) next;
 } GlobalProperty;
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index c46e908..fbca313 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -180,6 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
+void qdev_prop_check_global(void);
 void qdev_prop_set_globals(DeviceState *dev, Error **errp);
 void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp);
diff --git a/vl.c b/vl.c
index acd97a8..61fac1b 100644
--- a/vl.c
+++ b/vl.c
@@ -4490,6 +4490,8 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    qdev_prop_check_global();
+
     if (incoming) {
         Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
-- 
1.8.4

[PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

This can help a user understand why -global was ignored.

For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
ignored when "-global cirrus-vga.vgamem_mb=16" is not.

This is currently clear when the wrong property is provided:

out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
char device redirected to /dev/pts/20 (label compat_monitor0)
qemu-system-x86_64: Property '.vram_size_mb' not found
Aborted (core dumped)

vs

out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16 -monitor pty -vga cirrus
char device redirected to /dev/pts/20 (label compat_monitor0)
VNC server running on `::1:5900'
^Cqemu: terminating on signal 2

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/core/qdev-properties-system.c |  1 +
 hw/core/qdev-properties.c        | 15 +++++++++++++++
 include/hw/qdev-core.h           |  1 +
 include/hw/qdev-properties.h     |  1 +
 vl.c                             |  2 ++
 5 files changed, 20 insertions(+)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index de83561..9c742ca 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -444,6 +444,7 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
     g->driver   = qemu_opt_get(opts, "driver");
     g->property = qemu_opt_get(opts, "property");
     g->value    = qemu_opt_get(opts, "value");
+    g->not_used = true;
     qdev_prop_register_global(g);
     return 0;
 }
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index c67acf5..437c008 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -951,6 +951,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
+void qdev_prop_check_global(void)
+{
+    GlobalProperty *prop;
+
+    QTAILQ_FOREACH(prop, &global_props, next) {
+        if (!prop->not_used) {
+            continue;
+        }
+        fprintf(stderr, "Warning: \"-global %s.%s=%s\" not used\n",
+                prop->driver, prop->property, prop->value);
+
+    }
+}
+
 void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp)
 {
@@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
         if (strcmp(typename, prop->driver) != 0) {
             continue;
         }
+        prop->not_used = false;
         object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
         if (err != NULL) {
             error_propagate(errp, err);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dbe473c..131fb49 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -235,6 +235,7 @@ typedef struct GlobalProperty {
     const char *driver;
     const char *property;
     const char *value;
+    bool not_used;
     QTAILQ_ENTRY(GlobalProperty) next;
 } GlobalProperty;
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index c46e908..fbca313 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -180,6 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
 void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
+void qdev_prop_check_global(void);
 void qdev_prop_set_globals(DeviceState *dev, Error **errp);
 void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
                                     Error **errp);
diff --git a/vl.c b/vl.c
index acd97a8..61fac1b 100644
--- a/vl.c
+++ b/vl.c
@@ -4490,6 +4490,8 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    qdev_prop_check_global();
+
     if (incoming) {
         Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
-- 
1.8.4

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Andreas Färber]

Hi Don,

Am 25.03.2014 00:55, schrieb Don Slutz:
> This can help a user understand why -global was ignored.
> 
> For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
> ignored when "-global cirrus-vga.vgamem_mb=16" is not.
> 
> This is currently clear when the wrong property is provided:
> 
> out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
> char device redirected to /dev/pts/20 (label compat_monitor0)
> qemu-system-x86_64: Property '.vram_size_mb' not found
> Aborted (core dumped)
> 
> vs
> 
> out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16 -monitor pty -vga cirrus
> char device redirected to /dev/pts/20 (label compat_monitor0)
> VNC server running on `::1:5900'
> ^Cqemu: terminating on signal 2
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Improving this is greatly appreciated, thanks.

Now, I can see two ways things can go wrong: a) Mistyping or later
refactoring devices, or b) user typos or thinkos.
And four ways to set globals: -global, config file (I hope?), legacy
command line options (vl.c) and machine .compat_props.

If a property does not exist on the instance of an existing type,
object_property_parse() will raise an Error and we will abort in
device_post_init().

What we are silently missing is if a type is misspelled; for that we can
probably add an Error **errp to the two qdev_prop_register_global*()
functions - assuming QOM types are already registered at that point.
qom-test would help us catch any such mistake by instantiating each machine.

I note that your proposed check is not failing, but still, with hot-add
of e.g. PCI devices we might well get a global property default for a
type that is not instantiated immediately but possibly used later on.

> ---
>  hw/core/qdev-properties-system.c |  1 +
>  hw/core/qdev-properties.c        | 15 +++++++++++++++
>  include/hw/qdev-core.h           |  1 +
>  include/hw/qdev-properties.h     |  1 +
>  vl.c                             |  2 ++
>  5 files changed, 20 insertions(+)

FWIW I'd prefer "qdev:" for consistency (and yes, it's ambiguous), since
there are no "GlobalProperty" files or directory.

> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index de83561..9c742ca 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -444,6 +444,7 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
>      g->driver   = qemu_opt_get(opts, "driver");
>      g->property = qemu_opt_get(opts, "property");
>      g->value    = qemu_opt_get(opts, "value");
> +    g->not_used = true;
>      qdev_prop_register_global(g);
>      return 0;
>  }

IIUC your patch relies on not_used being false in the non-QemuOpts case
to avoid noise when using -nodefaults or pc*-x.y. Still, the C99 struct
initializations elsewhere get that field as well, hmm. An alternative
would be a separate linked list for user-supplied globals.

> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index c67acf5..437c008 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -951,6 +951,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
>      }
>  }
>  
> +void qdev_prop_check_global(void)
> +{
> +    GlobalProperty *prop;
> +
> +    QTAILQ_FOREACH(prop, &global_props, next) {
> +        if (!prop->not_used) {
> +            continue;
> +        }
> +        fprintf(stderr, "Warning: \"-global %s.%s=%s\" not used\n",
> +                prop->driver, prop->property, prop->value);
> +
> +    }
> +}
> +
>  void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>                                      Error **errp)
>  {
> @@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>          if (strcmp(typename, prop->driver) != 0) {
>              continue;
>          }
> +        prop->not_used = false;
>          object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
>          if (err != NULL) {
>              error_propagate(errp, err);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index dbe473c..131fb49 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -235,6 +235,7 @@ typedef struct GlobalProperty {
>      const char *driver;
>      const char *property;
>      const char *value;
> +    bool not_used;
>      QTAILQ_ENTRY(GlobalProperty) next;
>  } GlobalProperty;
>  
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index c46e908..fbca313 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -180,6 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>  
>  void qdev_prop_register_global(GlobalProperty *prop);
>  void qdev_prop_register_global_list(GlobalProperty *props);
> +void qdev_prop_check_global(void);
>  void qdev_prop_set_globals(DeviceState *dev, Error **errp);
>  void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>                                      Error **errp);
> diff --git a/vl.c b/vl.c
> index acd97a8..61fac1b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4490,6 +4490,8 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    qdev_prop_check_global();

I have some doubts about this placement. A machine init done notifier
might avoid touching vl.c by leaving it in qdev-properties.c. It happens
to be after that point as is, but later refactorings wrt QOM realize or
unrelated issues might change that.

> +
>      if (incoming) {
>          Error *local_err = NULL;
>          qemu_start_incoming_migration(incoming, &local_err);

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Fabio Fantoni]

[-- Attachment #1: Type: text/plain, Size: 6927 bytes --]

2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de>:

> Hi Don,
>
> Am 25.03.2014 00:55, schrieb Don Slutz:
> > This can help a user understand why -global was ignored.
> >
> > For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
> > ignored when "-global cirrus-vga.vgamem_mb=16" is not.
> >
> > This is currently clear when the wrong property is provided:
> >
> > out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16
> -monitor pty -vga cirrus
> > char device redirected to /dev/pts/20 (label compat_monitor0)
> > qemu-system-x86_64: Property '.vram_size_mb' not found
> > Aborted (core dumped)
> >
> > vs
> >
> > out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16
> -monitor pty -vga cirrus
> > char device redirected to /dev/pts/20 (label compat_monitor0)
> > VNC server running on `::1:5900'
> > ^Cqemu: terminating on signal 2
>

I added the cirrus video memory setting in libxl time ago (using -global
vga.vram_size_mb), testing it with qemu 1.3 and also on qemu 1.6 when I
changed from -vga to -device if I remember good.
Has been changed in recent versions or something was not right even though it
seemed right to me?

Thanks for any reply and sorry for my bad english.


> >
> > Signed-off-by: Don Slutz <dslutz@verizon.com>
>
> Improving this is greatly appreciated, thanks.
>
> Now, I can see two ways things can go wrong: a) Mistyping or later
> refactoring devices, or b) user typos or thinkos.
> And four ways to set globals: -global, config file (I hope?), legacy
> command line options (vl.c) and machine .compat_props.
>
> If a property does not exist on the instance of an existing type,
> object_property_parse() will raise an Error and we will abort in
> device_post_init().
>
> What we are silently missing is if a type is misspelled; for that we can
> probably add an Error **errp to the two qdev_prop_register_global*()
> functions - assuming QOM types are already registered at that point.
> qom-test would help us catch any such mistake by instantiating each
> machine.
>
> I note that your proposed check is not failing, but still, with hot-add
> of e.g. PCI devices we might well get a global property default for a
> type that is not instantiated immediately but possibly used later on.
>
> > ---
> >  hw/core/qdev-properties-system.c |  1 +
> >  hw/core/qdev-properties.c        | 15 +++++++++++++++
> >  include/hw/qdev-core.h           |  1 +
> >  include/hw/qdev-properties.h     |  1 +
> >  vl.c                             |  2 ++
> >  5 files changed, 20 insertions(+)
>
> FWIW I'd prefer "qdev:" for consistency (and yes, it's ambiguous), since
> there are no "GlobalProperty" files or directory.
>
> > diff --git a/hw/core/qdev-properties-system.c
> b/hw/core/qdev-properties-system.c
> > index de83561..9c742ca 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -444,6 +444,7 @@ static int qdev_add_one_global(QemuOpts *opts, void
> *opaque)
> >      g->driver   = qemu_opt_get(opts, "driver");
> >      g->property = qemu_opt_get(opts, "property");
> >      g->value    = qemu_opt_get(opts, "value");
> > +    g->not_used = true;
> >      qdev_prop_register_global(g);
> >      return 0;
> >  }
>
> IIUC your patch relies on not_used being false in the non-QemuOpts case
> to avoid noise when using -nodefaults or pc*-x.y. Still, the C99 struct
> initializations elsewhere get that field as well, hmm. An alternative
> would be a separate linked list for user-supplied globals.
>
> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > index c67acf5..437c008 100644
> > --- a/hw/core/qdev-properties.c
> > +++ b/hw/core/qdev-properties.c
> > @@ -951,6 +951,20 @@ void qdev_prop_register_global_list(GlobalProperty
> *props)
> >      }
> >  }
> >
> > +void qdev_prop_check_global(void)
> > +{
> > +    GlobalProperty *prop;
> > +
> > +    QTAILQ_FOREACH(prop, &global_props, next) {
> > +        if (!prop->not_used) {
> > +            continue;
> > +        }
> > +        fprintf(stderr, "Warning: \"-global %s.%s=%s\" not used\n",
> > +                prop->driver, prop->property, prop->value);
> > +
> > +    }
> > +}
> > +
> >  void qdev_prop_set_globals_for_type(DeviceState *dev, const char
> *typename,
> >                                      Error **errp)
> >  {
> > @@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState
> *dev, const char *typename,
> >          if (strcmp(typename, prop->driver) != 0) {
> >              continue;
> >          }
> > +        prop->not_used = false;
> >          object_property_parse(OBJECT(dev), prop->value, prop->property,
> &err);
> >          if (err != NULL) {
> >              error_propagate(errp, err);
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index dbe473c..131fb49 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -235,6 +235,7 @@ typedef struct GlobalProperty {
> >      const char *driver;
> >      const char *property;
> >      const char *value;
> > +    bool not_used;
> >      QTAILQ_ENTRY(GlobalProperty) next;
> >  } GlobalProperty;
> >
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index c46e908..fbca313 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -180,6 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char
> *name, void *value);
> >
> >  void qdev_prop_register_global(GlobalProperty *prop);
> >  void qdev_prop_register_global_list(GlobalProperty *props);
> > +void qdev_prop_check_global(void);
> >  void qdev_prop_set_globals(DeviceState *dev, Error **errp);
> >  void qdev_prop_set_globals_for_type(DeviceState *dev, const char
> *typename,
> >                                      Error **errp);
> > diff --git a/vl.c b/vl.c
> > index acd97a8..61fac1b 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4490,6 +4490,8 @@ int main(int argc, char **argv, char **envp)
> >          }
> >      }
> >
> > +    qdev_prop_check_global();
>
> I have some doubts about this placement. A machine init done notifier
> might avoid touching vl.c by leaving it in qdev-properties.c. It happens
> to be after that point as is, but later refactorings wrt QOM realize or
> unrelated issues might change that.
>
> > +
> >      if (incoming) {
> >          Error *local_err = NULL;
> >          qemu_start_incoming_migration(incoming, &local_err);
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

[-- Attachment #2: Type: text/html, Size: 8697 bytes --]

Re: [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Andreas Färber]

Am 18.04.2014 17:36, schrieb Fabio Fantoni:
> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de
> <mailto:afaerber@suse.de>>:
> 
>     Hi Don,
> 
>     Am 25.03.2014 00 <tel:25.03.2014%2000>:55, schrieb Don Slutz:
>     > This can help a user understand why -global was ignored.
>     >
>     > For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
>     > ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>     >
>     > This is currently clear when the wrong property is provided:
>     >
>     > out/x86_64-softmmu/qemu-system-x86_64 -global
>     cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
>     > char device redirected to /dev/pts/20 (label compat_monitor0)
>     > qemu-system-x86_64: Property '.vram_size_mb' not found
>     > Aborted (core dumped)
>     >
>     > vs
>     >
>     > out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16
>     -monitor pty -vga cirrus
>     > char device redirected to /dev/pts/20 (label compat_monitor0)
>     > VNC server running on `::1:5900'
>     > ^Cqemu: terminating on signal 2
> 
> 
> I added the cirrus video memory setting in libxl time ago (using -global
> vga.vram_size_mb), testing it with qemu 1.3 and also on qemu 1.6 when I
> changed from -vga to -device if I remember good.
> Has been changed in recent versions or something was not right even
> though it seemed right to me?

There are multiple graphics cards to choose from. When using -vga std or
-device vga, then -global vga.foo=bar gets used; if -vga cirrus or
-device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and
any -global vga.foo=bar gets ignored - unless you manage to add it as
secondary (PCI) graphics card.

Regards,
Andreas

P.S. Please remember to use text format mails.

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Fabio Fantoni]

Il 18/04/2014 17:59, Andreas Färber ha scritto:
> Am 18.04.2014 17:36, schrieb Fabio Fantoni:
>> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de
>> <mailto:afaerber@suse.de>>:
>>
>>      Hi Don,
>>
>>      Am 25.03.2014 00 <tel:25.03.2014%2000>:55, schrieb Don Slutz:
>>      > This can help a user understand why -global was ignored.
>>      >
>>      > For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
>>      > ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>>      >
>>      > This is currently clear when the wrong property is provided:
>>      >
>>      > out/x86_64-softmmu/qemu-system-x86_64 -global
>>      cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
>>      > char device redirected to /dev/pts/20 (label compat_monitor0)
>>      > qemu-system-x86_64: Property '.vram_size_mb' not found
>>      > Aborted (core dumped)
>>      >
>>      > vs
>>      >
>>      > out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16
>>      -monitor pty -vga cirrus
>>      > char device redirected to /dev/pts/20 (label compat_monitor0)
>>      > VNC server running on `::1:5900'
>>      > ^Cqemu: terminating on signal 2
>>
>>
>> I added the cirrus video memory setting in libxl time ago (using -global
>> vga.vram_size_mb), testing it with qemu 1.3 and also on qemu 1.6 when I
>> changed from -vga to -device if I remember good.
>> Has been changed in recent versions or something was not right even
>> though it seemed right to me?
> There are multiple graphics cards to choose from. When using -vga std or
> -device vga, then -global vga.foo=bar gets used; if -vga cirrus or
> -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and
> any -global vga.foo=bar gets ignored - unless you manage to add it as
> secondary (PCI) graphics card.

Thanks for your reply.
Can you tell me if also -device cirrus-vga,vram_size_mb=N is correct and 
working?

Thanks for any reply.
>
> Regards,
> Andreas
>
> P.S. Please remember to use text format mails.
>

Re: [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Fabio Fantoni]

Il 18/04/2014 18:54, Fabio Fantoni ha scritto:
>
> Il 18/04/2014 17:59, Andreas Färber ha scritto:
>> Am 18.04.2014 17:36, schrieb Fabio Fantoni:
>>> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de
>>> <mailto:afaerber@suse.de>>:
>>>
>>>      Hi Don,
>>>
>>>      Am 25.03.2014 00 <tel:25.03.2014%2000>:55, schrieb Don Slutz:
>>>      > This can help a user understand why -global was ignored.
>>>      >
>>>      > For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" 
>>> is just
>>>      > ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>>>      >
>>>      > This is currently clear when the wrong property is provided:
>>>      >
>>>      > out/x86_64-softmmu/qemu-system-x86_64 -global
>>>      cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
>>>      > char device redirected to /dev/pts/20 (label compat_monitor0)
>>>      > qemu-system-x86_64: Property '.vram_size_mb' not found
>>>      > Aborted (core dumped)
>>>      >
>>>      > vs
>>>      >
>>>      > out/x86_64-softmmu/qemu-system-x86_64 -global 
>>> vga.vram_size_mb=16
>>>      -monitor pty -vga cirrus
>>>      > char device redirected to /dev/pts/20 (label compat_monitor0)
>>>      > VNC server running on `::1:5900'
>>>      > ^Cqemu: terminating on signal 2
>>>
>>>
>>> I added the cirrus video memory setting in libxl time ago (using 
>>> -global
>>> vga.vram_size_mb), testing it with qemu 1.3 and also on qemu 1.6 when I
>>> changed from -vga to -device if I remember good.
>>> Has been changed in recent versions or something was not right even
>>> though it seemed right to me?
>> There are multiple graphics cards to choose from. When using -vga std or
>> -device vga, then -global vga.foo=bar gets used; if -vga cirrus or
>> -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and
>> any -global vga.foo=bar gets ignored - unless you manage to add it as
>> secondary (PCI) graphics card.
>
> Thanks for your reply.
> Can you tell me if also -device cirrus-vga,vram_size_mb=N is correct 
> and working?

I probably found the correct values settable:

in

in hw/display/cirrus_vga.c:
> 2988 static Property pci_vga_cirrus_properties[] = {
> 2989     DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
> 2990                        cirrus_vga.vga.vram_size_mb, 8),
> 2991     DEFINE_PROP_END_OF_LIST(),
> 2992 };
Than I "-device cirrus-vga,vgamem_mb=N", not show errors and should be 
correct, right?

in hw/display/vga-pci.c:
>  182 static Property vga_pci_properties[] = {
> 183     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 
> 16),
> 184     DEFINE_PROP_BIT("mmio", PCIVGAState, flags, 
> PCI_VGA_FLAG_ENABLE_MMIO, true),
> 185     DEFINE_PROP_END_OF_LIST(),
> 186 };
I tried time ago the videoram setting of stdvga on xen but seems was not 
worked (no error but performance remain bad on medium/large resolution, 
trying kvm with same parameters is better), I not know if is vgabios 
problem or low level xen changes are needed.
The mmio option seems new to me, what it is in practice, may need to 
disable it in xen?

Thanks for any reply and sorry for my bad english.


>
> Thanks for any reply.
>>
>> Regards,
>> Andreas
>>
>> P.S. Please remember to use text format mails.
>>
>

Re: [Xen-devel] [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

On 04/19/14 06:56, Fabio Fantoni wrote:
>
> Il 18/04/2014 18:54, Fabio Fantoni ha scritto:
>>
>> Il 18/04/2014 17:59, Andreas Färber ha scritto:
>>> Am 18.04.2014 17:36, schrieb Fabio Fantoni:
>>>> 2014-04-18 17:21 GMT+02:00 Andreas Färber <afaerber@suse.de
>>>> <mailto:afaerber@suse.de>>:
>>>>
>>>>      Hi Don,
>>>>
>>>>      Am 25.03.2014 00 <tel:25.03.2014%2000>:55, schrieb Don Slutz:
>>>>      > This can help a user understand why -global was ignored.
>>>>      >
>>>>      > For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
>>>>      > ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>>>>      >
>>>>      > This is currently clear when the wrong property is provided:
>>>>      >
>>>>      > out/x86_64-softmmu/qemu-system-x86_64 -global
>>>>      cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
>>>>      > char device redirected to /dev/pts/20 (label compat_monitor0)
>>>>      > qemu-system-x86_64: Property '.vram_size_mb' not found
>>>>      > Aborted (core dumped)
>>>>      >
>>>>      > vs
>>>>      >
>>>>      > out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16
>>>>      -monitor pty -vga cirrus
>>>>      > char device redirected to /dev/pts/20 (label compat_monitor0)
>>>>      > VNC server running on `::1:5900'
>>>>      > ^Cqemu: terminating on signal 2
>>>>
>>>>
>>>> I added the cirrus video memory setting in libxl time ago (using -global
>>>> vga.vram_size_mb), testing it with qemu 1.3 and also on qemu 1.6 when I
>>>> changed from -vga to -device if I remember good.
>>>> Has been changed in recent versions or something was not right even
>>>> though it seemed right to me?
>>> There are multiple graphics cards to choose from. When using -vga std or
>>> -device vga, then -global vga.foo=bar gets used; if -vga cirrus or
>>> -device cirrus-vga then it needs to be -global cirrus-vga.foo=bar and
>>> any -global vga.foo=bar gets ignored - unless you manage to add it as
>>> secondary (PCI) graphics card.
>>
>> Thanks for your reply.
>> Can you tell me if also -device cirrus-vga,vram_size_mb=N is correct and working?
>
> I probably found the correct values settable:
>
> in
>
> in hw/display/cirrus_vga.c:
>> 2988 static Property pci_vga_cirrus_properties[] = {
>> 2989     DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
>> 2990                        cirrus_vga.vga.vram_size_mb, 8),
>> 2991     DEFINE_PROP_END_OF_LIST(), 36d20cb
>> 2992 };
> Than I "-device cirrus-vga,vgamem_mb=N", not show errors and should be correct, right?
>
> in hw/display/vga-pci.c:
>>  182 static Property vga_pci_properties[] = {
>> 183     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
>> 184     DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
>> 185     DEFINE_PROP_END_OF_LIST(),
>> 186 };
> I tried time ago the videoram setting of stdvga on xen but seems was not worked (no error but performance remain bad on medium/large resolution, trying kvm with same parameters is better), I not know if is vgabios problem or low level xen changes are needed.
> The mmio option seems new to me, what it is in practice, may need to disable it in xen?
>
> Thanks for any reply and sorry for my bad english.
>

Using qemu 1.7.0, I verified that

    -vga std -global "VGA.vgamem_mb=N

and

    -vga cirrus -global cirrus-vga.vgamem_mb=N


Correctly set the size of videoram.  Have not checked that 2.0 is still
the same.  I do see:

   -device cirrus-vga -global vga.vram_size_mb=N

In Xen master, which is wrong.

    -Don Slutz

>
>>
>> Thanks for any reply.
>>>
>>> Regards,
>>> Andreas
>>>
>>> P.S. Please remember to use text format mails.
>>>
>>
>

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Paolo Bonzini]

Il 18/04/2014 11:21, Andreas Färber ha scritto:
> Improving this is greatly appreciated, thanks.
>
> Now, I can see two ways things can go wrong: a) Mistyping or later
> refactoring devices, or b) user typos or thinkos.
> And four ways to set globals: -global, config file (I hope?), legacy
> command line options (vl.c) and machine .compat_props.
>
> If a property does not exist on the instance of an existing type,
> object_property_parse() will raise an Error and we will abort in
> device_post_init().
>
> What we are silently missing is if a type is misspelled; for that we can
> probably add an Error **errp to the two qdev_prop_register_global*()
> functions - assuming QOM types are already registered at that point.
> qom-test would help us catch any such mistake by instantiating each machine.

Even then, I suspect sooner or later machines other than PC and Q35 will 
be versioned.  At that point we'll probably want QEMU-global 
compat_props that automatically apply to all machines, even if a type is 
not missing.  I think we should already approximate this behavior by 
allowing machine .compat_props where the type doesn't exist.

Paolo

> I note that your proposed check is not failing, but still, with hot-add
> of e.g. PCI devices we might well get a global property default for a
> type that is not instantiated immediately but possibly used later on.

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

On 04/19/14 16:54, Paolo Bonzini wrote:
> Il 18/04/2014 11:21, Andreas Färber ha scritto:
>> Improving this is greatly appreciated, thanks.
>>
>> Now, I can see two ways things can go wrong: a) Mistyping or later
>> refactoring devices, or b) user typos or thinkos.
>> And four ways to set globals: -global, config file (I hope?), legacy
>> command line options (vl.c) and machine .compat_props.
>>
>> If a property does not exist on the instance of an existing type,
>> object_property_parse() will raise an Error and we will abort in
>> device_post_init().
>>
>> What we are silently missing is if a type is misspelled; for that we can
>> probably add an Error **errp to the two qdev_prop_register_global*()
>> functions - assuming QOM types are already registered at that point.
>> qom-test would help us catch any such mistake by instantiating each machine.
>
> Even then, I suspect sooner or later machines other than PC and Q35 will be versioned.  At that point we'll probably want QEMU-global compat_props that automatically apply to all machines, even if a type is not missing.  I think we should already approximate this behavior by allowing machine .compat_props where the type doesn't exist.
>

If I am reading this correctly, this is asking that .compat_props to not
be reported.  This currently happens because of not_used being false
in all C99 struct initializations.

And that Andreas Färber would like me to extend qom-test to check
that at least 1 instance of each machine does use each .compat_props
so that a typo there gets "checked".

    -Don Slutz

> Paolo
>
>> I note that your proposed check is not failing, but still, with hot-add
>> of e.g. PCI devices we might well get a global property default for a
>> type that is not instantiated immediately but possibly used later on.
>

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Paolo Bonzini]

Il 22/04/2014 19:13, Don Slutz ha scritto:
>>
>> Even then, I suspect sooner or later machines other than PC and Q35
>> will be versioned.  At that point we'll probably want QEMU-global
>> compat_props that automatically apply to all machines, even if a type
>> is not missing.  I think we should already approximate this behavior
>> by allowing machine .compat_props where the type doesn't exist.
>
> If I am reading this correctly, this is asking that .compat_props to not
> be reported.  This currently happens because of not_used being false
> in all C99 struct initializations.

Oh, I see.  That's obvious in retrospect, but perhaps a comment can be 
useful around the definition of the not_used field.

Paolo

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

On 04/22/14 20:28, Paolo Bonzini wrote:
> Il 22/04/2014 19:13, Don Slutz ha scritto:
>>>
>>> Even then, I suspect sooner or later machines other than PC and Q35
>>> will be versioned.  At that point we'll probably want QEMU-global
>>> compat_props that automatically apply to all machines, even if a type
>>> is not missing.  I think we should already approximate this behavior
>>> by allowing machine .compat_props where the type doesn't exist.
>>
>> If I am reading this correctly, this is asking that .compat_props to not
>> be reported.  This currently happens because of not_used being false
>> in all C99 struct initializations.
>
> Oh, I see.  That's obvious in retrospect, but perhaps a comment can be 
> useful around the definition of the not_used field.
>

Will add a comment:

/**
  * GlobalProperty:
  * @not_used: Track use of a global property.  Defaults to false in all 
C99 struct initializations.
  *
  * This prevents reports of .compat_props when they are not used.
  */

(Not sure if this is the correct formatting.)

    -Don Slutz

> Paolo

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Andreas Färber]

Am 23.04.2014 01:13, schrieb Don Slutz:
> And that Andreas Färber would like me to extend qom-test to check
> that at least 1 instance of each machine does use each .compat_props
> so that a typo there gets "checked".

No, the machines are already being checked. I was saying, if we are
going to check correctness of types for .compat_props, we can easily
test for typos using make check (or make check-qtest).

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

On 04/18/14 11:21, Andreas Färber wrote:
> Hi Don,
>
> Am 25.03.2014 00:55, schrieb Don Slutz:
>> This can help a user understand why -global was ignored.
>>
>> For example: with "-vga cirrus"; "-global vga.vgamem_mb=16" is just
>> ignored when "-global cirrus-vga.vgamem_mb=16" is not.
>>
>> This is currently clear when the wrong property is provided:
>>
>> out/x86_64-softmmu/qemu-system-x86_64 -global cirrus-vga.vram_size_mb=16 -monitor pty -vga cirrus
>> char device redirected to /dev/pts/20 (label compat_monitor0)
>> qemu-system-x86_64: Property '.vram_size_mb' not found
>> Aborted (core dumped)
>>
>> vs
>>
>> out/x86_64-softmmu/qemu-system-x86_64 -global vga.vram_size_mb=16 -monitor pty -vga cirrus
>> char device redirected to /dev/pts/20 (label compat_monitor0)
>> VNC server running on `::1:5900'
>> ^Cqemu: terminating on signal 2
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> Improving this is greatly appreciated, thanks.
>
> Now, I can see two ways things can go wrong: a) Mistyping or later
> refactoring devices, or b) user typos or thinkos.
> And four ways to set globals: -global, config file (I hope?), legacy
> command line options (vl.c) and machine .compat_props.
>
> If a property does not exist on the instance of an existing type,
> object_property_parse() will raise an Error and we will abort in
> device_post_init().
>
> What we are silently missing is if a type is misspelled; for that we can
> probably add an Error **errp to the two qdev_prop_register_global*()
> functions - assuming QOM types are already registered at that point.
> qom-test would help us catch any such mistake by instantiating each machine.

I assume you are talking about qdev_prop_register_global() and
qdev_prop_register_global_list().  In my testing I did not see
QOM types being registered at that point.  I may have not been
looking at the right thing.  What I am sure on is that the new
object pc-memory-layout (added in 2/4) is not there just like
TYPE_ICC_BRIDGE at the calls to qdev_prop_register_global*().

Currently I have issues running tests:

dcs-xen-50:~/qemu/out>make test
make -C tests/tcg test
make[1]: Entering directory `/home/don/qemu/out/tests/tcg'
cc -m32 -I/home/don/qemu/tcg -I/home/don/qemu/tcg/i386 -I/home/don/qemu/linux-headers -I/home/don/qemu/out/linux-headers -I. -I/home/don/qemu -I/home/don/qemu/include -I/home/don/qemu/libcacard -I/home/don/qemu/tests/tcg -I. -I../.. -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -Wall -O2 -g -fno-strict-aliasing -c -o test_path.o /home/don/qemu/tests/tcg/test_path.c
In file included from /home/don/qemu/util/cutils.c:25:0,
                  from /home/don/qemu/tests/tcg/test_path.c:4:
/home/don/qemu/include/qemu/host-utils.h: In function 'mulu64':
/home/don/qemu/include/qemu/host-utils.h:35:5: error: unknown type name '__uint128_t'
/home/don/qemu/include/qemu/host-utils.h:35:22: error: '__uint128_t' undeclared (first use in this function)
/home/don/qemu/include/qemu/host-utils.h:35:22: note: each undeclared identifier is reported only once for each function it appears in
...




>
> I note that your proposed check is not failing, but still, with hot-add
> of e.g. PCI devices we might well get a global property default for a
> type that is not instantiated immediately but possibly used later on.

This looks correct to me.  I do not know enough in the area, but
at a quick look, type_register_static() could look at .hotpluggable
and .props.  and maybe do some checking.

>> ---
>>   hw/core/qdev-properties-system.c |  1 +
>>   hw/core/qdev-properties.c        | 15 +++++++++++++++
>>   include/hw/qdev-core.h           |  1 +
>>   include/hw/qdev-properties.h     |  1 +
>>   vl.c                             |  2 ++
>>   5 files changed, 20 insertions(+)
> FWIW I'd prefer "qdev:" for consistency (and yes, it's ambiguous), since
> there are no "GlobalProperty" files or directory.
>

Ok, Will change.

>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>> index de83561..9c742ca 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -444,6 +444,7 @@ static int qdev_add_one_global(QemuOpts *opts, void *opaque)
>>       g->driver   = qemu_opt_get(opts, "driver");
>>       g->property = qemu_opt_get(opts, "property");
>>       g->value    = qemu_opt_get(opts, "value");
>> +    g->not_used = true;
>>       qdev_prop_register_global(g);
>>       return 0;
>>   }
> IIUC your patch relies on not_used being false in the non-QemuOpts case
> to avoid noise when using -nodefaults or pc*-x.y. Still, the C99 struct
> initializations elsewhere get that field as well, hmm. An alternative
> would be a separate linked list for user-supplied globals.
>

Yes, a separate linked list could be used.  Did not look at
doing this.  Mostly since struct init will always set not_used to false.


>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index c67acf5..437c008 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -951,6 +951,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
>>       }
>>   }
>>   
>> +void qdev_prop_check_global(void)
>> +{
>> +    GlobalProperty *prop;
>> +
>> +    QTAILQ_FOREACH(prop, &global_props, next) {
>> +        if (!prop->not_used) {
>> +            continue;
>> +        }
>> +        fprintf(stderr, "Warning: \"-global %s.%s=%s\" not used\n",
>> +                prop->driver, prop->property, prop->value);
>> +
>> +    }
>> +}
>> +
>>   void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>>                                       Error **errp)
>>   {
>> @@ -962,6 +976,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>>           if (strcmp(typename, prop->driver) != 0) {
>>               continue;
>>           }
>> +        prop->not_used = false;
>>           object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
>>           if (err != NULL) {
>>               error_propagate(errp, err);
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index dbe473c..131fb49 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -235,6 +235,7 @@ typedef struct GlobalProperty {
>>       const char *driver;
>>       const char *property;
>>       const char *value;
>> +    bool not_used;
>>       QTAILQ_ENTRY(GlobalProperty) next;
>>   } GlobalProperty;
>>   
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index c46e908..fbca313 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -180,6 +180,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>>   
>>   void qdev_prop_register_global(GlobalProperty *prop);
>>   void qdev_prop_register_global_list(GlobalProperty *props);
>> +void qdev_prop_check_global(void);
>>   void qdev_prop_set_globals(DeviceState *dev, Error **errp);
>>   void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename,
>>                                       Error **errp);
>> diff --git a/vl.c b/vl.c
>> index acd97a8..61fac1b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4490,6 +4490,8 @@ int main(int argc, char **argv, char **envp)
>>           }
>>       }
>>   
>> +    qdev_prop_check_global();
> I have some doubts about this placement. A machine init done notifier
> might avoid touching vl.c by leaving it in qdev-properties.c. It happens
> to be after that point as is, but later refactorings wrt QOM realize or
> unrelated issues might change that.

Yes,  This was the best place I found for it.  I am happy to move
if that makes sense.

    -Don Slutz

>> +
>>       if (incoming) {
>>           Error *local_err = NULL;
>>           qemu_start_incoming_migration(incoming, &local_err);
> Regards,
> Andreas
>

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Paolo Bonzini]

Il 22/04/2014 16:23, Don Slutz ha scritto:
>
> Currently I have issues running tests:
>
> dcs-xen-50:~/qemu/out>make test

Use "make check", not "make test".  make test is old and suffered some 
bitrot.

Paolo

Re: [PATCH v3 2/4] GlobalProperty: Display warning about unused -global

[Don Slutz]

On 04/22/14 20:28, Paolo Bonzini wrote:
> Il 22/04/2014 16:23, Don Slutz ha scritto:
>>
>> Currently I have issues running tests:
>>
>> dcs-xen-50:~/qemu/out>make test
>
> Use "make check", not "make test".  make test is old and suffered some 
> bitrot.
>
> Paolo

Thanks, "make check" is working.
     -Don Slutz

[Qemu-devel] [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Don Slutz]

This new object has the property max-ram-below-4g.

If you add enough PCI devices then all mmio for them will not fit
below 4G which may not be the layout the user wanted. This allows
you to increase the below 4G address space that PCI devices can use
(aka decrease ram below 4G) and therefore in more cases not have any
mmio that is above 4G.

For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
the command line will limit the amount of ram that is below 4G to
2G.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/i386/pc.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc_piix.c    | 23 +++++++++++++++++++++--
 hw/i386/pc_q35.c     | 23 +++++++++++++++++++++--
 include/hw/i386/pc.h |  2 ++
 4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 14f0d91..45339f3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1429,3 +1429,45 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
         gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
     }
 }
+
+/* pc-memory-layout stuff */
+
+typedef struct PcMemoryLayoutState PcMemoryLayoutState;
+
+/**
+ * @PcMemoryLayoutState:
+ */
+struct PcMemoryLayoutState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+
+    uint64_t max_ram_below_4g;
+};
+
+static Property pc_memory_layout_props[] = {
+    DEFINE_PROP_SIZE("max-ram-below-4g", PcMemoryLayoutState,
+                     max_ram_below_4g, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pc_memory_layout_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = pc_memory_layout_props;
+}
+
+static const TypeInfo pc_memory_layout_info = {
+    .name = TYPE_PC_MEMORY_LAYOUT,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(PcMemoryLayoutState),
+    .class_init = pc_memory_layout_class_init,
+};
+
+static void pc_memory_layout_register_types(void)
+{
+    type_register_static(&pc_memory_layout_info);
+}
+
+type_init(pc_memory_layout_register_types)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index bd52f6e..81d730d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -95,6 +95,23 @@ static void pc_init1(QEMUMachineInitArgs *args,
     DeviceState *icc_bridge;
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
+    Object *pc_memory_layout;
+    uint64_t max_ram_below_4g;
+    ram_addr_t lowmem = 0xe0000000;
+
+    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
+    max_ram_below_4g = object_property_get_int(pc_memory_layout,
+                                               "max-ram-below-4g",
+                                               NULL);
+    if (max_ram_below_4g) {
+        if (max_ram_below_4g > (1ULL << 32)) {
+            fprintf(stderr,
+                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
+                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
+            max_ram_below_4g = 1ULL << 32;
+        }
+        lowmem = max_ram_below_4g;
+    }
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
      * If it doesn't, we need to split it in chunks below and above 4G.
@@ -103,8 +120,10 @@ static void pc_init1(QEMUMachineInitArgs *args,
      * For old machine types, use whatever split we used historically to avoid
      * breaking migration.
      */
-    if (args->ram_size >= 0xe0000000) {
-        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
+    if (args->ram_size >= lowmem) {
+        if (!max_ram_below_4g && gigabyte_align) {
+            lowmem = 0xc0000000;
+        }
         above_4g_mem_size = args->ram_size - lowmem;
         below_4g_mem_size = lowmem;
     } else {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6e34fe1..529f53d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -82,6 +82,23 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     PCIDevice *ahci;
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
+    Object *pc_memory_layout;
+    uint64_t max_ram_below_4g;
+    ram_addr_t lowmem = 0xb0000000;
+
+    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
+    max_ram_below_4g = object_property_get_int(pc_memory_layout,
+                                               "max-ram-below-4g",
+                                               NULL);
+    if (max_ram_below_4g) {
+        if (max_ram_below_4g > (1ULL << 32)) {
+            fprintf(stderr,
+                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
+                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
+            max_ram_below_4g = 1ULL << 32;
+        }
+        lowmem = max_ram_below_4g;
+    }
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -92,8 +109,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
      * For old machine types, use whatever split we used historically to avoid
      * breaking migration.
      */
-    if (args->ram_size >= 0xb0000000) {
-        ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000;
+    if (args->ram_size >= lowmem) {
+        if (!max_ram_below_4g && gigabyte_align) {
+            lowmem = 0x80000000;
+        }
         above_4g_mem_size = args->ram_size - lowmem;
         below_4g_mem_size = lowmem;
     } else {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9010246..9162d61 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -163,6 +163,8 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg);
 
 void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
+#define TYPE_PC_MEMORY_LAYOUT "pc-memory-layout"
+
 /* acpi_piix.c */
 
 I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-- 
1.8.4

[PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Don Slutz]

This new object has the property max-ram-below-4g.

If you add enough PCI devices then all mmio for them will not fit
below 4G which may not be the layout the user wanted. This allows
you to increase the below 4G address space that PCI devices can use
(aka decrease ram below 4G) and therefore in more cases not have any
mmio that is above 4G.

For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
the command line will limit the amount of ram that is below 4G to
2G.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 hw/i386/pc.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc_piix.c    | 23 +++++++++++++++++++++--
 hw/i386/pc_q35.c     | 23 +++++++++++++++++++++--
 include/hw/i386/pc.h |  2 ++
 4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 14f0d91..45339f3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1429,3 +1429,45 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
         gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
     }
 }
+
+/* pc-memory-layout stuff */
+
+typedef struct PcMemoryLayoutState PcMemoryLayoutState;
+
+/**
+ * @PcMemoryLayoutState:
+ */
+struct PcMemoryLayoutState {
+    /*< private >*/
+    Object parent_obj;
+    /*< public >*/
+
+    uint64_t max_ram_below_4g;
+};
+
+static Property pc_memory_layout_props[] = {
+    DEFINE_PROP_SIZE("max-ram-below-4g", PcMemoryLayoutState,
+                     max_ram_below_4g, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pc_memory_layout_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = pc_memory_layout_props;
+}
+
+static const TypeInfo pc_memory_layout_info = {
+    .name = TYPE_PC_MEMORY_LAYOUT,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(PcMemoryLayoutState),
+    .class_init = pc_memory_layout_class_init,
+};
+
+static void pc_memory_layout_register_types(void)
+{
+    type_register_static(&pc_memory_layout_info);
+}
+
+type_init(pc_memory_layout_register_types)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index bd52f6e..81d730d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -95,6 +95,23 @@ static void pc_init1(QEMUMachineInitArgs *args,
     DeviceState *icc_bridge;
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
+    Object *pc_memory_layout;
+    uint64_t max_ram_below_4g;
+    ram_addr_t lowmem = 0xe0000000;
+
+    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
+    max_ram_below_4g = object_property_get_int(pc_memory_layout,
+                                               "max-ram-below-4g",
+                                               NULL);
+    if (max_ram_below_4g) {
+        if (max_ram_below_4g > (1ULL << 32)) {
+            fprintf(stderr,
+                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
+                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
+            max_ram_below_4g = 1ULL << 32;
+        }
+        lowmem = max_ram_below_4g;
+    }
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
      * If it doesn't, we need to split it in chunks below and above 4G.
@@ -103,8 +120,10 @@ static void pc_init1(QEMUMachineInitArgs *args,
      * For old machine types, use whatever split we used historically to avoid
      * breaking migration.
      */
-    if (args->ram_size >= 0xe0000000) {
-        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
+    if (args->ram_size >= lowmem) {
+        if (!max_ram_below_4g && gigabyte_align) {
+            lowmem = 0xc0000000;
+        }
         above_4g_mem_size = args->ram_size - lowmem;
         below_4g_mem_size = lowmem;
     } else {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6e34fe1..529f53d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -82,6 +82,23 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     PCIDevice *ahci;
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
+    Object *pc_memory_layout;
+    uint64_t max_ram_below_4g;
+    ram_addr_t lowmem = 0xb0000000;
+
+    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
+    max_ram_below_4g = object_property_get_int(pc_memory_layout,
+                                               "max-ram-below-4g",
+                                               NULL);
+    if (max_ram_below_4g) {
+        if (max_ram_below_4g > (1ULL << 32)) {
+            fprintf(stderr,
+                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
+                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
+            max_ram_below_4g = 1ULL << 32;
+        }
+        lowmem = max_ram_below_4g;
+    }
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -92,8 +109,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
      * For old machine types, use whatever split we used historically to avoid
      * breaking migration.
      */
-    if (args->ram_size >= 0xb0000000) {
-        ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000;
+    if (args->ram_size >= lowmem) {
+        if (!max_ram_below_4g && gigabyte_align) {
+            lowmem = 0x80000000;
+        }
         above_4g_mem_size = args->ram_size - lowmem;
         below_4g_mem_size = lowmem;
     } else {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9010246..9162d61 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -163,6 +163,8 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg);
 
 void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
+#define TYPE_PC_MEMORY_LAYOUT "pc-memory-layout"
+
 /* acpi_piix.c */
 
 I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-- 
1.8.4

Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Andreas Färber]

Am 25.03.2014 00:55, schrieb Don Slutz:
> This new object has the property max-ram-below-4g.
> 
> If you add enough PCI devices then all mmio for them will not fit
> below 4G which may not be the layout the user wanted. This allows
> you to increase the below 4G address space that PCI devices can use
> (aka decrease ram below 4G) and therefore in more cases not have any
> mmio that is above 4G.
> 
> For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
> the command line will limit the amount of ram that is below 4G to
> 2G.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  hw/i386/pc.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/pc_piix.c    | 23 +++++++++++++++++++++--
>  hw/i386/pc_q35.c     | 23 +++++++++++++++++++++--
>  include/hw/i386/pc.h |  2 ++
>  4 files changed, 86 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 14f0d91..45339f3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1429,3 +1429,45 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
>      }
>  }
> +
> +/* pc-memory-layout stuff */
> +
> +typedef struct PcMemoryLayoutState PcMemoryLayoutState;

PCMemoryLayoutState?

> +
> +/**
> + * @PcMemoryLayoutState:

Please drop @.

> + */
> +struct PcMemoryLayoutState {
> +    /*< private >*/
> +    Object parent_obj;
> +    /*< public >*/
> +
> +    uint64_t max_ram_below_4g;
> +};
> +
> +static Property pc_memory_layout_props[] = {
> +    DEFINE_PROP_SIZE("max-ram-below-4g", PcMemoryLayoutState,
> +                     max_ram_below_4g, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pc_memory_layout_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = pc_memory_layout_props;
> +}
> +
> +static const TypeInfo pc_memory_layout_info = {
> +    .name = TYPE_PC_MEMORY_LAYOUT,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(PcMemoryLayoutState),
> +    .class_init = pc_memory_layout_class_init,
> +};
> +
> +static void pc_memory_layout_register_types(void)
> +{
> +    type_register_static(&pc_memory_layout_info);
> +}
> +
> +type_init(pc_memory_layout_register_types)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index bd52f6e..81d730d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -95,6 +95,23 @@ static void pc_init1(QEMUMachineInitArgs *args,
>      DeviceState *icc_bridge;
>      FWCfgState *fw_cfg = NULL;
>      PcGuestInfo *guest_info;
> +    Object *pc_memory_layout;
> +    uint64_t max_ram_below_4g;
> +    ram_addr_t lowmem = 0xe0000000;
> +
> +    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
> +    max_ram_below_4g = object_property_get_int(pc_memory_layout,
> +                                               "max-ram-below-4g",
> +                                               NULL);
> +    if (max_ram_below_4g) {

Isn't this always zero due to the default value?

> +        if (max_ram_below_4g > (1ULL << 32)) {
> +            fprintf(stderr,
> +                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
> +                    __func__, (long long)max_ram_below_4g, 1ULL << 32);

Is this forgotten debug output? For a user, __func__ is not helpful and
error_report() would be preferable. Also please just use PRIx64 for
max_ram_below_4g rather than casting.

> +            max_ram_below_4g = 1ULL << 32;
> +        }
> +        lowmem = max_ram_below_4g;
> +    }
>  
>      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
>       * If it doesn't, we need to split it in chunks below and above 4G.
> @@ -103,8 +120,10 @@ static void pc_init1(QEMUMachineInitArgs *args,
>       * For old machine types, use whatever split we used historically to avoid
>       * breaking migration.
>       */
> -    if (args->ram_size >= 0xe0000000) {
> -        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
> +    if (args->ram_size >= lowmem) {
> +        if (!max_ram_below_4g && gigabyte_align) {
> +            lowmem = 0xc0000000;
> +        }
>          above_4g_mem_size = args->ram_size - lowmem;
>          below_4g_mem_size = lowmem;
>      } else {

We don't have central, recursive realization of devices yet. That means
your device is instantiated but never realized, and while you're not
exposing any global state it allows to tweak the property value
throughout device lifetime.

Also you're forgetting to add this device to the composition tree via
object_property_add_child(). Neither ACPI code nor future QOM code can
discover it that way.

> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 6e34fe1..529f53d 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -82,6 +82,23 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      PCIDevice *ahci;
>      DeviceState *icc_bridge;
>      PcGuestInfo *guest_info;
> +    Object *pc_memory_layout;
> +    uint64_t max_ram_below_4g;
> +    ram_addr_t lowmem = 0xb0000000;
> +
> +    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
> +    max_ram_below_4g = object_property_get_int(pc_memory_layout,
> +                                               "max-ram-below-4g",
> +                                               NULL);
> +    if (max_ram_below_4g) {
> +        if (max_ram_below_4g > (1ULL << 32)) {
> +            fprintf(stderr,
> +                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
> +                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
> +            max_ram_below_4g = 1ULL << 32;
> +        }
> +        lowmem = max_ram_below_4g;
> +    }
>  
>      /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>       * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
> @@ -92,8 +109,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>       * For old machine types, use whatever split we used historically to avoid
>       * breaking migration.
>       */
> -    if (args->ram_size >= 0xb0000000) {
> -        ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000;
> +    if (args->ram_size >= lowmem) {
> +        if (!max_ram_below_4g && gigabyte_align) {
> +            lowmem = 0x80000000;
> +        }
>          above_4g_mem_size = args->ram_size - lowmem;
>          below_4g_mem_size = lowmem;
>      } else {

Dito here for all pc_piix.c comments.

> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 9010246..9162d61 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -163,6 +163,8 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg);
>  
>  void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>  
> +#define TYPE_PC_MEMORY_LAYOUT "pc-memory-layout"
> +
>  /* acpi_piix.c */
>  
>  I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Don Slutz]

On 04/18/14 11:45, Andreas Färber wrote:
> Am 25.03.2014 00:55, schrieb Don Slutz:
>> This new object has the property max-ram-below-4g.
>>
>> If you add enough PCI devices then all mmio for them will not fit
>> below 4G which may not be the layout the user wanted. This allows
>> you to increase the below 4G address space that PCI devices can use
>> (aka decrease ram below 4G) and therefore in more cases not have any
>> mmio that is above 4G.
>>
>> For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
>> the command line will limit the amount of ram that is below 4G to
>> 2G.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   hw/i386/pc.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   hw/i386/pc_piix.c    | 23 +++++++++++++++++++++--
>>   hw/i386/pc_q35.c     | 23 +++++++++++++++++++++--
>>   include/hw/i386/pc.h |  2 ++
>>   4 files changed, 86 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 14f0d91..45339f3 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1429,3 +1429,45 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>>           gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
>>       }
>>   }
>> +
>> +/* pc-memory-layout stuff */
>> +
>> +typedef struct PcMemoryLayoutState PcMemoryLayoutState;
> PCMemoryLayoutState?

Fine with me.

>> +
>> +/**
>> + * @PcMemoryLayoutState:
> Please drop @.

Will do.

>> + */
>> +struct PcMemoryLayoutState {
>> +    /*< private >*/
>> +    Object parent_obj;
>> +    /*< public >*/
>> +
>> +    uint64_t max_ram_below_4g;
>> +};
>> +
>> +static Property pc_memory_layout_props[] = {
>> +    DEFINE_PROP_SIZE("max-ram-below-4g", PcMemoryLayoutState,
>> +                     max_ram_below_4g, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pc_memory_layout_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->props = pc_memory_layout_props;
>> +}
>> +
>> +static const TypeInfo pc_memory_layout_info = {
>> +    .name = TYPE_PC_MEMORY_LAYOUT,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(PcMemoryLayoutState),
>> +    .class_init = pc_memory_layout_class_init,
>> +};
>> +
>> +static void pc_memory_layout_register_types(void)
>> +{
>> +    type_register_static(&pc_memory_layout_info);
>> +}
>> +
>> +type_init(pc_memory_layout_register_types)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index bd52f6e..81d730d 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -95,6 +95,23 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>       DeviceState *icc_bridge;
>>       FWCfgState *fw_cfg = NULL;
>>       PcGuestInfo *guest_info;
>> +    Object *pc_memory_layout;
>> +    uint64_t max_ram_below_4g;
>> +    ram_addr_t lowmem = 0xe0000000;
>> +
>> +    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
>> +    max_ram_below_4g = object_property_get_int(pc_memory_layout,
>> +                                               "max-ram-below-4g",
>> +                                               NULL);
>> +    if (max_ram_below_4g) {
> Isn't this always zero due to the default value?

Nope. I forget if object_new() or object_property_get_int()
check the global list.  So if the user sets this, the value is
found at this time.

>> +        if (max_ram_below_4g > (1ULL << 32)) {
>> +            fprintf(stderr,
>> +                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
>> +                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
> Is this forgotten debug output? For a user, __func__ is not helpful and
> error_report() would be preferable. Also please just use PRIx64 for
> max_ram_below_4g rather than casting.

Nope.  Will change to use error_report() without __func__ and PRIx64.


>> +            max_ram_below_4g = 1ULL << 32;
>> +        }
>> +        lowmem = max_ram_below_4g;
>> +    }
>>   
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory).
>>        * If it doesn't, we need to split it in chunks below and above 4G.
>> @@ -103,8 +120,10 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>        * For old machine types, use whatever split we used historically to avoid
>>        * breaking migration.
>>        */
>> -    if (args->ram_size >= 0xe0000000) {
>> -        ram_addr_t lowmem = gigabyte_align ? 0xc0000000 : 0xe0000000;
>> +    if (args->ram_size >= lowmem) {
>> +        if (!max_ram_below_4g && gigabyte_align) {
>> +            lowmem = 0xc0000000;
>> +        }
>>           above_4g_mem_size = args->ram_size - lowmem;
>>           below_4g_mem_size = lowmem;
>>       } else {
> We don't have central, recursive realization of devices yet. That means
> your device is instantiated but never realized, and while you're not
> exposing any global state it allows to tweak the property value
> throughout device lifetime.

Not sure how to mark this as read only.  I have not considered
how this can be changed.  There are a lot of other internals that
are generated off of this like ram_size_below_4g.

>
> Also you're forgetting to add this device to the composition tree via
> object_property_add_child(). Neither ACPI code nor future QOM code can
> discover it that way.

Ok, will add a call on object_property_add_child(). Or is the
fact that this ends up in PcGuestInfoState and passed directly
to acpi_setup() mean that it should not be?



>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 6e34fe1..529f53d 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -82,6 +82,23 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>       PCIDevice *ahci;
>>       DeviceState *icc_bridge;
>>       PcGuestInfo *guest_info;
>> +    Object *pc_memory_layout;
>> +    uint64_t max_ram_below_4g;
>> +    ram_addr_t lowmem = 0xb0000000;
>> +
>> +    pc_memory_layout = object_new(TYPE_PC_MEMORY_LAYOUT);
>> +    max_ram_below_4g = object_property_get_int(pc_memory_layout,
>> +                                               "max-ram-below-4g",
>> +                                               NULL);
>> +    if (max_ram_below_4g) {
>> +        if (max_ram_below_4g > (1ULL << 32)) {
>> +            fprintf(stderr,
>> +                    "%s: max_ram_below_4g=%lld too big adjusted to %lld\n",
>> +                    __func__, (long long)max_ram_below_4g, 1ULL << 32);
>> +            max_ram_below_4g = 1ULL << 32;
>> +        }
>> +        lowmem = max_ram_below_4g;
>> +    }
>>   
>>       /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
>>        * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
>> @@ -92,8 +109,10 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>        * For old machine types, use whatever split we used historically to avoid
>>        * breaking migration.
>>        */
>> -    if (args->ram_size >= 0xb0000000) {
>> -        ram_addr_t lowmem = gigabyte_align ? 0x80000000 : 0xb0000000;
>> +    if (args->ram_size >= lowmem) {
>> +        if (!max_ram_below_4g && gigabyte_align) {
>> +            lowmem = 0x80000000;
>> +        }
>>           above_4g_mem_size = args->ram_size - lowmem;
>>           below_4g_mem_size = lowmem;
>>       } else {
> Dito here for all pc_piix.c comments.

Same as above.
     -Don Slutz

>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 9010246..9162d61 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -163,6 +163,8 @@ void cpu_smm_register(cpu_set_smm_t callback, void *arg);
>>   
>>   void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>>   
>> +#define TYPE_PC_MEMORY_LAYOUT "pc-memory-layout"
>> +
>>   /* acpi_piix.c */
>>   
>>   I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> Regards,
> Andreas
>

Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Paolo Bonzini]

Il 24/03/2014 19:55, Don Slutz ha scritto:
> This new object has the property max-ram-below-4g.
>
> If you add enough PCI devices then all mmio for them will not fit
> below 4G which may not be the layout the user wanted. This allows
> you to increase the below 4G address space that PCI devices can use
> (aka decrease ram below 4G) and therefore in more cases not have any
> mmio that is above 4G.
>
> For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
> the command line will limit the amount of ram that is below 4G to
> 2G.

Does Xen's firmware allow 64-bit BARs?  I'm wondering why this is not a 
problem on KVM.

Also, overloading -global like this isn't the best long term choice.  We 
should get custom -machine properties in 2.1, we should use them.

Paolo

Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Don Slutz]

On 04/21/14 08:27, Paolo Bonzini wrote:
> Il 24/03/2014 19:55, Don Slutz ha scritto:
>> This new object has the property max-ram-below-4g.
>>
>> If you add enough PCI devices then all mmio for them will not fit
>> below 4G which may not be the layout the user wanted. This allows
>> you to increase the below 4G address space that PCI devices can use
>> (aka decrease ram below 4G) and therefore in more cases not have any
>> mmio that is above 4G.
>>
>> For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
>> the command line will limit the amount of ram that is below 4G to
>> 2G.
>
> Does Xen's firmware allow 64-bit BARs?

Yes.  But not all supported devices do.

> I'm wondering why this is not a problem on KVM.
>

I suspect that it is.  It only shows up when you add a lot of
PCI devices either directly or via pci pass through.  Also it
is more Xen focused because the default ram below 4G is
much larger so that 32 bit only guests can have a lot of ram.

> Also, overloading -global like this isn't the best long term choice.  We should get custom -machine properties in 2.1, we should use them.
>

I may have missed this.  Is this

[PATCH V3 0/5] remove QEMUMachine indirection from MachineClass

Or some patch that is yet to be posted?

(I did add this to qemu_machine_opts and changed to the current
way to handle:

Subject: 	Re: [Qemu-devel] [PATCH 1/1] vl.c: Add pci_hole_min_size machine option.
Date: 	Wed, 5 Mar 2014 08:46:33 +0200
From: 	Michael S. Tsirkin <mst@redhat.com>
To: 	Don Slutz <dslutz@verizon.com>
CC: 	<xen-devel@lists.xensource.com>, <qemu-devel@nongnu.org>, Anthony Liguori <aliguori@amazon.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>



On Tue, Mar 04, 2014 at 06:51:11PM -0500, Don Slutz wrote:
> On 03/04/14 17:20, Michael S. Tsirkin wrote:
> >On Tue, Mar 04, 2014 at 02:18:49PM -0500, Don Slutz wrote:
> >>On 03/04/14 11:58, Michael S. Tsirkin wrote:
> >>>On Tue, Mar 04, 2014 at 11:36:44AM -0500, Don Slutz wrote:
> >>>>On 03/04/14 02:34, Michael S. Tsirkin wrote:
> >>>>>On Thu, Feb 27, 2014 at 05:32:23PM -0500, Don Slutz wrote:
> >>>>>>This allows growing the pci_hole to the size needed.

...

> Yes. Also, please find a way to only add it to
> machine types that support it, instead of ignoring it
> silently for those that don't.

)

> Paolo

Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Paolo Bonzini]

Il 22/04/2014 20:13, Don Slutz ha scritto:
>
>
> (I did add this to qemu_machine_opts and changed to the current
> way to handle:
>
> Subject:     Re: [Qemu-devel] [PATCH 1/1] vl.c: Add pci_hole_min_size
> machine option.
> Date:     Wed, 5 Mar 2014 08:46:33 +0200
> From:     Michael S. Tsirkin <mst@redhat.com>
> To:     Don Slutz <dslutz@verizon.com>
> CC:     <xen-devel@lists.xensource.com>, <qemu-devel@nongnu.org>,
> Anthony Liguori <aliguori@amazon.com>, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com>
>

Yeah, this would have added pci_hole_min_size to all machines though. 
Marcel's work will allow per-machine customization of -machine.  I'm 
sorry that you were led down the "wrong" way.

Paolo

Re: [PATCH v3 3/4] pc & q35: Add new object pc-memory-layout.

[Marcel Apfelbaum]

On Tue, 2014-04-22 at 20:13 -0400, Don Slutz wrote:
> On 04/21/14 08:27, Paolo Bonzini wrote:
> > Il 24/03/2014 19:55, Don Slutz ha scritto:
> >> This new object has the property max-ram-below-4g.
> >>
> >> If you add enough PCI devices then all mmio for them will not fit
> >> below 4G which may not be the layout the user wanted. This allows
> >> you to increase the below 4G address space that PCI devices can use
> >> (aka decrease ram below 4G) and therefore in more cases not have any
> >> mmio that is above 4G.
> >>
> >> For example adding "-global pc-memory-layout.max-ram-below-4g=2G" to
> >> the command line will limit the amount of ram that is below 4G to
> >> 2G.
> >
> > Does Xen's firmware allow 64-bit BARs?
> 
> Yes.  But not all supported devices do.
> 
> > I'm wondering why this is not a problem on KVM.
> >
> 
> I suspect that it is.  It only shows up when you add a lot of
> PCI devices either directly or via pci pass through.  Also it
> is more Xen focused because the default ram below 4G is
> much larger so that 32 bit only guests can have a lot of ram.
> 
> > Also, overloading -global like this isn't the best long term choice.  We should get custom -machine properties in 2.1, we should use them.
> >
> 
> I may have missed this.  Is this
> 
> [PATCH V3 0/5] remove QEMUMachine indirection from MachineClass
> 
> Or some patch that is yet to be posted?
Hi Don,

I posted a patch some time ago and I am going to post it again soon with some modifications:
https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg00041.html

Basically what matters are these lines:

    machine_opts = qemu_get_machine_opts();
+    if (qemu_opt_foreach(machine_opts, object_set_property, current_machine, 1) < 0) {
+        object_unref(OBJECT(current_machine));
+        exit(1);
+    }

You create a subtype of MACHINE which has this property and it will be automatically
assigned. The positive thing here is that the property is not global anymore. 

Thanks,
Marcel


> 
> (I did add this to qemu_machine_opts and changed to the current
> way to handle:
> 
> Subject: 	Re: [Qemu-devel] [PATCH 1/1] vl.c: Add pci_hole_min_size machine option.
> Date: 	Wed, 5 Mar 2014 08:46:33 +0200
> From: 	Michael S. Tsirkin <mst@redhat.com>
> To: 	Don Slutz <dslutz@verizon.com>
> CC: 	<xen-devel@lists.xensource.com>, <qemu-devel@nongnu.org>, Anthony Liguori <aliguori@amazon.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> 
> 
> On Tue, Mar 04, 2014 at 06:51:11PM -0500, Don Slutz wrote:
> > On 03/04/14 17:20, Michael S. Tsirkin wrote:
> > >On Tue, Mar 04, 2014 at 02:18:49PM -0500, Don Slutz wrote:
> > >>On 03/04/14 11:58, Michael S. Tsirkin wrote:
> > >>>On Tue, Mar 04, 2014 at 11:36:44AM -0500, Don Slutz wrote:
> > >>>>On 03/04/14 02:34, Michael S. Tsirkin wrote:
> > >>>>>On Thu, Feb 27, 2014 at 05:32:23PM -0500, Don Slutz wrote:
> > >>>>>>This allows growing the pci_hole to the size needed.
> 
> ...
> 
> > Yes. Also, please find a way to only add it to
> > machine types that support it, instead of ignoring it
> > silently for those that don't.
> 
> )
> 
> > Paolo
> 
> 

[Qemu-devel] [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Don Slutz]

This is the xen part of "pc & q35: Add new object pc-memory-layout."

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3: Adjust for code readability. Set max_ram_below_4g always and use
it to calculate above_4g_mem_size, below_4g_mem_size.

 hw/i386/pc_piix.c    |  4 ++--
 hw/i386/pc_q35.c     |  4 ++--
 include/hw/xen/xen.h |  4 ++--
 xen-all.c            | 40 +++++++++++++++++++++-------------------
 xen-stub.c           |  4 ++--
 5 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 81d730d..8a93548 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
         below_4g_mem_size = args->ram_size;
     }
 
-    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
-                                      &ram_memory) != 0) {
+    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
+                                      &above_4g_mem_size, &ram_memory) != 0) {
         fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
         exit(1);
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 529f53d..09e98e6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
         below_4g_mem_size = args->ram_size;
     }
 
-    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
-                                      &ram_memory) != 0) {
+    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
+                                      &above_4g_mem_size, &ram_memory) != 0) {
         fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
         exit(1);
     }
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 0f3942e..eca39a5 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
-                 MemoryRegion **ram_memory);
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
                    struct MemoryRegion *mr);
 void xen_modified_memory(ram_addr_t start, ram_addr_t length);
diff --git a/xen-all.c b/xen-all.c
index c64300c..3f6f890 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
 
 /* Memory Ops */
 
-static void xen_ram_init(ram_addr_t *below_4g_mem_size,
+static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
+                         ram_addr_t *below_4g_mem_size,
                          ram_addr_t *above_4g_mem_size,
-                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
+                         MemoryRegion **ram_memory_p)
 {
     MemoryRegion *sysmem = get_system_memory();
     ram_addr_t block_len;
 
-    block_len = ram_size;
-    if (ram_size >= HVM_BELOW_4G_RAM_END) {
-        /* Xen does not allocate the memory continuously, and keep a hole at
-         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
-         */
-        block_len += HVM_BELOW_4G_MMIO_LENGTH;
-    }
-    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
-    *ram_memory_p = &ram_memory;
-    vmstate_register_ram_global(&ram_memory);
-
-    if (ram_size >= HVM_BELOW_4G_RAM_END) {
-        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
-        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
+    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;
+    if (ram_size >= max_ram_below_4g) {
+        *above_4g_mem_size = ram_size - max_ram_below_4g;
+        *below_4g_mem_size = max_ram_below_4g;
     } else {
         *above_4g_mem_size = 0;
         *below_4g_mem_size = ram_size;
     }
+    if (!*above_4g_mem_size) {
+        block_len = ram_size;
+    } else {
+        /* Xen does not allocate the memory continuously, and keep a hole of
+         * of the size computed above or passed in. */
+        block_len = (1ULL << 32) + *above_4g_mem_size;
+    }
+    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
+    *ram_memory_p = &ram_memory;
+    vmstate_register_ram_global(&ram_memory);
 
     memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
                              &ram_memory, 0, 0xa0000);
@@ -1069,8 +1070,8 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
-                 MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
 {
     int i, rc;
     unsigned long ioreq_pfn;
@@ -1148,7 +1149,8 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
 
     /* Init RAM management */
     xen_map_cache_init(xen_phys_offset_to_gaddr, state);
-    xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
+    xen_ram_init(ram_size, max_ram_below_4g, below_4g_mem_size,
+                 above_4g_mem_size, ram_memory);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
diff --git a/xen-stub.c b/xen-stub.c
index 0302dff..dd317a5 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -64,8 +64,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
 {
 }
 
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
-                 MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
 {
     return 0;
 }
-- 
1.8.4

[PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Don Slutz]

This is the xen part of "pc & q35: Add new object pc-memory-layout."

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v3: Adjust for code readability. Set max_ram_below_4g always and use
it to calculate above_4g_mem_size, below_4g_mem_size.

 hw/i386/pc_piix.c    |  4 ++--
 hw/i386/pc_q35.c     |  4 ++--
 include/hw/xen/xen.h |  4 ++--
 xen-all.c            | 40 +++++++++++++++++++++-------------------
 xen-stub.c           |  4 ++--
 5 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 81d730d..8a93548 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
         below_4g_mem_size = args->ram_size;
     }
 
-    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
-                                      &ram_memory) != 0) {
+    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
+                                      &above_4g_mem_size, &ram_memory) != 0) {
         fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
         exit(1);
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 529f53d..09e98e6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
         below_4g_mem_size = args->ram_size;
     }
 
-    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
-                                      &ram_memory) != 0) {
+    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
+                                      &above_4g_mem_size, &ram_memory) != 0) {
         fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
         exit(1);
     }
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 0f3942e..eca39a5 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
-                 MemoryRegion **ram_memory);
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
                    struct MemoryRegion *mr);
 void xen_modified_memory(ram_addr_t start, ram_addr_t length);
diff --git a/xen-all.c b/xen-all.c
index c64300c..3f6f890 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
 
 /* Memory Ops */
 
-static void xen_ram_init(ram_addr_t *below_4g_mem_size,
+static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
+                         ram_addr_t *below_4g_mem_size,
                          ram_addr_t *above_4g_mem_size,
-                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
+                         MemoryRegion **ram_memory_p)
 {
     MemoryRegion *sysmem = get_system_memory();
     ram_addr_t block_len;
 
-    block_len = ram_size;
-    if (ram_size >= HVM_BELOW_4G_RAM_END) {
-        /* Xen does not allocate the memory continuously, and keep a hole at
-         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
-         */
-        block_len += HVM_BELOW_4G_MMIO_LENGTH;
-    }
-    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
-    *ram_memory_p = &ram_memory;
-    vmstate_register_ram_global(&ram_memory);
-
-    if (ram_size >= HVM_BELOW_4G_RAM_END) {
-        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
-        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
+    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;
+    if (ram_size >= max_ram_below_4g) {
+        *above_4g_mem_size = ram_size - max_ram_below_4g;
+        *below_4g_mem_size = max_ram_below_4g;
     } else {
         *above_4g_mem_size = 0;
         *below_4g_mem_size = ram_size;
     }
+    if (!*above_4g_mem_size) {
+        block_len = ram_size;
+    } else {
+        /* Xen does not allocate the memory continuously, and keep a hole of
+         * of the size computed above or passed in. */
+        block_len = (1ULL << 32) + *above_4g_mem_size;
+    }
+    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
+    *ram_memory_p = &ram_memory;
+    vmstate_register_ram_global(&ram_memory);
 
     memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
                              &ram_memory, 0, 0xa0000);
@@ -1069,8 +1070,8 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
-                 MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
 {
     int i, rc;
     unsigned long ioreq_pfn;
@@ -1148,7 +1149,8 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
 
     /* Init RAM management */
     xen_map_cache_init(xen_phys_offset_to_gaddr, state);
-    xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
+    xen_ram_init(ram_size, max_ram_below_4g, below_4g_mem_size,
+                 above_4g_mem_size, ram_memory);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
diff --git a/xen-stub.c b/xen-stub.c
index 0302dff..dd317a5 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -64,8 +64,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
 {
 }
 
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
-                 MemoryRegion **ram_memory)
+int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
+                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
 {
     return 0;
 }
-- 
1.8.4

Re: [Qemu-devel] [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Stefano Stabellini]

On Mon, 24 Mar 2014, Don Slutz wrote:
> This is the xen part of "pc & q35: Add new object pc-memory-layout."
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> v3: Adjust for code readability. Set max_ram_below_4g always and use
> it to calculate above_4g_mem_size, below_4g_mem_size.
> 
>  hw/i386/pc_piix.c    |  4 ++--
>  hw/i386/pc_q35.c     |  4 ++--
>  include/hw/xen/xen.h |  4 ++--
>  xen-all.c            | 40 +++++++++++++++++++++-------------------
>  xen-stub.c           |  4 ++--
>  5 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 81d730d..8a93548 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 529f53d..09e98e6 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 0f3942e..eca39a5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
>  void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>  
>  #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory);
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>                     struct MemoryRegion *mr);
>  void xen_modified_memory(ram_addr_t start, ram_addr_t length);
> diff --git a/xen-all.c b/xen-all.c
> index c64300c..3f6f890 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
>  
>  /* Memory Ops */
>  
> -static void xen_ram_init(ram_addr_t *below_4g_mem_size,
> +static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
> +                         ram_addr_t *below_4g_mem_size,
>                           ram_addr_t *above_4g_mem_size,
> -                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
> +                         MemoryRegion **ram_memory_p)
>  {
>      MemoryRegion *sysmem = get_system_memory();
>      ram_addr_t block_len;
>  
> -    block_len = ram_size;
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        /* Xen does not allocate the memory continuously, and keep a hole at
> -         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
> -         */
> -        block_len += HVM_BELOW_4G_MMIO_LENGTH;
> -    }
> -    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> -    *ram_memory_p = &ram_memory;
> -    vmstate_register_ram_global(&ram_memory);
> -
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
> -        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
> +    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;
> +    if (ram_size >= max_ram_below_4g) {
> +        *above_4g_mem_size = ram_size - max_ram_below_4g;
> +        *below_4g_mem_size = max_ram_below_4g;
>      } else {
>          *above_4g_mem_size = 0;
>          *below_4g_mem_size = ram_size;
>      }
> +    if (!*above_4g_mem_size) {
> +        block_len = ram_size;
> +    } else {
> +        /* Xen does not allocate the memory continuously, and keep a hole of
> +         * of the size computed above or passed in. */
> +        block_len = (1ULL << 32) + *above_4g_mem_size;
> +    }
> +    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> +    *ram_memory_p = &ram_memory;
> +    vmstate_register_ram_global(&ram_memory);
>  
>      memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
>                               &ram_memory, 0, 0xa0000);
> @@ -1069,8 +1070,8 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>      xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
>  }
>  
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory)
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
>  {
>      int i, rc;
>      unsigned long ioreq_pfn;
> @@ -1148,7 +1149,8 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>  
>      /* Init RAM management */
>      xen_map_cache_init(xen_phys_offset_to_gaddr, state);
> -    xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
> +    xen_ram_init(ram_size, max_ram_below_4g, below_4g_mem_size,
> +                 above_4g_mem_size, ram_memory);
>  
>      qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
>  
> diff --git a/xen-stub.c b/xen-stub.c
> index 0302dff..dd317a5 100644
> --- a/xen-stub.c
> +++ b/xen-stub.c
> @@ -64,8 +64,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>  {
>  }
>  
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory)
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
>  {
>      return 0;
>  }
> -- 
> 1.8.4
> 

Re: [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Stefano Stabellini]

On Mon, 24 Mar 2014, Don Slutz wrote:
> This is the xen part of "pc & q35: Add new object pc-memory-layout."
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> v3: Adjust for code readability. Set max_ram_below_4g always and use
> it to calculate above_4g_mem_size, below_4g_mem_size.
> 
>  hw/i386/pc_piix.c    |  4 ++--
>  hw/i386/pc_q35.c     |  4 ++--
>  include/hw/xen/xen.h |  4 ++--
>  xen-all.c            | 40 +++++++++++++++++++++-------------------
>  xen-stub.c           |  4 ++--
>  5 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 81d730d..8a93548 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 529f53d..09e98e6 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 0f3942e..eca39a5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
>  void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>  
>  #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory);
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>                     struct MemoryRegion *mr);
>  void xen_modified_memory(ram_addr_t start, ram_addr_t length);
> diff --git a/xen-all.c b/xen-all.c
> index c64300c..3f6f890 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
>  
>  /* Memory Ops */
>  
> -static void xen_ram_init(ram_addr_t *below_4g_mem_size,
> +static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
> +                         ram_addr_t *below_4g_mem_size,
>                           ram_addr_t *above_4g_mem_size,
> -                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
> +                         MemoryRegion **ram_memory_p)
>  {
>      MemoryRegion *sysmem = get_system_memory();
>      ram_addr_t block_len;
>  
> -    block_len = ram_size;
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        /* Xen does not allocate the memory continuously, and keep a hole at
> -         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
> -         */
> -        block_len += HVM_BELOW_4G_MMIO_LENGTH;
> -    }
> -    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> -    *ram_memory_p = &ram_memory;
> -    vmstate_register_ram_global(&ram_memory);
> -
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
> -        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
> +    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;
> +    if (ram_size >= max_ram_below_4g) {
> +        *above_4g_mem_size = ram_size - max_ram_below_4g;
> +        *below_4g_mem_size = max_ram_below_4g;
>      } else {
>          *above_4g_mem_size = 0;
>          *below_4g_mem_size = ram_size;
>      }
> +    if (!*above_4g_mem_size) {
> +        block_len = ram_size;
> +    } else {
> +        /* Xen does not allocate the memory continuously, and keep a hole of
> +         * of the size computed above or passed in. */
> +        block_len = (1ULL << 32) + *above_4g_mem_size;
> +    }
> +    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> +    *ram_memory_p = &ram_memory;
> +    vmstate_register_ram_global(&ram_memory);
>  
>      memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
>                               &ram_memory, 0, 0xa0000);
> @@ -1069,8 +1070,8 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>      xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
>  }
>  
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory)
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
>  {
>      int i, rc;
>      unsigned long ioreq_pfn;
> @@ -1148,7 +1149,8 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>  
>      /* Init RAM management */
>      xen_map_cache_init(xen_phys_offset_to_gaddr, state);
> -    xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
> +    xen_ram_init(ram_size, max_ram_below_4g, below_4g_mem_size,
> +                 above_4g_mem_size, ram_memory);
>  
>      qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
>  
> diff --git a/xen-stub.c b/xen-stub.c
> index 0302dff..dd317a5 100644
> --- a/xen-stub.c
> +++ b/xen-stub.c
> @@ -64,8 +64,8 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>  {
>  }
>  
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory)
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory)
>  {
>      return 0;
>  }
> -- 
> 1.8.4
> 

Re: [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Andreas Färber]

Am 25.03.2014 00:55, schrieb Don Slutz:
> This is the xen part of "pc & q35: Add new object pc-memory-layout."
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
> v3: Adjust for code readability. Set max_ram_below_4g always and use
> it to calculate above_4g_mem_size, below_4g_mem_size.
> 
>  hw/i386/pc_piix.c    |  4 ++--
>  hw/i386/pc_q35.c     |  4 ++--
>  include/hw/xen/xen.h |  4 ++--
>  xen-all.c            | 40 +++++++++++++++++++++-------------------
>  xen-stub.c           |  4 ++--
>  5 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 81d730d..8a93548 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 529f53d..09e98e6 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>          below_4g_mem_size = args->ram_size;
>      }
>  
> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
> -                                      &ram_memory) != 0) {
> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>          fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>          exit(1);
>      }
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index 0f3942e..eca39a5 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
>  void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>  
>  #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> -                 MemoryRegion **ram_memory);
> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
>  void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>                     struct MemoryRegion *mr);
>  void xen_modified_memory(ram_addr_t start, ram_addr_t length);
> diff --git a/xen-all.c b/xen-all.c
> index c64300c..3f6f890 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
>  
>  /* Memory Ops */
>  
> -static void xen_ram_init(ram_addr_t *below_4g_mem_size,
> +static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
> +                         ram_addr_t *below_4g_mem_size,
>                           ram_addr_t *above_4g_mem_size,
> -                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
> +                         MemoryRegion **ram_memory_p)
>  {
>      MemoryRegion *sysmem = get_system_memory();
>      ram_addr_t block_len;
>  
> -    block_len = ram_size;
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        /* Xen does not allocate the memory continuously, and keep a hole at
> -         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
> -         */
> -        block_len += HVM_BELOW_4G_MMIO_LENGTH;
> -    }
> -    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> -    *ram_memory_p = &ram_memory;
> -    vmstate_register_ram_global(&ram_memory);
> -
> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
> -        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
> -        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
> +    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;

Isn't max_ram_below_4g still always zero at this point?

Are you guys still using your own Xen machine types? Then you could set
a compat_props entry for all (not just legacy) machines to override the
actual pc-memory-layout max-ram-below-4g property value to avoid the ?:
here. Especially since I'm not seeing this modified value being written
back to the device.

Considering that machines are objects now, might it be easier to set
this property on the machine object itself rather than having an unused
stub object hanging in limbo? Would create a dependency on Marcel's
series though (which like this series I now need to start reviewing).

> +    if (ram_size >= max_ram_below_4g) {
> +        *above_4g_mem_size = ram_size - max_ram_below_4g;
> +        *below_4g_mem_size = max_ram_below_4g;
>      } else {
>          *above_4g_mem_size = 0;
>          *below_4g_mem_size = ram_size;
>      }
> +    if (!*above_4g_mem_size) {
> +        block_len = ram_size;
> +    } else {
> +        /* Xen does not allocate the memory continuously, and keep a hole of
> +         * of the size computed above or passed in. */

Please keep */ in the front for easily spotting where the comment ends.

Possibly even /* is requested to be on its own line by Coding Style...?
But that'd be a mere code movement.

> +        block_len = (1ULL << 32) + *above_4g_mem_size;
> +    }
> +    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
> +    *ram_memory_p = &ram_memory;
> +    vmstate_register_ram_global(&ram_memory);
>  
>      memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
>                               &ram_memory, 0, 0xa0000);
[snip]

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v3 4/4] xen-all: Pass max_ram_below_4g to xen_hvm_init.

[Don Slutz]

On 04/18/14 12:19, Andreas Färber wrote:
> Am 25.03.2014 00:55, schrieb Don Slutz:
>> This is the xen part of "pc & q35: Add new object pc-memory-layout."
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>> v3: Adjust for code readability. Set max_ram_below_4g always and use
>> it to calculate above_4g_mem_size, below_4g_mem_size.
>>
>>   hw/i386/pc_piix.c    |  4 ++--
>>   hw/i386/pc_q35.c     |  4 ++--
>>   include/hw/xen/xen.h |  4 ++--
>>   xen-all.c            | 40 +++++++++++++++++++++-------------------
>>   xen-stub.c           |  4 ++--
>>   5 files changed, 29 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 81d730d..8a93548 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -131,8 +131,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>           below_4g_mem_size = args->ram_size;
>>       }
>>   
>> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
>> -                                      &ram_memory) != 0) {
>> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
>> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>>           fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>>           exit(1);
>>       }
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 529f53d..09e98e6 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -120,8 +120,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>           below_4g_mem_size = args->ram_size;
>>       }
>>   
>> -    if (xen_enabled() && xen_hvm_init(&below_4g_mem_size, &above_4g_mem_size,
>> -                                      &ram_memory) != 0) {
>> +    if (xen_enabled() && xen_hvm_init(max_ram_below_4g, &below_4g_mem_size,
>> +                                      &above_4g_mem_size, &ram_memory) != 0) {
>>           fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
>>           exit(1);
>>       }
>> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
>> index 0f3942e..eca39a5 100644
>> --- a/include/hw/xen/xen.h
>> +++ b/include/hw/xen/xen.h
>> @@ -40,8 +40,8 @@ int xen_init(QEMUMachine *machine);
>>   void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>>   
>>   #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
>> -int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>> -                 MemoryRegion **ram_memory);
>> +int xen_hvm_init(ram_addr_t max_ram_below_4g, ram_addr_t *below_4g_mem_size,
>> +                 ram_addr_t *above_4g_mem_size, MemoryRegion **ram_memory);
>>   void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
>>                      struct MemoryRegion *mr);
>>   void xen_modified_memory(ram_addr_t start, ram_addr_t length);
>> diff --git a/xen-all.c b/xen-all.c
>> index c64300c..3f6f890 100644
>> --- a/xen-all.c
>> +++ b/xen-all.c
>> @@ -155,31 +155,32 @@ qemu_irq *xen_interrupt_controller_init(void)
>>   
>>   /* Memory Ops */
>>   
>> -static void xen_ram_init(ram_addr_t *below_4g_mem_size,
>> +static void xen_ram_init(ram_addr_t ram_size, ram_addr_t max_ram_below_4g,
>> +                         ram_addr_t *below_4g_mem_size,
>>                            ram_addr_t *above_4g_mem_size,
>> -                         ram_addr_t ram_size, MemoryRegion **ram_memory_p)
>> +                         MemoryRegion **ram_memory_p)
>>   {
>>       MemoryRegion *sysmem = get_system_memory();
>>       ram_addr_t block_len;
>>   
>> -    block_len = ram_size;
>> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
>> -        /* Xen does not allocate the memory continuously, and keep a hole at
>> -         * HVM_BELOW_4G_MMIO_START of HVM_BELOW_4G_MMIO_LENGTH
>> -         */
>> -        block_len += HVM_BELOW_4G_MMIO_LENGTH;
>> -    }
>> -    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
>> -    *ram_memory_p = &ram_memory;
>> -    vmstate_register_ram_global(&ram_memory);
>> -
>> -    if (ram_size >= HVM_BELOW_4G_RAM_END) {
>> -        *above_4g_mem_size = ram_size - HVM_BELOW_4G_RAM_END;
>> -        *below_4g_mem_size = HVM_BELOW_4G_RAM_END;
>> +    max_ram_below_4g = max_ram_below_4g ? max_ram_below_4g : HVM_BELOW_4G_RAM_END;
> Isn't max_ram_below_4g still always zero at this point?

No. Patch 3/4 gets this from the global "max-ram-below-4g".  So
it is only 0 when the user has not set this.

> Are you guys still using your own Xen machine types?

Yes and no:

             flexarray_append(dm_args, "pc,accel=xen");
         } else {
             flexarray_append(dm_args, "xenfv");


>   Then you could set
> a compat_props entry for all (not just legacy) machines to override the
> actual pc-memory-layout max-ram-below-4g property value to avoid the ?:
> here. Especially since I'm not seeing this modified value being written
> back to the device.

I think this is still possible.  Will look into it.  Did not see any
need to write this back.


> Considering that machines are objects now, might it be easier to set
> this property on the machine object itself rather than having an unused
> stub object hanging in limbo?

That would be fine with me.

>   Would create a dependency on Marcel's
> series though (which like this series I now need to start reviewing).

I will look into using this machine object(s).  What I found was:


[PATCH v3 0/3] qemu-machine as a QOM object
[PATCH V3 0/5] remove QEMUMachine indirection from MachineClass

And if I have it correct, you are referring to the 2nd of these as
a dependency.



>> +    if (ram_size >= max_ram_below_4g) {
>> +        *above_4g_mem_size = ram_size - max_ram_below_4g;
>> +        *below_4g_mem_size = max_ram_below_4g;
>>       } else {
>>           *above_4g_mem_size = 0;
>>           *below_4g_mem_size = ram_size;
>>       }
>> +    if (!*above_4g_mem_size) {
>> +        block_len = ram_size;
>> +    } else {
>> +        /* Xen does not allocate the memory continuously, and keep a hole of
>> +         * of the size computed above or passed in. */
> Please keep */ in the front for easily spotting where the comment ends.
>
> Possibly even /* is requested to be on its own line by Coding Style...?
> But that'd be a mere code movement.
>

I did not find any statement about this in CODING_STYLE, but will adjust
to this.

    -Don Slutz

>> +        block_len = (1ULL << 32) + *above_4g_mem_size;
>> +    }
>> +    memory_region_init_ram(&ram_memory, NULL, "xen.ram", block_len);
>> +    *ram_memory_p = &ram_memory;
>> +    vmstate_register_ram_global(&ram_memory);
>>   
>>       memory_region_init_alias(&ram_640k, NULL, "xen.ram.640k",
>>                                &ram_memory, 0, 0xa0000);
> [snip]
>
> Regards,
> Andreas
>

Re: [Qemu-devel] [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
> Changes v2 to v3:
>   Stefano Stabellini:
>     Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
>     Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
>        Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
>        below_4g_mem_size.
> 
> Changes v1 to v2:
>   Michael S. Tsirkin:
>     Rename option.
>     Only add it to machine types that support it.
>   Split into 4 parts.
> 
> 1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
> 
>   This looks to be a possible bug that has yet to be found.
>   below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
>   (pc_guest_info_init) which are currently not "correct".  This and
>   4/4 change the same lines.
> 
> 2/4 -- GlobalProperty: Display warning about unused -global
> 
>     My testing showed that setting a global property on an object
>     that is not used is not reported at all.  This is added to help
>     when the new global is set but not used.  The negative not_used
>     was picked so that all static objects are assumed to be used
>     even when they are not.
> 
> 3/4 -- pc & q35: Add new object pc-memory-layout
> 
>   The objects that it might make sense to add this property to all
>   get created too late.  So add a new object just to hold this
>   property.  Name it so that it is expected that only pc (and q35)
>   machine types support it.
> 
> 4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
> 
>   Seprate the xen only part of the change.  Currectly based on patch 1/4
> 
> Don Slutz (4):
>   xen-all: Fix xen_hvm_init() to adjust pc memory layout.
>   GlobalProperty: Display warning about unused -global
>   pc & q35: Add new object pc-memory-layout.
>   xen-all: Pass max_ram_below_4g to xen_hvm_init.
> 
>  hw/core/qdev-properties-system.c |  1 +
>  hw/core/qdev-properties.c        | 15 ++++++++++++
>  hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
>  hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
>  hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
>  include/hw/i386/pc.h             |  2 ++
>  include/hw/qdev-core.h           |  1 +
>  include/hw/qdev-properties.h     |  1 +
>  include/hw/xen/xen.h             |  3 ++-
>  vl.c                             |  2 ++
>  xen-all.c                        | 46 +++++++++++++++++++----------------
>  xen-stub.c                       |  3 ++-
>  12 files changed, 165 insertions(+), 53 deletions(-)

Andreas, could you review pls, esp patch 2?

> -- 
> 1.8.4

Re: [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
> Changes v2 to v3:
>   Stefano Stabellini:
>     Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
>     Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
>        Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
>        below_4g_mem_size.
> 
> Changes v1 to v2:
>   Michael S. Tsirkin:
>     Rename option.
>     Only add it to machine types that support it.
>   Split into 4 parts.
> 
> 1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
> 
>   This looks to be a possible bug that has yet to be found.
>   below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
>   (pc_guest_info_init) which are currently not "correct".  This and
>   4/4 change the same lines.
> 
> 2/4 -- GlobalProperty: Display warning about unused -global
> 
>     My testing showed that setting a global property on an object
>     that is not used is not reported at all.  This is added to help
>     when the new global is set but not used.  The negative not_used
>     was picked so that all static objects are assumed to be used
>     even when they are not.
> 
> 3/4 -- pc & q35: Add new object pc-memory-layout
> 
>   The objects that it might make sense to add this property to all
>   get created too late.  So add a new object just to hold this
>   property.  Name it so that it is expected that only pc (and q35)
>   machine types support it.
> 
> 4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
> 
>   Seprate the xen only part of the change.  Currectly based on patch 1/4
> 
> Don Slutz (4):
>   xen-all: Fix xen_hvm_init() to adjust pc memory layout.
>   GlobalProperty: Display warning about unused -global
>   pc & q35: Add new object pc-memory-layout.
>   xen-all: Pass max_ram_below_4g to xen_hvm_init.
> 
>  hw/core/qdev-properties-system.c |  1 +
>  hw/core/qdev-properties.c        | 15 ++++++++++++
>  hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
>  hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
>  hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
>  include/hw/i386/pc.h             |  2 ++
>  include/hw/qdev-core.h           |  1 +
>  include/hw/qdev-properties.h     |  1 +
>  include/hw/xen/xen.h             |  3 ++-
>  vl.c                             |  2 ++
>  xen-all.c                        | 46 +++++++++++++++++++----------------
>  xen-stub.c                       |  3 ++-
>  12 files changed, 165 insertions(+), 53 deletions(-)

Andreas, could you review pls, esp patch 2?

> -- 
> 1.8.4

PING Re: [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Don Slutz]

Ping since 2.0 is now out.
    -Don Slutz

On 03/25/14 05:08, Michael S. Tsirkin wrote:
> On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
>> Changes v2 to v3:
>>    Stefano Stabellini:
>>      Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
>>      Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
>>         Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
>>         below_4g_mem_size.
>>
>> Changes v1 to v2:
>>    Michael S. Tsirkin:
>>      Rename option.
>>      Only add it to machine types that support it.
>>    Split into 4 parts.
>>
>> 1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
>>
>>    This looks to be a possible bug that has yet to be found.
>>    below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
>>    (pc_guest_info_init) which are currently not "correct".  This and
>>    4/4 change the same lines.
>>
>> 2/4 -- GlobalProperty: Display warning about unused -global
>>
>>      My testing showed that setting a global property on an object
>>      that is not used is not reported at all.  This is added to help
>>      when the new global is set but not used.  The negative not_used
>>      was picked so that all static objects are assumed to be used
>>      even when they are not.
>>
>> 3/4 -- pc & q35: Add new object pc-memory-layout
>>
>>    The objects that it might make sense to add this property to all
>>    get created too late.  So add a new object just to hold this
>>    property.  Name it so that it is expected that only pc (and q35)
>>    machine types support it.
>>
>> 4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
>>
>>    Seprate the xen only part of the change.  Currectly based on patch 1/4
>>
>> Don Slutz (4):
>>    xen-all: Fix xen_hvm_init() to adjust pc memory layout.
>>    GlobalProperty: Display warning about unused -global
>>    pc & q35: Add new object pc-memory-layout.
>>    xen-all: Pass max_ram_below_4g to xen_hvm_init.
>>
>>   hw/core/qdev-properties-system.c |  1 +
>>   hw/core/qdev-properties.c        | 15 ++++++++++++
>>   hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
>>   hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
>>   hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
>>   include/hw/i386/pc.h             |  2 ++
>>   include/hw/qdev-core.h           |  1 +
>>   include/hw/qdev-properties.h     |  1 +
>>   include/hw/xen/xen.h             |  3 ++-
>>   vl.c                             |  2 ++
>>   xen-all.c                        | 46 +++++++++++++++++++----------------
>>   xen-stub.c                       |  3 ++-
>>   12 files changed, 165 insertions(+), 53 deletions(-)
> Andreas, could you review pls, esp patch 2?
>
>> -- 
>> 1.8.4

Re: [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Andreas Färber]

Am 25.03.2014 10:08, schrieb Michael S. Tsirkin:
> On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
>> Changes v2 to v3:
>>   Stefano Stabellini:
>>     Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
>>     Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
>>        Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
>>        below_4g_mem_size.
>>
>> Changes v1 to v2:
>>   Michael S. Tsirkin:
>>     Rename option.
>>     Only add it to machine types that support it.
>>   Split into 4 parts.
>>
>> 1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
>>
>>   This looks to be a possible bug that has yet to be found.
>>   below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
>>   (pc_guest_info_init) which are currently not "correct".  This and
>>   4/4 change the same lines.
>>
>> 2/4 -- GlobalProperty: Display warning about unused -global
>>
>>     My testing showed that setting a global property on an object
>>     that is not used is not reported at all.  This is added to help
>>     when the new global is set but not used.  The negative not_used
>>     was picked so that all static objects are assumed to be used
>>     even when they are not.
>>
>> 3/4 -- pc & q35: Add new object pc-memory-layout
>>
>>   The objects that it might make sense to add this property to all
>>   get created too late.  So add a new object just to hold this
>>   property.  Name it so that it is expected that only pc (and q35)
>>   machine types support it.
>>
>> 4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
>>
>>   Seprate the xen only part of the change.  Currectly based on patch 1/4
>>
>> Don Slutz (4):
>>   xen-all: Fix xen_hvm_init() to adjust pc memory layout.
>>   GlobalProperty: Display warning about unused -global
>>   pc & q35: Add new object pc-memory-layout.
>>   xen-all: Pass max_ram_below_4g to xen_hvm_init.
>>
>>  hw/core/qdev-properties-system.c |  1 +
>>  hw/core/qdev-properties.c        | 15 ++++++++++++
>>  hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
>>  hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
>>  hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
>>  include/hw/i386/pc.h             |  2 ++
>>  include/hw/qdev-core.h           |  1 +
>>  include/hw/qdev-properties.h     |  1 +
>>  include/hw/xen/xen.h             |  3 ++-
>>  vl.c                             |  2 ++
>>  xen-all.c                        | 46 +++++++++++++++++++----------------
>>  xen-stub.c                       |  3 ++-
>>  12 files changed, 165 insertions(+), 53 deletions(-)
> 
> Andreas, could you review pls, esp patch 2?

Michael, I don't see a dependency of the later patches on 2/4, so I'd
like to split that one out of this series and reiterate.

I'll have some minor comments on the rest.

Don, in general please do not end subjects with a dot.

CC'ing Eduardo and Igor who looked into 4G issues previously.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Don Slutz]

On 04/18/14 10:10, Andreas Färber wrote:
> Am 25.03.2014 10:08, schrieb Michael S. Tsirkin:
>> On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
>>> Changes v2 to v3:
>>>    Stefano Stabellini:
>>>      Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
>>>      Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
>>>         Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
>>>         below_4g_mem_size.
>>>
>>> Changes v1 to v2:
>>>    Michael S. Tsirkin:
>>>      Rename option.
>>>      Only add it to machine types that support it.
>>>    Split into 4 parts.
>>>
>>> 1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
>>>
>>>    This looks to be a possible bug that has yet to be found.
>>>    below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
>>>    (pc_guest_info_init) which are currently not "correct".  This and
>>>    4/4 change the same lines.
>>>
>>> 2/4 -- GlobalProperty: Display warning about unused -global
>>>
>>>      My testing showed that setting a global property on an object
>>>      that is not used is not reported at all.  This is added to help
>>>      when the new global is set but not used.  The negative not_used
>>>      was picked so that all static objects are assumed to be used
>>>      even when they are not.
>>>
>>> 3/4 -- pc & q35: Add new object pc-memory-layout
>>>
>>>    The objects that it might make sense to add this property to all
>>>    get created too late.  So add a new object just to hold this
>>>    property.  Name it so that it is expected that only pc (and q35)
>>>    machine types support it.
>>>
>>> 4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
>>>
>>>    Seprate the xen only part of the change.  Currectly based on patch 1/4
>>>
>>> Don Slutz (4):
>>>    xen-all: Fix xen_hvm_init() to adjust pc memory layout.
>>>    GlobalProperty: Display warning about unused -global
>>>    pc & q35: Add new object pc-memory-layout.
>>>    xen-all: Pass max_ram_below_4g to xen_hvm_init.
>>>
>>>   hw/core/qdev-properties-system.c |  1 +
>>>   hw/core/qdev-properties.c        | 15 ++++++++++++
>>>   hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
>>>   hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
>>>   hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
>>>   include/hw/i386/pc.h             |  2 ++
>>>   include/hw/qdev-core.h           |  1 +
>>>   include/hw/qdev-properties.h     |  1 +
>>>   include/hw/xen/xen.h             |  3 ++-
>>>   vl.c                             |  2 ++
>>>   xen-all.c                        | 46 +++++++++++++++++++----------------
>>>   xen-stub.c                       |  3 ++-
>>>   12 files changed, 165 insertions(+), 53 deletions(-)
>> Andreas, could you review pls, esp patch 2?
> Michael, I don't see a dependency of the later patches on 2/4, so I'd
> like to split that one out of this series and reiterate.

I am happy to split out 2/4.

> I'll have some minor comments on the rest.
>
> Don, in general please do not end subjects with a dot.

Ok, will attempt to do so.

    -Don Slutz

> CC'ing Eduardo and Igor who looked into 4G issues previously.
>
> Regards,
> Andreas
>

Re: [Qemu-devel] [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Michael S. Tsirkin]

On Tue, Apr 22, 2014 at 12:31:16PM -0400, Don Slutz wrote:
> On 04/18/14 10:10, Andreas Färber wrote:
> >Am 25.03.2014 10:08, schrieb Michael S. Tsirkin:
> >>On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
> >>>Changes v2 to v3:
> >>>   Stefano Stabellini:
> >>>     Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
> >>>     Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
> >>>        Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
> >>>        below_4g_mem_size.
> >>>
> >>>Changes v1 to v2:
> >>>   Michael S. Tsirkin:
> >>>     Rename option.
> >>>     Only add it to machine types that support it.
> >>>   Split into 4 parts.
> >>>
> >>>1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
> >>>
> >>>   This looks to be a possible bug that has yet to be found.
> >>>   below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
> >>>   (pc_guest_info_init) which are currently not "correct".  This and
> >>>   4/4 change the same lines.
> >>>
> >>>2/4 -- GlobalProperty: Display warning about unused -global
> >>>
> >>>     My testing showed that setting a global property on an object
> >>>     that is not used is not reported at all.  This is added to help
> >>>     when the new global is set but not used.  The negative not_used
> >>>     was picked so that all static objects are assumed to be used
> >>>     even when they are not.
> >>>
> >>>3/4 -- pc & q35: Add new object pc-memory-layout
> >>>
> >>>   The objects that it might make sense to add this property to all
> >>>   get created too late.  So add a new object just to hold this
> >>>   property.  Name it so that it is expected that only pc (and q35)
> >>>   machine types support it.
> >>>
> >>>4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
> >>>
> >>>   Seprate the xen only part of the change.  Currectly based on patch 1/4
> >>>
> >>>Don Slutz (4):
> >>>   xen-all: Fix xen_hvm_init() to adjust pc memory layout.
> >>>   GlobalProperty: Display warning about unused -global
> >>>   pc & q35: Add new object pc-memory-layout.
> >>>   xen-all: Pass max_ram_below_4g to xen_hvm_init.
> >>>
> >>>  hw/core/qdev-properties-system.c |  1 +
> >>>  hw/core/qdev-properties.c        | 15 ++++++++++++
> >>>  hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
> >>>  hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
> >>>  hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
> >>>  include/hw/i386/pc.h             |  2 ++
> >>>  include/hw/qdev-core.h           |  1 +
> >>>  include/hw/qdev-properties.h     |  1 +
> >>>  include/hw/xen/xen.h             |  3 ++-
> >>>  vl.c                             |  2 ++
> >>>  xen-all.c                        | 46 +++++++++++++++++++----------------
> >>>  xen-stub.c                       |  3 ++-
> >>>  12 files changed, 165 insertions(+), 53 deletions(-)
> >>Andreas, could you review pls, esp patch 2?
> >Michael, I don't see a dependency of the later patches on 2/4, so I'd
> >like to split that one out of this series and reiterate.
> 
> I am happy to split out 2/4.

I'm waiting for a repost or did I misunderstand?

> >I'll have some minor comments on the rest.
> >
> >Don, in general please do not end subjects with a dot.
> 
> Ok, will attempt to do so.
> 
>    -Don Slutz
> 
> >CC'ing Eduardo and Igor who looked into 4G issues previously.
> >
> >Regards,
> >Andreas
> >

Re: [Qemu-devel] [PATCH v3 0/4] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

[Don Slutz]

On 05/05/14 04:12, Michael S. Tsirkin wrote:
> On Tue, Apr 22, 2014 at 12:31:16PM -0400, Don Slutz wrote:
>> On 04/18/14 10:10, Andreas Färber wrote:
>>> Am 25.03.2014 10:08, schrieb Michael S. Tsirkin:
>>>> On Mon, Mar 24, 2014 at 07:55:32PM -0400, Don Slutz wrote:
>>>>> Changes v2 to v3:
>>>>>    Stefano Stabellini:
>>>>>      Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
>>>>>      Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
>>>>>         Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
>>>>>         below_4g_mem_size.
>>>>>
>>>>> Changes v1 to v2:
>>>>>    Michael S. Tsirkin:
>>>>>      Rename option.
>>>>>      Only add it to machine types that support it.
>>>>>    Split into 4 parts.
>>>>>
>>>>> 1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
>>>>>
>>>>>    This looks to be a possible bug that has yet to be found.
>>>>>    below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
>>>>>    (pc_guest_info_init) which are currently not "correct".  This and
>>>>>    4/4 change the same lines.
>>>>>
>>>>> 2/4 -- GlobalProperty: Display warning about unused -global
>>>>>
>>>>>      My testing showed that setting a global property on an object
>>>>>      that is not used is not reported at all.  This is added to help
>>>>>      when the new global is set but not used.  The negative not_used
>>>>>      was picked so that all static objects are assumed to be used
>>>>>      even when they are not.
>>>>>
>>>>> 3/4 -- pc & q35: Add new object pc-memory-layout
>>>>>
>>>>>    The objects that it might make sense to add this property to all
>>>>>    get created too late.  So add a new object just to hold this
>>>>>    property.  Name it so that it is expected that only pc (and q35)
>>>>>    machine types support it.
>>>>>
>>>>> 4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
>>>>>
>>>>>    Seprate the xen only part of the change.  Currectly based on patch 1/4
>>>>>
>>>>> Don Slutz (4):
>>>>>    xen-all: Fix xen_hvm_init() to adjust pc memory layout.
>>>>>    GlobalProperty: Display warning about unused -global
>>>>>    pc & q35: Add new object pc-memory-layout.
>>>>>    xen-all: Pass max_ram_below_4g to xen_hvm_init.
>>>>>
>>>>>   hw/core/qdev-properties-system.c |  1 +
>>>>>   hw/core/qdev-properties.c        | 15 ++++++++++++
>>>>>   hw/i386/pc.c                     | 42 ++++++++++++++++++++++++++++++++
>>>>>   hw/i386/pc_piix.c                | 52 +++++++++++++++++++++++++++-------------
>>>>>   hw/i386/pc_q35.c                 | 50 ++++++++++++++++++++++++++------------
>>>>>   include/hw/i386/pc.h             |  2 ++
>>>>>   include/hw/qdev-core.h           |  1 +
>>>>>   include/hw/qdev-properties.h     |  1 +
>>>>>   include/hw/xen/xen.h             |  3 ++-
>>>>>   vl.c                             |  2 ++
>>>>>   xen-all.c                        | 46 +++++++++++++++++++----------------
>>>>>   xen-stub.c                       |  3 ++-
>>>>>   12 files changed, 165 insertions(+), 53 deletions(-)
>>>> Andreas, could you review pls, esp patch 2?
>>> Michael, I don't see a dependency of the later patches on 2/4, so I'd
>>> like to split that one out of this series and reiterate.
>> I am happy to split out 2/4.
> I'm waiting for a repost or did I misunderstand?

Sorry about that.  No you did not misunderstand.  I was looking into how to do
patches 3 & 4.  Also I was still thinking about points that Andreas Färber had
stated.

I should be sending one out soon.

    -Don Slutz

>>> I'll have some minor comments on the rest.
>>>
>>> Don, in general please do not end subjects with a dot.
>> Ok, will attempt to do so.
>>
>>     -Don Slutz
>>
>>> CC'ing Eduardo and Igor who looked into 4G issues previously.
>>>
>>> Regards,
>>> Andreas
>>>