[Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Igor Mammedov]

* simplify PCI address space mapping into system address space,
  replacing code duplication in piix/q53 PCs with helper function
* add fw_cfg 'etc/pcimem64-minimum-address' to allow QEMU reserve
  additional address space before 64-bit PCI hole. Which will be
  need for resevinig memory hotplug region in highmem.

SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/

Igor Mammedov (4):
  pc: sanitize i440fx_init() arguments
  pc: consolidate mapping of PCI address space into system address
    space
  fw_cfg: make cast macro available to world
  pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

 hw/i386/pc.c              |   47 +++++++++++++++++++++++++++++++++-----------
 hw/i386/pc_piix.c         |    5 +--
 hw/nvram/fw_cfg.c         |    4 ---
 hw/pci-host/piix.c        |   34 +++++++++++---------------------
 hw/pci-host/q35.c         |   30 +++++++++-------------------
 include/hw/i386/pc.h      |   22 +++++++-------------
 include/hw/nvram/fw_cfg.h |    7 ++++++
 7 files changed, 74 insertions(+), 75 deletions(-)

[Qemu-devel] [PATCH 1/4] pc: sanitize i440fx_init() arguments

[Igor Mammedov]

* rename pci_hole_start to below_4g_mem_size to reflect what is
  really passed in and move pci_hole_size calculation inside
  i440fx.
* remove ram_size arg from function signature, since it could be
  retrieved as below_4g_mem_size + above_4g_mem_size sum,
  internally.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc_piix.c    |    3 +--
 hw/pci-host/piix.c   |   11 ++++++-----
 include/hw/i386/pc.h |    4 +---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c6042c7..2fed5fd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -145,9 +145,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
-                              system_memory, system_io, args->ram_size,
+                              system_memory, system_io,
                               below_4g_mem_size,
-                              0x100000000ULL - below_4g_mem_size,
                               above_4g_mem_size,
                               pci_memory, ram_memory);
     } else {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index c041149..5b9cc1f 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -311,9 +311,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
+                    hwaddr below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_address_space,
                     MemoryRegion *ram_memory)
@@ -327,6 +325,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     unsigned i;
     I440FXState *i440fx;
     uint64_t pci_hole64_size;
+    ram_addr_t ram_size = below_4g_mem_size + above_4g_mem_size;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
     s = PCI_HOST_BRIDGE(dev);
@@ -355,8 +354,10 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     }
 
     memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
-                             pci_hole_start, pci_hole_size);
-    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
+                             below_4g_mem_size,
+                             0x100000000ULL - below_4g_mem_size);
+    memory_region_add_subregion(f->system_memory, below_4g_mem_size,
+                                &f->pci_hole);
 
     pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6083839..95fc6cc 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -166,9 +166,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     ISABus **isa_bus, qemu_irq *pic,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
-                    ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
+                    hwaddr below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
-- 
1.7.1

[Qemu-devel] [PATCH 2/4] pc: consolidate mapping of PCI address space into system address space

[Igor Mammedov]

Reduce code duplication by moving PCI address space mappings
into a common helper, and reuse code in PIIX and Q35.
Also rename variables to reflect that PCI memory region mappings
are not the same as PCI holes which are programmed by BIOS to
avoid confusion with PCI holes.

PS:
Although keep user visible via "info mtree" names for regions
"pci-hole" and "pci-hole64" to avoid management tools regressions.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c         |   40 ++++++++++++++++++++++++++++------------
 hw/pci-host/piix.c   |   28 ++++++++--------------------
 hw/pci-host/q35.c    |   28 ++++++++--------------------
 include/hw/i386/pc.h |   15 +++++----------
 4 files changed, 49 insertions(+), 62 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0c313fe..7ecb028 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1053,21 +1053,37 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
     return guest_info;
 }
 
-void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start,
-                        uint64_t pci_hole64_size)
+/* setup pci memory regions mappings into system address space */
+void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
+                            MemoryRegion *pci_address_space,
+                            MemoryRegion *pci32_as, MemoryRegion *pci64_as,
+                            uint32_t pci32_as_start, uint32_t pci32_as_size,
+                            uint64_t pci64_as_start, uint64_t pci64_as_size)
 {
-    if ((sizeof(hwaddr) == 4) || (!pci_hole64_size)) {
+    memory_region_init_alias(pci32_as, owner, "pci-hole", pci_address_space,
+                             pci32_as_start, pci32_as_size);
+    memory_region_add_subregion(system_memory, pci32_as_start, pci32_as);
+
+    if (sizeof(hwaddr) == 4) {
         return;
     }
-    /*
-     * BIOS does not set MTRR entries for the 64 bit window, so no need to
-     * align address to power of two.  Align address at 1G, this makes sure
-     * it can be exactly covered with a PAT entry even when using huge
-     * pages.
-     */
-    pci_info->w64.begin = ROUND_UP(pci_hole64_start, 0x1ULL << 30);
-    pci_info->w64.end = pci_info->w64.begin + pci_hole64_size;
-    assert(pci_info->w64.begin <= pci_info->w64.end);
+
+    if (pci64_as_size == DEFAULT_PCI_HOLE64_SIZE) {
+        pci64_as_size = 1ULL << 62;
+    }
+    memory_region_init_alias(pci64_as, owner, "pci-hole64", pci_address_space,
+                             pci64_as_start, pci64_as_size);
+    if (pci64_as_size) {
+        /*
+         * BIOS does not set MTRR entries for the 64 bit window, so no need to
+         * align address to power of two.  Align address at 1G, this makes sure
+         * it can be exactly covered with a PAT entry even when using huge
+         * pages.
+         */
+        memory_region_add_subregion(system_memory,
+                                    ROUND_UP(pci64_as_start, 0x1ULL << 30),
+                                    pci64_as);
+    }
 }
 
 void pc_acpi_init(const char *default_dsdt)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 5b9cc1f..d422b25 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -324,7 +324,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     PCII440FXState *f;
     unsigned i;
     I440FXState *i440fx;
-    uint64_t pci_hole64_size;
     ram_addr_t ram_size = below_4g_mem_size + above_4g_mem_size;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
@@ -353,25 +352,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
         i440fx->pci_info.w32.begin = 0xe0000000;
     }
 
