[Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Igor Mammedov]

* simplify PCI address space mapping into system address space,
  replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
  SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/

v2:
 *  use negative priority to map PCI address space under RAM memory
    regions which allows simplify code by removing pci_hole &
    pci_hole64 memory region aliases

Series depends on:
 "memory: Change MemoryRegion priorities from unsigned to signed:

Git tree for testing:
  https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2

Igor Mammedov (1):
  pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS

Michael S. Tsirkin (1):
  pc: map PCI address space as catchall region for not mapped addresses

 hw/i386/pc.c              |   28 ++++++++++++++++------------
 hw/i386/pc_piix.c         |    2 --
 hw/pci-host/piix.c        |   27 +++++----------------------
 hw/pci-host/q35.c         |   28 ++++++----------------------
 include/hw/i386/pc.h      |   15 +++------------
 include/hw/pci-host/q35.h |    2 --
 6 files changed, 30 insertions(+), 72 deletions(-)

[Qemu-devel] [PATCH 1/2] pc: map PCI address space as catchall region for not mapped addresses

[Igor Mammedov]

From: Michael S. Tsirkin <mst@redhat.com>

With a help of negative memory region priority PCI address space
is mapped underneath RAM regions effectively catching every access
to addresses not mapped by any other region.
It simplifies PCI address space mapping into system address space.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c              |   20 ++++++--------------
 hw/i386/pc_piix.c         |    2 --
 hw/pci-host/piix.c        |   26 ++++----------------------
 hw/pci-host/q35.c         |   27 +++++----------------------
 include/hw/i386/pc.h      |   14 ++------------
 include/hw/pci-host/q35.h |    2 --
 6 files changed, 17 insertions(+), 74 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0c313fe..340c696 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1053,21 +1053,13 @@ 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 address space mapping into system address space */
+void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
+                            MemoryRegion *pci_address_space)
 {
-    if ((sizeof(hwaddr) == 4) || (!pci_hole64_size)) {
-        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);
+    /* Set to lower priority than RAM */
+    memory_region_add_subregion_overlap(system_memory, 0x0,
+                                        pci_address_space, -1);
 }
 
 void pc_acpi_init(const char *default_dsdt)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c6042c7..73053cb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -146,8 +146,6 @@ 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,
-                              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..d34d20b 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;
@@ -312,8 +310,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
                     ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_address_space,
                     MemoryRegion *ram_memory)
@@ -326,7 +322,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 +349,10 @@ 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);
-    }
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
+                           f->pci_address_space);
+
     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..3e82caf 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -336,28 +336,11 @@ 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);
