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

[David Hildenbrand]

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?

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.


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?
[...]


--
Thanks,

David / dhildenb