[Qemu-devel] [PATCH 0/2] pc: fix 64-bit PCI window clashing with memory hotplug region

[Qemu-devel] [PATCH 0/2] pc: fix 64-bit PCI window clashing with memory hotplug region

[Igor Mammedov]

Igor Mammedov (2):
  pc: memhotplug: fix incorrectly set reserved-memory-end
  pc: memhotplug: keep reserved-memory-end broken on 2.4 and earlier
    machines

 hw/i386/pc.c         | 8 +++++++-
 hw/i386/pc_piix.c    | 2 ++
 hw/i386/pc_q35.c     | 2 ++
 include/hw/i386/pc.h | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] pc: memhotplug: fix incorrectly set reserved-memory-end

[Igor Mammedov]

reserved-memory-end tells firmware address from which
it could start treating memory as PCI address space
and map PCI BARs after it to avoid collisions with
RAM.
Currently it is incorrectly pointing to address where
hotplugged memory range starts which could redirect
hotplugged RAM accesses to PCI BARs when firmware
maps them over RAM or viceverse.
Fix this by pointing reserved-memory-end to the end
of memory hotplug area.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9f2924e..354e1b3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1412,7 +1412,9 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
         uint64_t *val = g_malloc(sizeof(*val));
-        *val = cpu_to_le64(ROUND_UP(pcms->hotplug_memory.base, 0x1ULL << 30));
+        uint64_t res_mem_end = pcms->hotplug_memory.base +
+                               memory_region_size(&pcms->hotplug_memory.mr);
+        *val = cpu_to_le64(ROUND_UP(res_mem_end, 0x1ULL << 30));
         fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] pc: memhotplug: keep reserved-memory-end broken on 2.4 and earlier machines

[Igor Mammedov]

it will prevent guests on old machines from seeing
inconsistent memory mapping in firmware/ACPI views.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS: this patch probably clashes with Eduardo's
    pc_compat refactoring, so I've split it from
    main fix, if applied via Eduardo's it could
    be squashed in main fix after merging on top
    of his patches or as is if goes in first.
---
 hw/i386/pc.c         | 8 ++++++--
 hw/i386/pc_piix.c    | 2 ++
 hw/i386/pc_q35.c     | 2 ++
 include/hw/i386/pc.h | 1 +
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 354e1b3..b5107f7 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1412,8 +1412,12 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
 
     if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
         uint64_t *val = g_malloc(sizeof(*val));
-        uint64_t res_mem_end = pcms->hotplug_memory.base +
-                               memory_region_size(&pcms->hotplug_memory.mr);
+        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+        uint64_t res_mem_end = pcms->hotplug_memory.base;
+
+        if (!pcmc->broken_reserved_end) {
+            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
+        }
         *val = cpu_to_le64(ROUND_UP(res_mem_end, 0x1ULL << 30));
         fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
     }
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b82921d..589f530 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -451,7 +451,9 @@ static void pc_i440fx_machine_options(MachineClass *m)
 
 static void pc_i440fx_2_4_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_machine_options(m);
+    pcmc->broken_reserved_end = true;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->alias = "pc";
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7217cbf..839d3b9 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -370,7 +370,9 @@ static void pc_q35_machine_options(MachineClass *m)
 
 static void pc_q35_2_4_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_machine_options(m);
+    pcmc->broken_reserved_end = true;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->no_floppy = 1;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d0cad87..ff0b48b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -59,6 +59,7 @@ struct PCMachineClass {
     MachineClass parent_class;
 
     /*< public >*/
+    bool broken_reserved_end;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
 };
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] pc: memhotplug: fix incorrectly set reserved-memory-end

[Eduardo Habkost]

On Mon, Sep 07, 2015 at 01:55:31PM +0200, Igor Mammedov wrote:
> reserved-memory-end tells firmware address from which
> it could start treating memory as PCI address space
> and map PCI BARs after it to avoid collisions with
> RAM.
> Currently it is incorrectly pointing to address where
> hotplugged memory range starts which could redirect
> hotplugged RAM accesses to PCI BARs when firmware
> maps them over RAM or viceverse.
> Fix this by pointing reserved-memory-end to the end
> of memory hotplug area.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But I would like to see this squashed into patch 2/2 (or the rebased
version of patch 2/2) to keep bisectability.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] pc: memhotplug: keep reserved-memory-end broken on 2.4 and earlier machines