-    memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
-                             below_4g_mem_size,
-                             0x100000000ULL - below_4g_mem_size);
-    memory_region_add_subregion(f->system_memory, below_4g_mem_size,
-                                &f->pci_hole);
-
-    pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size);
-
-    pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size,
-                       pci_hole64_size);
-    memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
-                             f->pci_address_space,
-                             i440fx->pci_info.w64.begin,
-                             pci_hole64_size);
-    if (pci_hole64_size) {
-        memory_region_add_subregion(f->system_memory,
-                                    i440fx->pci_info.w64.begin,
-                                    &f->pci_hole_64bit);
-    }
+    pc_pci_as_mapping_init(OBJECT(d), f->system_memory,
+                           f->pci_address_space,
+                           &f->pci_hole, &f->pci_hole_64bit,
+                           below_4g_mem_size,
+                           0x100000000ULL - below_4g_mem_size,
+                           0x100000000ULL + above_4g_mem_size,
+                           i440fx->pci_hole64_size);
+
     memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
                              f->pci_address_space, 0xa0000, 0x20000);
     memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index ad703a4..cb8aea0 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -336,28 +336,16 @@ static int mch_init(PCIDevice *d)
 {
     int i;
     MCHPCIState *mch = MCH_PCI_DEVICE(d);
-    uint64_t pci_hole64_size;
 
     /* setup pci memory regions */
-    memory_region_init_alias(&mch->pci_hole, OBJECT(mch), "pci-hole",
-                             mch->pci_address_space,
-                             mch->below_4g_mem_size,
-                             0x100000000ULL - mch->below_4g_mem_size);
-    memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
-                                &mch->pci_hole);
-
-    pci_hole64_size = pci_host_get_hole64_size(mch->pci_hole64_size);
-    pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size,
-                       pci_hole64_size);
-    memory_region_init_alias(&mch->pci_hole_64bit, OBJECT(mch), "pci-hole64",
-                             mch->pci_address_space,
-                             mch->pci_info.w64.begin,
-                             pci_hole64_size);
-    if (pci_hole64_size) {
-        memory_region_add_subregion(mch->system_memory,
-                                    mch->pci_info.w64.begin,
-                                    &mch->pci_hole_64bit);
-    }
+    pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
+                           mch->pci_address_space,
+                           &mch->pci_hole, &mch->pci_hole_64bit,
+                           mch->below_4g_mem_size,
+                           0x100000000ULL - mch->below_4g_mem_size,
+                           0x100000000ULL + mch->above_4g_mem_size,
+                           mch->pci_hole64_size);
+
     /* smram */
     cpu_smm_register(&mch_set_smm, mch);
     memory_region_init_alias(&mch->smram_region, OBJECT(mch), "smram-region",
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 95fc6cc..ed86642 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -108,17 +108,12 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
 #define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
 #define DEFAULT_PCI_HOLE64_SIZE (~0x0ULL)
 
-static inline uint64_t pci_host_get_hole64_size(uint64_t pci_hole64_size)
-{
-    if (pci_hole64_size == DEFAULT_PCI_HOLE64_SIZE) {
-        return 1ULL << 62;
-    } else {
-        return pci_hole64_size;
-    }
-}
 
-void pc_init_pci64_hole(PcPciInfo *pci_info, uint64_t pci_hole64_start,
-                        uint64_t pci_hole64_size);
+void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
+                            MemoryRegion *pci_address_space,
+                            MemoryRegion *pci32_as, MemoryRegion *pci64_as,
+                            uint32_t pci32_as_start, uint32_t pci32_as_size,
+                            uint64_t pci64_as_start, uint64_t pci64_as_size);
 
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
-- 
1.7.1

[Qemu-devel] [PATCH 3/4] fw_cfg: make cast macro available to world

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/nvram/fw_cfg.c         |    4 ----
 include/hw/nvram/fw_cfg.h |    7 +++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index d0820e5..6c31e24 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -32,10 +32,6 @@
 
 #define FW_CFG_SIZE 2
 #define FW_CFG_DATA_SIZE 1
-#define TYPE_FW_CFG "fw_cfg"
-#define FW_CFG_NAME "fw_cfg"
-#define FW_CFG_PATH "/machine/" FW_CFG_NAME
-#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
 
 typedef struct FWCfgEntry {
     uint32_t len;
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f60dd67..651fe21 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -6,6 +6,7 @@
 #include <stddef.h>
 
 #include "exec/hwaddr.h"
+#include "qom/object.h"
 #include "qemu/typedefs.h"
 #endif
 
@@ -47,6 +48,12 @@
 #define FW_CFG_INVALID          0xffff
 
 #ifndef NO_QEMU_PROTOS
+
+#define TYPE_FW_CFG "fw_cfg"
+#define FW_CFG_NAME "fw_cfg"
+#define FW_CFG_PATH "/machine/" FW_CFG_NAME
+#define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
+
 typedef struct FWCfgFile {
     uint32_t  size;        /* file size */
     uint16_t  select;      /* write this to 0x510 to read it */
-- 
1.7.1

[Qemu-devel] [PATCH 4/4] pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

[Igor Mammedov]

'etc/pcimem64-minimum-address' will allow QEMU to communicate to BIOS
where PCI memory address space mapping starts in high memory.

Allowing BIOS start mapping 64-bit PCI BARs at address where QEMU
placed this mapping vs. hardcoded value right after highmem RAM.

That will allow QEMU to reserve extra address space before
64-bit PCI hole for memory hotplug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  * SeaBIOS patch: http://patchwork.ozlabs.org/patch/283623/

---
 hw/i386/pc.c         |    9 ++++++++-
 hw/i386/pc_piix.c    |    2 +-
 hw/pci-host/piix.c   |    5 +++--
 hw/pci-host/q35.c    |    4 +++-
 include/hw/i386/pc.h |    5 +++--
 5 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7ecb028..ac99ae1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1055,7 +1055,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
 
 /* setup pci memory regions mappings into system address space */
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
-                            MemoryRegion *pci_address_space,
+                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
                             MemoryRegion *pci32_as, MemoryRegion *pci64_as,
                             uint32_t pci32_as_start, uint32_t pci32_as_size,
                             uint64_t pci64_as_start, uint64_t pci64_as_size)
@@ -1083,6 +1083,13 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
         memory_region_add_subregion(system_memory,
                                     ROUND_UP(pci64_as_start, 0x1ULL << 30),
                                     pci64_as);
+        if (fw_cfg) {
+            uint64_t *pcimem64_start = g_malloc(sizeof(*pcimem64_start));
+
+            *pcimem64_start = cpu_to_le64(pci64_as_start);
+            fw_cfg_add_file(fw_cfg, "etc/pcimem64-minimum-address",
+                            pcimem64_start, sizeof(*pcimem64_start));
+        }
     }
 }
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2fed5fd..966c48a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -148,7 +148,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
                               system_memory, system_io,
                               below_4g_mem_size,
                               above_4g_mem_size,
