Re: [Qemu-devel] [PATCH 2/2] pc: memhp: force gaps between DIMM's GPA

[Michael S. Tsirkin]

On Fri, Sep 25, 2015 at 03:53:12PM +0200, Igor Mammedov wrote:
> mapping DIMMs non contiguously allows to workaround
> virtio bug reported earlier:
> http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html
> in this case guest kernel doesn't allocate buffers
> that can cross DIMM boundary keeping each buffer
> local to a DIMM.
> 
> Suggested-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> benefit of this workaround is that no guest side
> changes are required.

That's a hard requirement, I agree.


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

Aren't other architectures besides PC ever affected?
Do they all allocate all of memory contigious in HVA space?

Also - does the issue only affect hotplugged memory?

Can't the patch be local to pc-dimm (except maybe the
backwards compatibility thing)?


> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 91d134c..c462c4e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1629,6 +1629,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm);
> @@ -1644,7 +1645,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>          goto out;
>      }
>  
> -    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, 0, 
> &local_err);
> +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align,
> +                        pcmc->inter_dimm_gap, &local_err);
>      if (local_err) {
>          goto out;
>      }
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3ffb05f..3165667 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -457,11 +457,13 @@ static void pc_xen_hvm_init(MachineState *machine)
>  
>  static void pc_i440fx_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      m->family = "pc_piix";
>      m->desc = "Standard PC (i440FX + PIIX, 1996)";
>      m->hot_add_cpu = pc_hot_add_cpu;
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
> +    pcmc->inter_dimm_gap = PC_2MB_DIMM_GAP;
>  }
>  
>  static void pc_i440fx_2_5_machine_options(MachineClass *m)
> @@ -482,6 +484,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      m->alias = NULL;
>      m->is_default = 0;
>      pcmc->broken_reserved_end = true;
> +    pcmc->inter_dimm_gap = 0;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1b7d3b6..8ad6687 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -360,6 +360,7 @@ static void pc_compat_1_4(MachineState *machine)
>  
>  static void pc_q35_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      m->family = "pc_q35";
>      m->desc = "Standard PC (Q35 + ICH9, 2009)";
>      m->hot_add_cpu = pc_hot_add_cpu;
> @@ -368,6 +369,7 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->default_display = "std";
>      m->no_floppy = 1;
>      m->no_tco = 0;
> +    pcmc->inter_dimm_gap = PC_2MB_DIMM_GAP;
>  }
>  
>  static void pc_q35_2_5_machine_options(MachineClass *m)
> @@ -385,6 +387,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      pc_q35_2_5_machine_options(m);
>      m->alias = NULL;
>      pcmc->broken_reserved_end = true;
> +    pcmc->inter_dimm_gap = 0;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 6896328..dd6b34a 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -50,6 +50,7 @@ struct PCMachineState {
>  #define PC_MACHINE_SMM              "smm"
>  #define PC_MACHINE_ENFORCE_ALIGNED_DIMM "enforce-aligned-dimm"
>  
> +#define PC_2MB_DIMM_GAP (1ULL << 21)
>  /**
>   * PCMachineClass:
>   * @get_hotplug_handler: pointer to parent class callback 
> @get_hotplug_handler

Seems somewhat arbitrary. It's aligned later - so won't a 1 byte gap be enough?

> @@ -60,6 +61,7 @@ struct PCMachineClass {
>  
>      /*< public >*/
>      bool broken_reserved_end;
> +    uint64_t inter_dimm_gap;
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
>  };
> -- 
> 1.8.3.1