-    }
+
+    /* setup pci memory mapping */
+    pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
+                           mch->pci_address_space);
+
     /* 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 6083839..de5a347 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -108,17 +108,9 @@ 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);
 
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
@@ -167,8 +159,6 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
                     ram_addr_t ram_size,
-                    hwaddr pci_hole_start,
-                    hwaddr pci_hole_size,
                     ram_addr_t above_4g_mem_size,
                     MemoryRegion *pci_memory,
                     MemoryRegion *ram_memory);
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 56de92e..fd53ff9 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -53,8 +53,6 @@ typedef struct MCHPCIState {
     MemoryRegion *address_space_io;
     PAMMemoryRegion pam_regions[13];
     MemoryRegion smram_region;
-    MemoryRegion pci_hole;
-    MemoryRegion pci_hole_64bit;
     PcPciInfo pci_info;
     uint8_t smm_enabled;
     ram_addr_t below_4g_mem_size;
-- 
1.7.1

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

[Igor Mammedov]

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

Allowing BIOS to start mapping 64-bit PCI BARs at address where it
wouldn't conflict with other mappings QEMU might place before it.

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

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

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 340c696..53d07a4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1055,11 +1055,23 @@ PcGuestInfo *pc_guest_info_init(ram_addr_t below_4g_mem_size,
 
 /* setup pci memory address space mapping into system address space */
 void pc_pci_as_mapping_init(Object *owner, MemoryRegion *system_memory,
-                            MemoryRegion *pci_address_space)
+                            MemoryRegion *pci_address_space,
+                            uint64_t pcimem64_min_addr)
 {
+    uint64_t *val;
+    FWCfgState *fw_cfg = fw_cfg_find();
+
     /* Set to lower priority than RAM */
     memory_region_add_subregion_overlap(system_memory, 0x0,
                                         pci_address_space, -1);
+    g_assert(fw_cfg);
+    /*
+     *  Align address at 1G, this makes sure it can be exactly covered
+     *  with a PAT entry even when using huge pages.
+     */
+    val = g_malloc(sizeof(*val));
+    *val = cpu_to_le64(ROUND_UP(pcimem64_min_addr, 0x1ULL << 30));
+    fw_cfg_add_file(fw_cfg, "etc/pcimem64-minimum-address", val, sizeof(*val));
 }
 
 void pc_acpi_init(const char *default_dsdt)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index d34d20b..fcebe9a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -351,7 +351,8 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
 
     /* setup pci memory mapping */
     pc_pci_as_mapping_init(OBJECT(f), f->system_memory,
-                           f->pci_address_space);
+                           f->pci_address_space,
+                           0x100000000ULL + above_4g_mem_size);
 
     memory_region_init_alias(&f->smram_region, OBJECT(d), "smram-region",
                              f->pci_address_space, 0xa0000, 0x20000);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 3e82caf..0f3aabc 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -339,7 +339,8 @@ static int mch_init(PCIDevice *d)
 
     /* setup pci memory mapping */
     pc_pci_as_mapping_init(OBJECT(mch), mch->system_memory,
-                           mch->pci_address_space);
+                           mch->pci_address_space,
+                           0x100000000ULL + mch->above_4g_mem_size);
 
     /* smram */
     cpu_smm_register(&mch_set_smm, mch);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index de5a347..9b9bcf0 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -110,7 +110,8 @@ 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,
+                            uint64_t pcimem64_min_addr);
 
 FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                            const char *kernel_filename,
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Michael S. Tsirkin]

On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> * simplify PCI address space mapping into system address space,
>   replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
>   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/

I'd like to see if we can figure out the migration issue with
memory layout. Because if we do, and get rid of the separate 64 bit
region as a concept, exposing the start of this non-existent
region in FW CFG will make very little sense IMHO.

> v2:
>  *  use negative priority to map PCI address space under RAM memory
>     regions which allows simplify code by removing pci_hole &
>     pci_hole64 memory region aliases
> 
> Series depends on:
>  "memory: Change MemoryRegion priorities from unsigned to signed:
> 
> Git tree for testing:
>   https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2
> 
> Igor Mammedov (1):
>   pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS
> 
> Michael S. Tsirkin (1):
>   pc: map PCI address space as catchall region for not mapped addresses
> 
>  hw/i386/pc.c              |   28 ++++++++++++++++------------
>  hw/i386/pc_piix.c         |    2 --
>  hw/pci-host/piix.c        |   27 +++++----------------------
>  hw/pci-host/q35.c         |   28 ++++++----------------------
>  include/hw/i386/pc.h      |   15 +++------------
>  include/hw/pci-host/q35.h |    2 --
>  6 files changed, 30 insertions(+), 72 deletions(-)

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Igor Mammedov]

On Tue, 29 Oct 2013 17:10:47 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> > * simplify PCI address space mapping into system address space,
> >   replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
> >   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/
> 
> I'd like to see if we can figure out the migration issue with
> memory layout.
It seems that there isn't migration issue here.

> Because if we do, and get rid of the separate 64 bit
> region as a concept, exposing the start of this non-existent
> region in FW CFG will make very little sense IMHO.
Well, BIOS have to know where it could start 64-bit BARs mappings and
telling it explicitly where, looks like a good way to do it.