-                              pci_memory, ram_memory);
+                              pci_memory, ram_memory, fw_cfg);
     } else {
         pci_bus = NULL;
         i440fx_state = NULL;
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index d422b25..9347099 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -314,7 +314,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     hwaddr below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_address_space,
-                    MemoryRegion *ram_memory)
+                    MemoryRegion *ram_memory,
+                    FWCfgState *fw_cfg)
 {
     DeviceState *dev;
     PCIBus *b;
@@ -353,7 +354,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     }
 
     pc_pci_as_mapping_init(OBJECT(d), f->system_memory,
-                           f->pci_address_space,
+                           f->pci_address_space, fw_cfg,
                            &f->pci_hole, &f->pci_hole_64bit,
                            below_4g_mem_size,
                            0x100000000ULL - below_4g_mem_size,
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index cb8aea0..583585c 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -30,6 +30,7 @@
 #include "hw/hw.h"
 #include "hw/pci-host/q35.h"
 #include "qapi/visitor.h"
+#include "hw/nvram/fw_cfg.h"
 
 /****************************************************************************
  * Q35 host
@@ -336,10 +337,11 @@ static int mch_init(PCIDevice *d)
 {
     int i;
     MCHPCIState *mch = MCH_PCI_DEVICE(d);
+    FWCfgState *fw_cfg = FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
 
     /* setup pci memory regions */
     pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
-                           mch->pci_address_space,
+                           mch->pci_address_space, fw_cfg,
                            &mch->pci_hole, &mch->pci_hole_64bit,
                            mch->below_4g_mem_size,
                            0x100000000ULL - mch->below_4g_mem_size,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ed86642..6d0d8c9 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -110,7 +110,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
 
 
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
-                            MemoryRegion *pci_address_space,
+                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
                             MemoryRegion *pci32_as, MemoryRegion *pci64_as,
                             uint32_t pci32_as_start, uint32_t pci32_as_size,
                             uint64_t pci64_as_start, uint64_t pci64_as_size);
@@ -164,7 +164,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     hwaddr below_4g_mem_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_memory,
-                    MemoryRegion *ram_memory);
+                    MemoryRegion *ram_memory,
+                    FWCfgState *fw_cfg);
 
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Michael S. Tsirkin]

On Wed, Oct 16, 2013 at 10:49:10AM +0200, Igor Mammedov wrote:
> * simplify PCI address space mapping into system address space,
>   replacing code duplication in piix/q53 PCs with helper function

I think this does not go far enough.

I was always wondering about PCI hole in QEMU.
Some real PCs have a "PCI hole" where PCI
masks real memory, but PIIX does not do this,
instead PCI is whenever RAM does not mask it.

So it looks like the hole concept is a left-over
from when we didn't have priorities in the memory API.
How about we remove them?
See patch below.
I did a quick boot test and it seems to work, of course
it needs much more testing.
It's on top of Marcel's series adding negative priorities,
so works on top of the acpi branch or the pci branch.

I'm also wondering about the smram region - it uses
priority 1 but does not say why.
Need to check what does it overlap with, and why.



diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index bad3953..988516a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -102,8 +102,6 @@ struct PCII440FXState {
     MemoryRegion *system_memory;
     MemoryRegion *pci_address_space;
     MemoryRegion *ram_memory;
-    MemoryRegion pci_hole;
-    MemoryRegion pci_hole_64bit;
     PAMMemoryRegion pam_regions[13];
     MemoryRegion smram_region;
     uint8_t smm_enabled;
@@ -326,7 +324,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     PCII440FXState *f;
     unsigned i;
     I440FXState *i440fx;
-    uint64_t pci_hole64_size;
 
     dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
     s = PCI_HOST_BRIDGE(dev);
@@ -354,23 +351,9 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
         i440fx->pci_info.w32.begin = 0xe0000000;
     }
 
