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

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Sun, Sep 27, 2015 at 04:04:06PM +0200, Igor Mammedov wrote:
> On Sun, 27 Sep 2015 16:11:02 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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.
> I'll check it.
> 
> > 
> > >  
> > > > 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.
> Still pc-dimm.addr is not allocation is not part of pc-dimm
> device. it's just helper functions that happen to live in
> the same file source file.
> 
> But more importantly every target might have it's own
> notion how it partitions hotplug address space so making
> the same gap global might break them.
> 
> It's safer to enable gaps per target, I think ppc guys
> will make their own patch on top of this to taking
> in account their target specific and compat stuff.

I have never seen this issue that you mention at

http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg00522.html

in PowerPC. I have not been able to reproduce the QEMU crash with the
commandline suggested there.

(# ./ppc64-softmmu/qemu-system-ppc64 --enable-kvm --nographic -machine pseries -m 8G,slots=32,maxmem=32G -device virtio-blk-pci,drive=rootdisk -drive file=/home/bharata/F20-snap1,if=none,cache=none,id=rootdisk,format=qcow2 -monitor telnet:localhost:1235,server,nowait -vga none -bios /home/bharata/slof/slof.bin -smp 16,maxcpus=32 -netdev tap,id=foo,ifname=tap0,script=/home/bharata/qemu-ifup -device virtio-net-pci,id=n1,netdev=foo `for i in $(seq 0 15); do echo -n "-object memory-backend-ram,id=m$i,size=256M -device pc-dimm,id=dimm$i,memdev=m$i "; done` -snapshot)

PowerPC sPAPR memory hotplug enforces memory alignment of 256MB
for both boottime as well as hotplugged memory.

So not sure if anything other than the default gap=0 which you have
done in this patchset for PowerPC is necessary.

Regards,
Bharata.