> 
> > v2:
> >  *  use negative priority to map PCI address space under RAM memory
> >     regions which allows simplify code by removing pci_hole &
> >     pci_hole64 memory region aliases
> > 
> > Series depends on:
> >  "memory: Change MemoryRegion priorities from unsigned to signed:
> > 
> > Git tree for testing:
> >   https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2
> > 
> > Igor Mammedov (1):
> >   pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS
> > 
> > Michael S. Tsirkin (1):
> >   pc: map PCI address space as catchall region for not mapped addresses
> > 
> >  hw/i386/pc.c              |   28 ++++++++++++++++------------
> >  hw/i386/pc_piix.c         |    2 --
> >  hw/pci-host/piix.c        |   27 +++++----------------------
> >  hw/pci-host/q35.c         |   28 ++++++----------------------
> >  include/hw/i386/pc.h      |   15 +++------------
> >  include/hw/pci-host/q35.h |    2 --
> >  6 files changed, 30 insertions(+), 72 deletions(-)

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Michael S. Tsirkin]

On Tue, Oct 29, 2013 at 04:28:25PM +0100, Igor Mammedov wrote:
> On Tue, 29 Oct 2013 17:10:47 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> > > * simplify PCI address space mapping into system address space,
> > >   replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
> > >   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/
> > 
> > I'd like to see if we can figure out the migration issue with
> > memory layout.
> It seems that there isn't migration issue here.

Hmm earlier you thought there was - it's ok now?

> > Because if we do, and get rid of the separate 64 bit
> > region as a concept, exposing the start of this non-existent
> > region in FW CFG will make very little sense IMHO.
> Well, BIOS have to know where it could start 64-bit BARs mappings
> and
> telling it explicitly where, looks like a good way to do it.

As far as I can tell, BIOS can start any mappings anywhere it wants to
as long as they don't overlap anything else.
What is has to know is what hardware is there.


> > 
> > > v2:
> > >  *  use negative priority to map PCI address space under RAM memory
> > >     regions which allows simplify code by removing pci_hole &
> > >     pci_hole64 memory region aliases
> > > 
> > > Series depends on:
> > >  "memory: Change MemoryRegion priorities from unsigned to signed:
> > > 
> > > Git tree for testing:
> > >   https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2
> > > 
> > > Igor Mammedov (1):
> > >   pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS
> > > 
> > > Michael S. Tsirkin (1):
> > >   pc: map PCI address space as catchall region for not mapped addresses
> > > 
> > >  hw/i386/pc.c              |   28 ++++++++++++++++------------
> > >  hw/i386/pc_piix.c         |    2 --
> > >  hw/pci-host/piix.c        |   27 +++++----------------------
> > >  hw/pci-host/q35.c         |   28 ++++++----------------------
> > >  include/hw/i386/pc.h      |   15 +++------------
> > >  include/hw/pci-host/q35.h |    2 --
> > >  6 files changed, 30 insertions(+), 72 deletions(-)

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Gerd Hoffmann]

  Hi,

> > Well, BIOS have to know where it could start 64-bit BARs mappings
> > and
> > telling it explicitly where, looks like a good way to do it.
> 
> As far as I can tell, BIOS can start any mappings anywhere it wants to
> as long as they don't overlap anything else.
> What is has to know is what hardware is there.

Use case is memory hotplug.  Once we generate the acpi tables in qemu
seabios doesn't need to know anything about hotpluggable memory slots.
Still it better should not map 64bit pci bars into that address space.
So it IMHO makes sense to give seabios a hit where it should map the
64bit pci bars.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Igor Mammedov]

On Tue, 29 Oct 2013 20:52:42 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 29, 2013 at 04:28:25PM +0100, Igor Mammedov wrote:
> > On Tue, 29 Oct 2013 17:10:47 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> > > > * simplify PCI address space mapping into system address space,
> > > >   replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
> > > >   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/
> > > 
> > > I'd like to see if we can figure out the migration issue with
> > > memory layout.
> > It seems that there isn't migration issue here.
> 
> Hmm earlier you thought there was - it's ok now?
> 
> > > Because if we do, and get rid of the separate 64 bit
> > > region as a concept, exposing the start of this non-existent
> > > region in FW CFG will make very little sense IMHO.
> > Well, BIOS have to know where it could start 64-bit BARs mappings
> > and
> > telling it explicitly where, looks like a good way to do it.
> 
> As far as I can tell, BIOS can start any mappings anywhere it wants to
> as long as they don't overlap anything else.
> What is has to know is what hardware is there.
lets suppose we are describing HW to Seabios (about which it doesn't really
needs to know when we move ACPI tables into QEMU)
 1. we inform Seabios where memory hotplug region ends (describing new hw)
    and name parameter "etc/mem-hoplug-region-end" and make appropriate change
    to Seabois so it would know about memory hotlug region.
 2. than in several releases we decide to add another device that would
    reserve address space after memory hotplug region. That would require
    another modification of Seabios to teach it about new hardware so it
    could place 64-bit PCI mappings after it.