-    memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
-                             pci_hole_start, pci_hole_size);
-    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
-
-    pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size);
-
-    pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size,
-                       pci_hole64_size);
-    memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
-                             f->pci_address_space,
-                             i440fx->pci_info.w64.begin,
-                             pci_hole64_size);
-    if (pci_hole64_size) {
-        memory_region_add_subregion(f->system_memory,
-                                    i440fx->pci_info.w64.begin,
-                                    &f->pci_hole_64bit);
-    }
+    /* Set to lower priority than RAM */
+    memory_region_add_subregion_overlap(f->system_memory, 0x0,
+                                        f->pci_address_space, -1);
     memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
                              f->pci_address_space, 0xa0000, 0x20000);
     memory_region_add_subregion_overlap(f->system_memory, 0xa0000,

Re: [Qemu-devel] [PATCH 4/4] pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

[Michael S. Tsirkin]

On Wed, Oct 16, 2013 at 10:49:14AM +0200, Igor Mammedov wrote:
> 'etc/pcimem64-minimum-address' will allow QEMU to communicate to BIOS
> where PCI memory address space mapping starts in high memory.
> 
> Allowing BIOS start mapping 64-bit PCI BARs at address where QEMU
> placed this mapping vs. hardcoded value right after highmem RAM.
> 
> That will allow QEMU to reserve extra address space before
> 64-bit PCI hole for memory hotplug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

So I don't like the name (seems to tell bios what to do instead of
describing hardware - it's really just the end of hot-pluggable RAM)
but it's not critical.
However I don't like that it touches pci-host devices.
Esp if my suggestion works out and we can remove the "pci hole"
concept from pci-host code, this interface also should be localized in
PC code.

> ---
>   * SeaBIOS patch: http://patchwork.ozlabs.org/patch/283623/
> 
> ---
>  hw/i386/pc.c         |    9 ++++++++-
>  hw/i386/pc_piix.c    |    2 +-
>  hw/pci-host/piix.c   |    5 +++--
>  hw/pci-host/q35.c    |    4 +++-
>  include/hw/i386/pc.h |    5 +++--
>  5 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7ecb028..ac99ae1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1055,7 +1055,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
>  
>  /* setup pci memory regions mappings into system address space */
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> -                            MemoryRegion *pci_address_space,
> +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
>                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
>                              uint32_t pci32_as_start, uint32_t pci32_as_size,
>                              uint64_t pci64_as_start, uint64_t pci64_as_size)
> @@ -1083,6 +1083,13 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
>          memory_region_add_subregion(system_memory,
>                                      ROUND_UP(pci64_as_start, 0x1ULL << 30),
>                                      pci64_as);
> +        if (fw_cfg) {
> +            uint64_t *pcimem64_start = g_malloc(sizeof(*pcimem64_start));
> +
> +            *pcimem64_start = cpu_to_le64(pci64_as_start);
> +            fw_cfg_add_file(fw_cfg, "etc/pcimem64-minimum-address",
> +                            pcimem64_start, sizeof(*pcimem64_start));
> +        }
>      }
>  }
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 2fed5fd..966c48a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -148,7 +148,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>                                system_memory, system_io,
>                                below_4g_mem_size,
>                                above_4g_mem_size,
> -                              pci_memory, ram_memory);
> +                              pci_memory, ram_memory, fw_cfg);
>      } else {
>          pci_bus = NULL;
>          i440fx_state = NULL;
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index d422b25..9347099 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -314,7 +314,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>                      hwaddr below_4g_mem_size,
>                      ram_addr_t above_4g_mem_size,
>                      MemoryRegion *pci_address_space,
> -                    MemoryRegion *ram_memory)
> +                    MemoryRegion *ram_memory,
> +                    FWCfgState *fw_cfg)
>  {
>      DeviceState *dev;
>      PCIBus *b;
> @@ -353,7 +354,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      }
>  
>      pc_pci_as_mapping_init(OBJECT(d), f->system_memory,
> -                           f->pci_address_space,
> +                           f->pci_address_space, fw_cfg,
>                             &f->pci_hole, &f->pci_hole_64bit,
>                             below_4g_mem_size,
>                             0x100000000ULL - below_4g_mem_size,
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index cb8aea0..583585c 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -30,6 +30,7 @@
>  #include "hw/hw.h"
>  #include "hw/pci-host/q35.h"
>  #include "qapi/visitor.h"
> +#include "hw/nvram/fw_cfg.h"
>  
>  /****************************************************************************
>   * Q35 host
> @@ -336,10 +337,11 @@ static int mch_init(PCIDevice *d)
>  {
>      int i;
>      MCHPCIState *mch = MCH_PCI_DEVICE(d);
> +    FWCfgState *fw_cfg = FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>  
>      /* setup pci memory regions */
>      pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
> -                           mch->pci_address_space,
> +                           mch->pci_address_space, fw_cfg,
>                             &mch->pci_hole, &mch->pci_hole_64bit,
>                             mch->below_4g_mem_size,
>                             0x100000000ULL - mch->below_4g_mem_size,
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ed86642..6d0d8c9 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -110,7 +110,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
>  
>  
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> -                            MemoryRegion *pci_address_space,
> +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
>                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
>                              uint32_t pci32_as_start, uint32_t pci32_as_size,
>                              uint64_t pci64_as_start, uint64_t pci64_as_size);
> @@ -164,7 +164,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>                      hwaddr below_4g_mem_size,
>                      ram_addr_t above_4g_mem_size,
>                      MemoryRegion *pci_memory,
> -                    MemoryRegion *ram_memory);
> +                    MemoryRegion *ram_memory,
> +                    FWCfgState *fw_cfg);
>  
>  /* piix4.c */
>  extern PCIDevice *piix4_dev;
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 4/4] pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

[Michael S. Tsirkin]

On Wed, Oct 16, 2013 at 10:49:14AM +0200, Igor Mammedov wrote:
> 'etc/pcimem64-minimum-address' will allow QEMU to communicate to BIOS
> where PCI memory address space mapping starts in high memory.
> 
> Allowing BIOS start mapping 64-bit PCI BARs at address where QEMU
> placed this mapping vs. hardcoded value right after highmem RAM.
> 
> That will allow QEMU to reserve extra address space before
> 64-bit PCI hole for memory hotplug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   * SeaBIOS patch: http://patchwork.ozlabs.org/patch/283623/
> 
> ---
>  hw/i386/pc.c         |    9 ++++++++-
>  hw/i386/pc_piix.c    |    2 +-
>  hw/pci-host/piix.c   |    5 +++--
>  hw/pci-host/q35.c    |    4 +++-
>  include/hw/i386/pc.h |    5 +++--
>  5 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7ecb028..ac99ae1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1055,7 +1055,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
>  
>  /* setup pci memory regions mappings into system address space */
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> -                            MemoryRegion *pci_address_space,
> +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
>                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
>                              uint32_t pci32_as_start, uint32_t pci32_as_size,
>                              uint64_t pci64_as_start, uint64_t pci64_as_size)
> @@ -1083,6 +1083,13 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
>          memory_region_add_subregion(system_memory,
>                                      ROUND_UP(pci64_as_start, 0x1ULL << 30),
>                                      pci64_as);
> +        if (fw_cfg) {
> +            uint64_t *pcimem64_start = g_malloc(sizeof(*pcimem64_start));
> +
> +            *pcimem64_start = cpu_to_le64(pci64_as_start);
> +            fw_cfg_add_file(fw_cfg, "etc/pcimem64-minimum-address",
> +                            pcimem64_start, sizeof(*pcimem64_start));
> +        }
>      }
>  }
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 2fed5fd..966c48a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -148,7 +148,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
>                                system_memory, system_io,
>                                below_4g_mem_size,
>                                above_4g_mem_size,
> -                              pci_memory, ram_memory);
> +                              pci_memory, ram_memory, fw_cfg);
>      } else {
>          pci_bus = NULL;
>          i440fx_state = NULL;
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index d422b25..9347099 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -314,7 +314,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>                      hwaddr below_4g_mem_size,
>                      ram_addr_t above_4g_mem_size,
>                      MemoryRegion *pci_address_space,
> -                    MemoryRegion *ram_memory)
> +                    MemoryRegion *ram_memory,
> +                    FWCfgState *fw_cfg)
>  {
>      DeviceState *dev;
>      PCIBus *b;
> @@ -353,7 +354,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      }
>  
>      pc_pci_as_mapping_init(OBJECT(d), f->system_memory,
> -                           f->pci_address_space,
> +                           f->pci_address_space, fw_cfg,
>                             &f->pci_hole, &f->pci_hole_64bit,
>                             below_4g_mem_size,
>                             0x100000000ULL - below_4g_mem_size,
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index cb8aea0..583585c 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -30,6 +30,7 @@
>  #include "hw/hw.h"
>  #include "hw/pci-host/q35.h"
>  #include "qapi/visitor.h"
> +#include "hw/nvram/fw_cfg.h"
>  
>  /****************************************************************************
>   * Q35 host
> @@ -336,10 +337,11 @@ static int mch_init(PCIDevice *d)
>  {
>      int i;
>      MCHPCIState *mch = MCH_PCI_DEVICE(d);
> +    FWCfgState *fw_cfg = FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));

Seems to duplicate code from fw_cfg_find().
Why not just use it?

>  
>      /* setup pci memory regions */
>      pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
> -                           mch->pci_address_space,
> +                           mch->pci_address_space, fw_cfg,
>                             &mch->pci_hole, &mch->pci_hole_64bit,
>                             mch->below_4g_mem_size,
>                             0x100000000ULL - mch->below_4g_mem_size,

Or call fw_cfg_find internally in pc_pci_as_mapping_init?

> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ed86642..6d0d8c9 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -110,7 +110,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
>  
>  
>  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> -                            MemoryRegion *pci_address_space,
> +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
>                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
>                              uint32_t pci32_as_start, uint32_t pci32_as_size,
>                              uint64_t pci64_as_start, uint64_t pci64_as_size);
> @@ -164,7 +164,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
>                      hwaddr below_4g_mem_size,
>                      ram_addr_t above_4g_mem_size,
>                      MemoryRegion *pci_memory,
> -                    MemoryRegion *ram_memory);
> +                    MemoryRegion *ram_memory,
> +                    FWCfgState *fw_cfg);
>  
>  /* piix4.c */
>  extern PCIDevice *piix4_dev;
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH 4/4] pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

