Re: [PATCH v2] hw/arm/virt enable support for virtio-mem

[Jonathan Cameron]

On Mon, 9 Nov 2020 20:47:09 +0100
David Hildenbrand <david@redhat.com> wrote:

+CC Eric based on similar query in other branch of the thread.

> On 05.11.20 18:43, Jonathan Cameron wrote:
> > Basically a cut and paste job from the x86 support with the exception of
> > needing a larger block size as the Memory Block Size (MIN_SECTION_SIZE)
> > on ARM64 in Linux is 1G.
> > 
> > Tested:
> > * In full emulation and with KVM on an arm64 server.
> > * cold and hotplug for the virtio-mem-pci device.
> > * Wide range of memory sizes, added at creation and later.
> > * Fairly basic memory usage of memory added.  Seems to function as normal.
> > * NUMA setup with virtio-mem-pci devices on each node.
> > * Simple migration test.
> > 
> > Related kernel patch just enables the Kconfig item for ARM64 as an
> > alternative to x86 in drivers/virtio/Kconfig
> > 
> > The original patches from David Hildenbrand stated that he thought it should
> > work for ARM64 but it wasn't enabled in the kernel [1]
> > It appears he was correct and everything 'just works'.
> > 
> > The build system related stuff is intended to ensure virtio-mem support is
> > not built for arm32 (build will fail due no defined block size).
> > If there is a more elegant way to do this, please point me in the right
> > direction.  
> 
> You might be aware of https://virtio-mem.gitlab.io/developer-guide.html 
> and the "issue" with 64k base pages - 512MB granularity. Similar as the 
> question from Auger, have you tried running arm64 with differing page 
> sizes in host/guest?
> 

Hi David,

> With recent kernels, you can use "memhp_default_state=online_movable" on 
> the kernel cmdline to make memory unplug more likely to succeed - 
> especially with 64k base pages. You just have to be sure to not hotplug 
> "too much memory" to a VM.

Thanks for the pointer - that definitely simplifies testing.  Was getting a bit
tedious with out that.

As ever other stuff got in the way, so I only just got back to looking at this.

I've not done a particularly comprehensive set of tests yet, but things seem
to 'work' with mixed page sizes.

With 64K pages in general, you run into a problem with the device block_size 
being
smaller than the subblock_size.  I've just added a check for that into the
virtio-mem kernel driver and have it fail to probe if that happens.  I don't
think such a setup makes any sense anyway so no loss there.  Should it make 
sense
to drop that restriction in the future we can deal with that then without 
breaking
backwards compatibility.

So the question is whether it makes sense to bother with virtio-mem support
at all on ARM64 with 64k pages given currently the minimum workable block_size
is 512MiB?  I guess there is an argument of virtio-mem being a possibly more
convenient interface than full memory HP.  Curious to hear what people think on
this?

4K guest on 64K host seems fine and no such limit is needed - though there
may be performance issues doing that.

64k guest on 4k host with 512MiB block size seems fine.

If there are any places anyone thinks need particular poking I'd appreciate a 
hint :)

Jonathan



> 
> 
> I had my prototype living at
> 
> git@github.com:davidhildenbrand/qemu.git / virtio-mem-arm64
> 
> which looks very similar to your patch. That is good :)
> 
> [...]
> 
> >   static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >                                           DeviceState *dev, Error **errp)
> >   {
> > @@ -2336,6 +2389,9 @@ static void 
> > virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> >           virt_memory_plug(hotplug_dev, dev, errp);
> >       }
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> > +        virt_virtio_md_pci_plug(hotplug_dev, dev, errp);
> > +    }  
> 
> These better all be "else if".
> 
> >       if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> >           PCIDevice *pdev = PCI_DEVICE(dev);
> >   
> > @@ -2363,6 +2419,11 @@ static void virt_dimm_unplug_request(HotplugHandler 
> > *hotplug_dev,
> >           goto out;
> >       }
> >   
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> > +        error_setg(&local_err,
> > +                   "virtio-mem based memory devices cannot be unplugged.");
> > +        goto out;
> > +    }  
> 
> This should go into virt_machine_device_unplug_request_cb() instead, no?
> [...]
> 
>