So describing a new hardware each time will only increase amount of PV
interfaces overtime, breaking old BIOSes and forcing us to maintain
compatibility layers for old versions in Seabios and QEMU.

Opposite approach in this series tries to minimize all above mentioned
problems by providing Seabios with an explicit information that it needs
to make correct 64-bit PCI BARs mappings and it won't introduce
compatibility issues with Seabios if we reserve extra space behind memory
hotlpug region in future.

> 
> 
> > > 
> > > > v2:
> > > >  *  use negative priority to map PCI address space under RAM memory
> > > >     regions which allows simplify code by removing pci_hole &
> > > >     pci_hole64 memory region aliases
> > > > 
> > > > Series depends on:
> > > >  "memory: Change MemoryRegion priorities from unsigned to signed:
> > > > 
> > > > Git tree for testing:
> > > >   https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2
> > > > 
> > > > Igor Mammedov (1):
> > > >   pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS
> > > > 
> > > > Michael S. Tsirkin (1):
> > > >   pc: map PCI address space as catchall region for not mapped addresses
> > > > 
> > > >  hw/i386/pc.c              |   28 ++++++++++++++++------------
> > > >  hw/i386/pc_piix.c         |    2 --
> > > >  hw/pci-host/piix.c        |   27 +++++----------------------
> > > >  hw/pci-host/q35.c         |   28 ++++++----------------------
> > > >  include/hw/i386/pc.h      |   15 +++------------
> > > >  include/hw/pci-host/q35.h |    2 --
> > > >  6 files changed, 30 insertions(+), 72 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Igor Mammedov]

On Tue, 29 Oct 2013 20:52:42 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 29, 2013 at 04:28:25PM +0100, Igor Mammedov wrote:
> > On Tue, 29 Oct 2013 17:10:47 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> > > > * simplify PCI address space mapping into system address space,
> > > >   replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
> > > >   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/
> > > 
> > > I'd like to see if we can figure out the migration issue with
> > > memory layout.
> > It seems that there isn't migration issue here.
> 
> Hmm earlier you thought there was - it's ok now?
You've probably missed my reply
http://www.mail-archive.com/qemu-devel@nongnu.org/msg199586.html

it was error it test environment.

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 02:24:54PM +0100, Igor Mammedov wrote:
> On Tue, 29 Oct 2013 20:52:42 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Oct 29, 2013 at 04:28:25PM +0100, Igor Mammedov wrote:
> > > On Tue, 29 Oct 2013 17:10:47 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> > > > > * simplify PCI address space mapping into system address space,
> > > > >   replacing code duplication in piix/q53 PCs with a 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 reserving memory hotplug region in highmem.
> > > > >   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/
> > > > 
> > > > I'd like to see if we can figure out the migration issue with
> > > > memory layout.
> > > It seems that there isn't migration issue here.
> > 
> > Hmm earlier you thought there was - it's ok now?
> > 
> > > > Because if we do, and get rid of the separate 64 bit
> > > > region as a concept, exposing the start of this non-existent
> > > > region in FW CFG will make very little sense IMHO.
> > > Well, BIOS have to know where it could start 64-bit BARs mappings
> > > and
> > > telling it explicitly where, looks like a good way to do it.
> > 
> > As far as I can tell, BIOS can start any mappings anywhere it wants to
> > as long as they don't overlap anything else.
> > What is has to know is what hardware is there.
> lets suppose we are describing HW to Seabios (about which it doesn't really
> needs to know when we move ACPI tables into QEMU)
>  1. we inform Seabios where memory hotplug region ends (describing new hw)
>     and name parameter "etc/mem-hoplug-region-end" and make appropriate change
>     to Seabois so it would know about memory hotlug region.
>  2. than in several releases we decide to add another device that would
>     reserve address space after memory hotplug region. That would require
>     another modification of Seabios to teach it about new hardware so it
>     could place 64-bit PCI mappings after it.
> So describing a new hardware each time will only increase amount of PV
> interfaces overtime, breaking old BIOSes and forcing us to maintain
> compatibility layers for old versions in Seabios and QEMU.