[Igor Mammedov]

On Wed, 16 Oct 2013 12:29:48 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Oct 16, 2013 at 10:49:14AM +0200, Igor Mammedov wrote:
> > 'etc/pcimem64-minimum-address' will allow QEMU to communicate to BIOS
> > where PCI memory address space mapping starts in high memory.
> > 
> > Allowing BIOS start mapping 64-bit PCI BARs at address where QEMU
> > placed this mapping vs. hardcoded value right after highmem RAM.
> > 
> > That will allow QEMU to reserve extra address space before
> > 64-bit PCI hole for memory hotplug.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   * SeaBIOS patch: http://patchwork.ozlabs.org/patch/283623/
> > 
> > ---
> >  hw/i386/pc.c         |    9 ++++++++-
> >  hw/i386/pc_piix.c    |    2 +-
> >  hw/pci-host/piix.c   |    5 +++--
> >  hw/pci-host/q35.c    |    4 +++-
> >  include/hw/i386/pc.h |    5 +++--
> >  5 files changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7ecb028..ac99ae1 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1055,7 +1055,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> >  
> >  /* setup pci memory regions mappings into system address space */
> >  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> > -                            MemoryRegion *pci_address_space,
> > +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
> >                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
> >                              uint32_t pci32_as_start, uint32_t pci32_as_size,
> >                              uint64_t pci64_as_start, uint64_t pci64_as_size)
> > @@ -1083,6 +1083,13 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> >          memory_region_add_subregion(system_memory,
> >                                      ROUND_UP(pci64_as_start, 0x1ULL << 30),
> >                                      pci64_as);
> > +        if (fw_cfg) {
> > +            uint64_t *pcimem64_start = g_malloc(sizeof(*pcimem64_start));
> > +
> > +            *pcimem64_start = cpu_to_le64(pci64_as_start);
> > +            fw_cfg_add_file(fw_cfg, "etc/pcimem64-minimum-address",
> > +                            pcimem64_start, sizeof(*pcimem64_start));
> > +        }
> >      }
> >  }
> >  
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 2fed5fd..966c48a 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -148,7 +148,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
> >                                system_memory, system_io,
> >                                below_4g_mem_size,
> >                                above_4g_mem_size,
> > -                              pci_memory, ram_memory);
> > +                              pci_memory, ram_memory, fw_cfg);
> >      } else {
> >          pci_bus = NULL;
> >          i440fx_state = NULL;
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index d422b25..9347099 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -314,7 +314,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >                      hwaddr below_4g_mem_size,
> >                      ram_addr_t above_4g_mem_size,
> >                      MemoryRegion *pci_address_space,
> > -                    MemoryRegion *ram_memory)
> > +                    MemoryRegion *ram_memory,
> > +                    FWCfgState *fw_cfg)
> >  {
> >      DeviceState *dev;
> >      PCIBus *b;
> > @@ -353,7 +354,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >      }
> >  
> >      pc_pci_as_mapping_init(OBJECT(d), f->system_memory,
> > -                           f->pci_address_space,
> > +                           f->pci_address_space, fw_cfg,
> >                             &f->pci_hole, &f->pci_hole_64bit,
> >                             below_4g_mem_size,
> >                             0x100000000ULL - below_4g_mem_size,
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index cb8aea0..583585c 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -30,6 +30,7 @@
> >  #include "hw/hw.h"
> >  #include "hw/pci-host/q35.h"
> >  #include "qapi/visitor.h"
> > +#include "hw/nvram/fw_cfg.h"
> >  
> >  /****************************************************************************
> >   * Q35 host
> > @@ -336,10 +337,11 @@ static int mch_init(PCIDevice *d)
> >  {
> >      int i;
> >      MCHPCIState *mch = MCH_PCI_DEVICE(d);
> > +    FWCfgState *fw_cfg = FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> 
> Seems to duplicate code from fw_cfg_find().
> Why not just use it?
I've thought CAST(object_resolve_path()) is preferred method to do it,
But I can surely use fw_cfg_find().

> 
> >  
> >      /* setup pci memory regions */
> >      pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
> > -                           mch->pci_address_space,
> > +                           mch->pci_address_space, fw_cfg,
> >                             &mch->pci_hole, &mch->pci_hole_64bit,
> >                             mch->below_4g_mem_size,
> >                             0x100000000ULL - mch->below_4g_mem_size,
> 
> Or call fw_cfg_find internally in pc_pci_as_mapping_init?
I'll do it, thanks!

> 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index ed86642..6d0d8c9 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -110,7 +110,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> >  
> >  
> >  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> > -                            MemoryRegion *pci_address_space,
> > +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
> >                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
> >                              uint32_t pci32_as_start, uint32_t pci32_as_size,
> >                              uint64_t pci64_as_start, uint64_t pci64_as_size);
> > @@ -164,7 +164,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> >                      hwaddr below_4g_mem_size,
> >                      ram_addr_t above_4g_mem_size,
> >                      MemoryRegion *pci_memory,
> > -                    MemoryRegion *ram_memory);
> > +                    MemoryRegion *ram_memory,
> > +                    FWCfgState *fw_cfg);
> >  
> >  /* piix4.c */
> >  extern PCIDevice *piix4_dev;
> > -- 
> > 1.7.1