[Eduardo Habkost]

On Mon, Sep 07, 2015 at 01:55:32PM +0200, Igor Mammedov wrote:
> it will prevent guests on old machines from seeing
> inconsistent memory mapping in firmware/ACPI views.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS: this patch probably clashes with Eduardo's
>     pc_compat refactoring, so I've split it from
>     main fix, if applied via Eduardo's it could
>     be squashed in main fix after merging on top
>     of his patches or as is if goes in first.

It conflicts with my "pc: Initialization and compat function cleanup"
series, but my series needs to be rebased because of smbios changes
anyway. I can rebase my series after your fix gets in.

[...]
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 354e1b3..b5107f7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1412,8 +1412,12 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
>  
>      if (guest_info->has_reserved_memory && pcms->hotplug_memory.base) {
>          uint64_t *val = g_malloc(sizeof(*val));
> -        uint64_t res_mem_end = pcms->hotplug_memory.base +
> -                               memory_region_size(&pcms->hotplug_memory.mr);
> +        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);

Style suggestion: move pcmc to the beginning of the function, because
other blocks inside the function are likely to look at PCMachineClass
fields in the future.

> +        uint64_t res_mem_end = pcms->hotplug_memory.base;
> +
> +        if (!pcmc->broken_reserved_end) {
> +            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
> +        }
>          *val = cpu_to_le64(ROUND_UP(res_mem_end, 0x1ULL << 30));
>          fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
>      }
[...]
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d0cad87..ff0b48b 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -59,6 +59,7 @@ struct PCMachineClass {
>      MachineClass parent_class;
>  
>      /*< public >*/
> +    bool broken_reserved_end;

A comment describing what exactly the new field does would be welcome.

I don't think my suggestions should block the patch, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] pc: memhotplug: fix incorrectly set reserved-memory-end

[Igor Mammedov]

On Wed, 9 Sep 2015 17:03:56 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Sep 07, 2015 at 01:55:31PM +0200, Igor Mammedov wrote:
> > reserved-memory-end tells firmware address from which
> > it could start treating memory as PCI address space
> > and map PCI BARs after it to avoid collisions with
> > RAM.
> > Currently it is incorrectly pointing to address where
> > hotplugged memory range starts which could redirect
> > hotplugged RAM accesses to PCI BARs when firmware
> > maps them over RAM or viceverse.
> > Fix this by pointing reserved-memory-end to the end
> > of memory hotplug area.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> But I would like to see this squashed into patch 2/2 (or the rebased
> version of patch 2/2) to keep bisectability.

Eduardo, thanks for review.
It's ok squash 2/2, could you handle it and take it through your tree?

Re: [Qemu-devel] [PATCH 1/2] pc: memhotplug: fix incorrectly set reserved-memory-end

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 11:59:07AM +0200, Igor Mammedov wrote:
> On Wed, 9 Sep 2015 17:03:56 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Sep 07, 2015 at 01:55:31PM +0200, Igor Mammedov wrote:
> > > reserved-memory-end tells firmware address from which
> > > it could start treating memory as PCI address space
> > > and map PCI BARs after it to avoid collisions with
> > > RAM.
> > > Currently it is incorrectly pointing to address where
> > > hotplugged memory range starts which could redirect
> > > hotplugged RAM accesses to PCI BARs when firmware
> > > maps them over RAM or viceverse.
> > > Fix this by pointing reserved-memory-end to the end
> > > of memory hotplug area.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > But I would like to see this squashed into patch 2/2 (or the rebased
> > version of patch 2/2) to keep bisectability.
> 
> Eduardo, thanks for review.
> It's ok squash 2/2, could you handle it and take it through your tree?

I merged them separately already:
I'm not too concerned about breaking cross version migration
being slightly racy when doing bisect.

-- 
MST