I don't think we can predict the future.
It seems just as likely that BIOS will need to drive the
new hardware so it will not be enough to have "start pci here".
In particular, non-contiguous hotpluggable memory does not seem all that
far-fetched, and we might want to put PCI devices in the holes
that are left free.

Maybe, if you want it make it kind of generic you can just say
"reserved-memory-end" with the same value.

> 
> Opposite approach in this series tries to minimize all above mentioned
> problems by providing Seabios with an explicit information that it needs
> to make correct 64-bit PCI BARs mappings and it won't introduce
> compatibility issues with Seabios if we reserve extra space behind memory
> hotlpug region in future.

Point is that the value you expose has nothing to do
with PCI. Yes BIOS might use it for that but interfaces
need to tell what they are, not what they are used for.

> > 
> > 
> > > > 
> > > > > v2:
> > > > >  *  use negative priority to map PCI address space under RAM memory
> > > > >     regions which allows simplify code by removing pci_hole &
> > > > >     pci_hole64 memory region aliases
> > > > > 
> > > > > Series depends on:
> > > > >  "memory: Change MemoryRegion priorities from unsigned to signed:
> > > > > 
> > > > > Git tree for testing:
> > > > >   https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2
> > > > > 
> > > > > Igor Mammedov (1):
> > > > >   pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS
> > > > > 
> > > > > Michael S. Tsirkin (1):
> > > > >   pc: map PCI address space as catchall region for not mapped addresses
> > > > > 
> > > > >  hw/i386/pc.c              |   28 ++++++++++++++++------------
> > > > >  hw/i386/pc_piix.c         |    2 --
> > > > >  hw/pci-host/piix.c        |   27 +++++----------------------
> > > > >  hw/pci-host/q35.c         |   28 ++++++----------------------
> > > > >  include/hw/i386/pc.h      |   15 +++------------
> > > > >  include/hw/pci-host/q35.h |    2 --
> > > > >  6 files changed, 30 insertions(+), 72 deletions(-)
> > 

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Gerd Hoffmann]

  Hi,

> I don't think we can predict the future.
> It seems just as likely that BIOS will need to drive the
> new hardware so it will not be enough to have "start pci here".
> In particular, non-contiguous hotpluggable memory does not seem all that
> far-fetched, and we might want to put PCI devices in the holes
> that are left free.
> 
> Maybe, if you want it make it kind of generic you can just say
> "reserved-memory-end" with the same value.

What is the plan for handling e820 with memory hotplug btw?

You've mentioned e820 entries for hotpluggable dimms present at boot are
needed for some guests.

I'm about to add a new e820 fw_cfg interface (see
http://comments.gmane.org/gmane.comp.emulators.qemu/238860).

So maybe design that with memory hotplug in mind?  Such as adding a new
qemu-specific type QEMU_RAM_HOTPLUG?  Which seabios could use to reserve
the memory (but not add it to the e820 table for the guest)?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Igor Mammedov]

On Wed, 30 Oct 2013 15:33:35 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > I don't think we can predict the future.
> > It seems just as likely that BIOS will need to drive the
> > new hardware so it will not be enough to have "start pci here".
> > In particular, non-contiguous hotpluggable memory does not seem all that
> > far-fetched, and we might want to put PCI devices in the holes
> > that are left free.
> > 
> > Maybe, if you want it make it kind of generic you can just say
> > "reserved-memory-end" with the same value.
> 
> What is the plan for handling e820 with memory hotplug btw?
There wasn't immediate plans for using e820 for hotplugged memory yet.