Re: [Qemu-devel] [PATCH 4/4] pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

[Michael S. Tsirkin]

On Wed, Oct 16, 2013 at 02:19:13PM +0200, Igor Mammedov wrote:
> On Wed, 16 Oct 2013 12:29:48 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Oct 16, 2013 at 10:49:14AM +0200, Igor Mammedov wrote:
> > > 'etc/pcimem64-minimum-address' will allow QEMU to communicate to BIOS
> > > where PCI memory address space mapping starts in high memory.
> > > 
> > > Allowing BIOS start mapping 64-bit PCI BARs at address where QEMU
> > > placed this mapping vs. hardcoded value right after highmem RAM.
> > > 
> > > That will allow QEMU to reserve extra address space before
> > > 64-bit PCI hole for memory hotplug.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >   * SeaBIOS patch: http://patchwork.ozlabs.org/patch/283623/
> > > 
> > > ---
> > >  hw/i386/pc.c         |    9 ++++++++-
> > >  hw/i386/pc_piix.c    |    2 +-
> > >  hw/pci-host/piix.c   |    5 +++--
> > >  hw/pci-host/q35.c    |    4 +++-
> > >  include/hw/i386/pc.h |    5 +++--
> > >  5 files changed, 18 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 7ecb028..ac99ae1 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1055,7 +1055,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > >  
> > >  /* setup pci memory regions mappings into system address space */
> > >  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> > > -                            MemoryRegion *pci_address_space,
> > > +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
> > >                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
> > >                              uint32_t pci32_as_start, uint32_t pci32_as_size,
> > >                              uint64_t pci64_as_start, uint64_t pci64_as_size)
> > > @@ -1083,6 +1083,13 @@ void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> > >          memory_region_add_subregion(system_memory,
> > >                                      ROUND_UP(pci64_as_start, 0x1ULL << 30),
> > >                                      pci64_as);
> > > +        if (fw_cfg) {
> > > +            uint64_t *pcimem64_start = g_malloc(sizeof(*pcimem64_start));
> > > +
> > > +            *pcimem64_start = cpu_to_le64(pci64_as_start);
> > > +            fw_cfg_add_file(fw_cfg, "etc/pcimem64-minimum-address",
> > > +                            pcimem64_start, sizeof(*pcimem64_start));
> > > +        }
> > >      }
> > >  }
> > >  
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index 2fed5fd..966c48a 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -148,7 +148,7 @@ static void pc_init1(QEMUMachineInitArgs *args,
> > >                                system_memory, system_io,
> > >                                below_4g_mem_size,
> > >                                above_4g_mem_size,
> > > -                              pci_memory, ram_memory);
> > > +                              pci_memory, ram_memory, fw_cfg);
> > >      } else {
> > >          pci_bus = NULL;
> > >          i440fx_state = NULL;
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index d422b25..9347099 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -314,7 +314,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > >                      hwaddr below_4g_mem_size,
> > >                      ram_addr_t above_4g_mem_size,
> > >                      MemoryRegion *pci_address_space,
> > > -                    MemoryRegion *ram_memory)
> > > +                    MemoryRegion *ram_memory,
> > > +                    FWCfgState *fw_cfg)
> > >  {
> > >      DeviceState *dev;
> > >      PCIBus *b;
> > > @@ -353,7 +354,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > >      }
> > >  
> > >      pc_pci_as_mapping_init(OBJECT(d), f->system_memory,
> > > -                           f->pci_address_space,
> > > +                           f->pci_address_space, fw_cfg,
> > >                             &f->pci_hole, &f->pci_hole_64bit,
> > >                             below_4g_mem_size,
> > >                             0x100000000ULL - below_4g_mem_size,
> > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > index cb8aea0..583585c 100644
> > > --- a/hw/pci-host/q35.c
> > > +++ b/hw/pci-host/q35.c
> > > @@ -30,6 +30,7 @@
> > >  #include "hw/hw.h"
> > >  #include "hw/pci-host/q35.h"
> > >  #include "qapi/visitor.h"
> > > +#include "hw/nvram/fw_cfg.h"
> > >  
> > >  /****************************************************************************
> > >   * Q35 host
> > > @@ -336,10 +337,11 @@ static int mch_init(PCIDevice *d)
> > >  {
> > >      int i;
> > >      MCHPCIState *mch = MCH_PCI_DEVICE(d);
> > > +    FWCfgState *fw_cfg = FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> > 
> > Seems to duplicate code from fw_cfg_find().
> > Why not just use it?
> I've thought CAST(object_resolve_path()) is preferred method to do it,
> But I can surely use fw_cfg_find().

I've no idea what's preferable, but duplicating code generally
is considered a bad idea in C.

> > 
> > >  
> > >      /* setup pci memory regions */
> > >      pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
> > > -                           mch->pci_address_space,
> > > +                           mch->pci_address_space, fw_cfg,
> > >                             &mch->pci_hole, &mch->pci_hole_64bit,
> > >                             mch->below_4g_mem_size,
> > >                             0x100000000ULL - mch->below_4g_mem_size,
> > 
> > Or call fw_cfg_find internally in pc_pci_as_mapping_init?
> I'll do it, thanks!
> 
> > 
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index ed86642..6d0d8c9 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -110,7 +110,7 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
> > >  
> > >  
> > >  void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
> > > -                            MemoryRegion *pci_address_space,
> > > +                            MemoryRegion *pci_address_space, FWCfgState *fw_cfg,
> > >                              MemoryRegion *pci32_as, MemoryRegion *pci64_as,
> > >                              uint32_t pci32_as_start, uint32_t pci32_as_size,
> > >                              uint64_t pci64_as_start, uint64_t pci64_as_size);
> > > @@ -164,7 +164,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
> > >                      hwaddr below_4g_mem_size,
> > >                      ram_addr_t above_4g_mem_size,
> > >                      MemoryRegion *pci_memory,
> > > -                    MemoryRegion *ram_memory);
> > > +                    MemoryRegion *ram_memory,
> > > +                    FWCfgState *fw_cfg);
> > >  
> > >  /* piix4.c */
> > >  extern PCIDevice *piix4_dev;
> > > -- 
> > > 1.7.1

Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Igor Mammedov]

