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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Sep 27, 2015 at 03:06:24PM +0200, Igor Mammedov wrote:
> On Sun, 27 Sep 2015 13:48:21 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mst@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > 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?
> I'm not sure about other targets I've CCed interested parties.
> 
> > 
> > Also - does the issue only affect hotplugged memory?
> Potentially it affects -numa memdev=foo, but however I've
> tried I wasn't able to reproduce.
> We could do it as
> separate workaround later if it would affect someone
> and virtio is not fixed to handle split buffers by that time.
> 

You can't reproduce a crash or you can't reproduce getting contigious
GPA with fragmented HVA?
If you can see fragmentation that's enough to assume guest crash can
be triggered, even if it doesn't with Linux.

>  
> > Can't the patch be local to pc-dimm (except maybe the
> > backwards compatibility thing)?
> I think decision about using gaps and its size
> should be done by board and not generic pc-dimm.
> 

Well virtio is generic and can be used by all boards.


> > > 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?
> 1 byte should be also enough, since effectively it would kick alignment adjustment.
> 
> The reason why I've picked 2Mb is that QEMU ram allocator allocates
> 2Mb granularity.

It will align it itself. We don't want that logic to spread out to
unrelated parts of QEMU.

> > 
> > > @@ -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