> You've mentioned e820 entries for hotpluggable dimms present at boot are
> needed for some guests.
Windows guest are fine and do not require e820 entries for present
hotplugged memory, automatically onlining memory devices described in ACPI.
Linux guests see memory devices described in ACPI but do not online them
automatically.

I'm not sure how valid would be putting present hotplugged memory into
e820 which is supposed to be static.
It would be better if linux guests behave as windows, it could online
memory regions in userspace (using udev) or fix kernel to do it at boot
time after scanning ACPI devices.
 
> I'm about to add a new e820 fw_cfg interface (see
> http://comments.gmane.org/gmane.comp.emulators.qemu/238860).
> 
> So maybe design that with memory hotplug in mind?  Such as adding a new
> qemu-specific type QEMU_RAM_HOTPLUG?  Which seabios could use to reserve
> the memory (but not add it to the e820 table for the guest)?
It will do job too. But extending semantics of standard table would be
confusing. Yes, Seabios will filter it out but it doesn't make table
less confusing.
I'd prefer having a dedicated interface for it as a more clean solution.

> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Gerd Hoffmann]

  Hi,

> > So maybe design that with memory hotplug in mind?  Such as adding a new
> > qemu-specific type QEMU_RAM_HOTPLUG?  Which seabios could use to reserve
> > the memory (but not add it to the e820 table for the guest)?
> It will do job too. But extending semantics of standard table would be
> confusing. Yes, Seabios will filter it out but it doesn't make table
> less confusing.

Was just thinking that it might be easier that way if we need e820
entries for hotplug memory address space _anyway_.

> I'd prefer having a dedicated interface for it as a more clean solution.

Agree.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Igor Mammedov]

On Mon, 04 Nov 2013 13:48:03 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > So maybe design that with memory hotplug in mind?  Such as adding a new
> > > qemu-specific type QEMU_RAM_HOTPLUG?  Which seabios could use to reserve
> > > the memory (but not add it to the e820 table for the guest)?
> > It will do job too. But extending semantics of standard table would be
> > confusing. Yes, Seabios will filter it out but it doesn't make table
> > less confusing.
> 
> Was just thinking that it might be easier that way if we need e820
> entries for hotplug memory address space _anyway_.
I don't think that we need e820 entries for hotplug memory reserved space as
e820 should. In case present at boot hotpluggable DIMMs would be needed in E820,
we can add them as usual E820_RAM entries.

But regardless of what we do here it might be good keep option of adding non
standard entries in future, by filtering out unknown types in SeaBIOS.

> 
> > I'd prefer having a dedicated interface for it as a more clean solution.
> 
> Agree.

So back to naming question, would you agree to renaming fw_cfg to the last
Michael's suggestion "reserved-memory-end"?
 
> cheers,
>   Gerd
> 
> 
> 

Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Gerd Hoffmann]

On Mo, 2013-11-04 at 15:35 +0100, Igor Mammedov wrote:
> On Mon, 04 Nov 2013 13:48:03 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> > 
> > > > So maybe design that with memory hotplug in mind?  Such as adding a new
> > > > qemu-specific type QEMU_RAM_HOTPLUG?  Which seabios could use to reserve
> > > > the memory (but not add it to the e820 table for the guest)?
> > > It will do job too. But extending semantics of standard table would be
> > > confusing. Yes, Seabios will filter it out but it doesn't make table
> > > less confusing.
> > 
> > Was just thinking that it might be easier that way if we need e820
> > entries for hotplug memory address space _anyway_.
> I don't think that we need e820 entries for hotplug memory reserved space as
> e820 should. In case present at boot hotpluggable DIMMs would be needed in E820,
> we can add them as usual E820_RAM entries.
> 
> But regardless of what we do here it might be good keep option of adding non
> standard entries in future, by filtering out unknown types in SeaBIOS.
> 
> > 
> > > I'd prefer having a dedicated interface for it as a more clean solution.
> > 
> > Agree.
> 
> So back to naming question, would you agree to renaming fw_cfg to the last
> Michael's suggestion "reserved-memory-end"?

Fine with me.

cheers,
  Gerd