On Wed, 16 Oct 2013 12:20:45 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Oct 16, 2013 at 10:49:10AM +0200, Igor Mammedov wrote:
> > * simplify PCI address space mapping into system address space,
> >   replacing code duplication in piix/q53 PCs with helper function
> 
> I think this does not go far enough.
> 
> I was always wondering about PCI hole in QEMU.
> Some real PCs have a "PCI hole" where PCI
> masks real memory, but PIIX does not do this,
> instead PCI is whenever RAM does not mask it.
> 
> So it looks like the hole concept is a left-over
> from when we didn't have priorities in the memory API.
> How about we remove them?
> See patch below.
> I did a quick boot test and it seems to work, of course
> it needs much more testing.
> It's on top of Marcel's series adding negative priorities,
> so works on top of the acpi branch or the pci branch.
I have done quite thorough testing and it works well except of
one caveat, it breaks migration due to different memory regions
layout.

So we'll have to keep an old aliasing scheme at least for old
machine types. Having that in mind do we still want to add
an extra implementation as you suggested?

> 
> I'm also wondering about the smram region - it uses
> priority 1 but does not say why.
> Need to check what does it overlap with, and why.
> 
> 
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index bad3953..988516a 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -102,8 +102,6 @@ struct PCII440FXState {
>      MemoryRegion *system_memory;
>      MemoryRegion *pci_address_space;
>      MemoryRegion *ram_memory;
> -    MemoryRegion pci_hole;
> -    MemoryRegion pci_hole_64bit;
>      PAMMemoryRegion pam_regions[13];
>      MemoryRegion smram_region;
>      uint8_t smm_enabled;
> @@ -326,7 +324,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      PCII440FXState *f;
>      unsigned i;
>      I440FXState *i440fx;
> -    uint64_t pci_hole64_size;
>  
>      dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
>      s = PCI_HOST_BRIDGE(dev);
> @@ -354,23 +351,9 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>          i440fx->pci_info.w32.begin = 0xe0000000;
>      }
>  
> -    memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> -                             pci_hole_start, pci_hole_size);
> -    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> -
> -    pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size);
> -
> -    pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size,
> -                       pci_hole64_size);
> -    memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> -                             f->pci_address_space,
> -                             i440fx->pci_info.w64.begin,
> -                             pci_hole64_size);
> -    if (pci_hole64_size) {
> -        memory_region_add_subregion(f->system_memory,
> -                                    i440fx->pci_info.w64.begin,
> -                                    &f->pci_hole_64bit);
> -    }
> +    /* Set to lower priority than RAM */
> +    memory_region_add_subregion_overlap(f->system_memory, 0x0,
> +                                        f->pci_address_space, -1);
>      memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
>                               f->pci_address_space, 0xa0000, 0x20000);
>      memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
> 

Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Michael S. Tsirkin]

On Tue, Oct 29, 2013 at 11:08:56AM +0100, Igor Mammedov wrote:
> On Wed, 16 Oct 2013 12:20:45 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Oct 16, 2013 at 10:49:10AM +0200, Igor Mammedov wrote:
> > > * simplify PCI address space mapping into system address space,
> > >   replacing code duplication in piix/q53 PCs with helper function
> > 
> > I think this does not go far enough.
> > 
> > I was always wondering about PCI hole in QEMU.
> > Some real PCs have a "PCI hole" where PCI
> > masks real memory, but PIIX does not do this,
> > instead PCI is whenever RAM does not mask it.
> > 
> > So it looks like the hole concept is a left-over
> > from when we didn't have priorities in the memory API.
> > How about we remove them?
> > See patch below.
> > I did a quick boot test and it seems to work, of course
> > it needs much more testing.
> > It's on top of Marcel's series adding negative priorities,
> > so works on top of the acpi branch or the pci branch.
> I have done quite thorough testing and it works well except of
> one caveat, it breaks migration due to different memory regions
> layout.

Interesting. We don't migrate PCI memory regions so what's going on?

> So we'll have to keep an old aliasing scheme at least for old
> machine types. Having that in mind do we still want to add
> an extra implementation as you suggested?
> 
> > 
> > I'm also wondering about the smram region - it uses
> > priority 1 but does not say why.
> > Need to check what does it overlap with, and why.
> > 
> > 
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index bad3953..988516a 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -102,8 +102,6 @@ struct PCII440FXState {
> >      MemoryRegion *system_memory;
> >      MemoryRegion *pci_address_space;
> >      MemoryRegion *ram_memory;
> > -    MemoryRegion pci_hole;
> > -    MemoryRegion pci_hole_64bit;
> >      PAMMemoryRegion pam_regions[13];
> >      MemoryRegion smram_region;
> >      uint8_t smm_enabled;
> > @@ -326,7 +324,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >      PCII440FXState *f;
> >      unsigned i;
> >      I440FXState *i440fx;
> > -    uint64_t pci_hole64_size;
> >  
> >      dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> >      s = PCI_HOST_BRIDGE(dev);
> > @@ -354,23 +351,9 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >          i440fx->pci_info.w32.begin = 0xe0000000;
> >      }
> >  
> > -    memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > -                             pci_hole_start, pci_hole_size);
> > -    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > -
> > -    pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size);
> > -
> > -    pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size,
> > -                       pci_hole64_size);
> > -    memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > -                             f->pci_address_space,
> > -                             i440fx->pci_info.w64.begin,
> > -                             pci_hole64_size);
> > -    if (pci_hole64_size) {
> > -        memory_region_add_subregion(f->system_memory,
> > -                                    i440fx->pci_info.w64.begin,
> > -                                    &f->pci_hole_64bit);
> > -    }
> > +    /* Set to lower priority than RAM */
> > +    memory_region_add_subregion_overlap(f->system_memory, 0x0,
> > +                                        f->pci_address_space, -1);
> >      memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> >                               f->pci_address_space, 0xa0000, 0x20000);
> >      memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
> > 

Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Igor Mammedov]

On Tue, 29 Oct 2013 12:22:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 29, 2013 at 11:08:56AM +0100, Igor Mammedov wrote:
> > On Wed, 16 Oct 2013 12:20:45 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Oct 16, 2013 at 10:49:10AM +0200, Igor Mammedov wrote:
> > > > * simplify PCI address space mapping into system address space,
> > > >   replacing code duplication in piix/q53 PCs with helper function
> > > 
> > > I think this does not go far enough.
> > > 
> > > I was always wondering about PCI hole in QEMU.
> > > Some real PCs have a "PCI hole" where PCI
> > > masks real memory, but PIIX does not do this,
> > > instead PCI is whenever RAM does not mask it.
> > > 
> > > So it looks like the hole concept is a left-over
> > > from when we didn't have priorities in the memory API.
> > > How about we remove them?
> > > See patch below.
> > > I did a quick boot test and it seems to work, of course
> > > it needs much more testing.
> > > It's on top of Marcel's series adding negative priorities,
> > > so works on top of the acpi branch or the pci branch.
> > I have done quite thorough testing and it works well except of
> > one caveat, it breaks migration due to different memory regions
> > layout.
> 
> Interesting. We don't migrate PCI memory regions so what's going on?

I'm sorry for noise, it was a false alarm due to wrong testing
configuration. It was failing with:
"
qemu-system-x86_64: pci_add_option_rom: failed to find romfile "efi-e1000.rom"
Unknown ramblock "0000:03.0/e1000.rom", cannot accept migration
"
Caused by incorrect -L option. It works fine when correct BIOS path is
provided.


One more questions about whether we should disable "etc/pci-info" fw_cfg
for 1.7 again since Seabios doesn't use it (and it looks like it's not
going to do so)?

> 
> > So we'll have to keep an old aliasing scheme at least for old
> > machine types. Having that in mind do we still want to add
> > an extra implementation as you suggested?
> > 
> > > 
> > > I'm also wondering about the smram region - it uses
> > > priority 1 but does not say why.
> > > Need to check what does it overlap with, and why.
> > > 
> > > 
> > > 
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index bad3953..988516a 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -102,8 +102,6 @@ struct PCII440FXState {
> > >      MemoryRegion *system_memory;
> > >      MemoryRegion *pci_address_space;
> > >      MemoryRegion *ram_memory;
> > > -    MemoryRegion pci_hole;
> > > -    MemoryRegion pci_hole_64bit;
> > >      PAMMemoryRegion pam_regions[13];
> > >      MemoryRegion smram_region;
> > >      uint8_t smm_enabled;
> > > @@ -326,7 +324,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > >      PCII440FXState *f;
> > >      unsigned i;
> > >      I440FXState *i440fx;
> > > -    uint64_t pci_hole64_size;
> > >  
> > >      dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE);
> > >      s = PCI_HOST_BRIDGE(dev);
> > > @@ -354,23 +351,9 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> > >          i440fx->pci_info.w32.begin = 0xe0000000;
> > >      }
> > >  
> > > -    memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
> > > -                             pci_hole_start, pci_hole_size);
> > > -    memory_region_add_subregion(f->system_memory, pci_hole_start, &f->pci_hole);
> > > -
> > > -    pci_hole64_size = pci_host_get_hole64_size(i440fx->pci_hole64_size);
> > > -
> > > -    pc_init_pci64_hole(&i440fx->pci_info, 0x100000000ULL + above_4g_mem_size,
> > > -                       pci_hole64_size);
> > > -    memory_region_init_alias(&f->pci_hole_64bit, OBJECT(d), "pci-hole64",
> > > -                             f->pci_address_space,
> > > -                             i440fx->pci_info.w64.begin,
> > > -                             pci_hole64_size);
> > > -    if (pci_hole64_size) {
> > > -        memory_region_add_subregion(f->system_memory,
> > > -                                    i440fx->pci_info.w64.begin,
> > > -                                    &f->pci_hole_64bit);
> > > -    }
> > > +    /* Set to lower priority than RAM */
> > > +    memory_region_add_subregion_overlap(f->system_memory, 0x0,
> > > +                                        f->pci_address_space, -1);
> > >      memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
> > >                               f->pci_address_space, 0xa0000, 0x20000);
> > >      memory_region_add_subregion_overlap(f->system_memory, 0xa0000,
> > > 
> 

Re: [Qemu-devel] [PATCH 0/4] pc: inform SeaBIOS where 64-bit PCI hole start and PCI mappings cleanup

[Michael S. Tsirkin]

On Tue, Oct 29, 2013 at 11:08:56AM +0100, Igor Mammedov wrote:
> On Wed, 16 Oct 2013 12:20:45 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Oct 16, 2013 at 10:49:10AM +0200, Igor Mammedov wrote:
> > > * simplify PCI address space mapping into system address space,
> > >   replacing code duplication in piix/q53 PCs with helper function
> > 
> > I think this does not go far enough.
> > 
> > I was always wondering about PCI hole in QEMU.
> > Some real PCs have a "PCI hole" where PCI
> > masks real memory, but PIIX does not do this,
> > instead PCI is whenever RAM does not mask it.
> > 
> > So it looks like the hole concept is a left-over
> > from when we didn't have priorities in the memory API.
> > How about we remove them?
> > See patch below.
> > I did a quick boot test and it seems to work, of course
> > it needs much more testing.
> > It's on top of Marcel's series adding negative priorities,
> > so works on top of the acpi branch or the pci branch.
> I have done quite thorough testing and it works well except of
> one caveat, it breaks migration due to different memory regions
> layout.
> 
> So we'll have to keep an old aliasing scheme at least for old
> machine types. Having that in mind do we still want to add
> an extra implementation as you suggested?

Sorry I didn't answer this question.
I think it's a bug - PCI hole should not affect migration.