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

[Auger Eric]

Hi Jonathan,

On 11/5/20 6:43 PM, 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.

I would add in the commit message that the hot-unplug of the device is
not supported.
> 
> 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'.
Did you try with 64kB page guest as well?
> 
> 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.
I guess you meant CONFIG_ARCH_VIRTIO_MEM_SUPPORTED introduction
> 
> [1] https://lore.kernel.org/linux-mm/20191212171137.13872-1-david@redhat.com/
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  default-configs/devices/aarch64-softmmu.mak |  1 +
>  hw/arm/Kconfig                              |  1 +
>  hw/arm/virt.c                               | 64 ++++++++++++++++++++-
>  hw/virtio/Kconfig                           |  4 ++
>  hw/virtio/virtio-mem.c                      |  2 +
>  5 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/default-configs/devices/aarch64-softmmu.mak 
> b/default-configs/devices/aarch64-softmmu.mak
> index 958b1e08e4..31d6128a29 100644
> --- a/default-configs/devices/aarch64-softmmu.mak
> +++ b/default-configs/devices/aarch64-softmmu.mak
> @@ -6,3 +6,4 @@ include arm-softmmu.mak
>  CONFIG_XLNX_ZYNQMP_ARM=y
>  CONFIG_XLNX_VERSAL=y
>  CONFIG_SBSA_REF=y
> +CONFIG_ARCH_VIRTIO_MEM_SUPPORTED=y
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index fdf4464b94..eeae77eee9 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -20,6 +20,7 @@ config ARM_VIRT
>      select PLATFORM_BUS
>      select SMBIOS
>      select VIRTIO_MMIO
> +    select VIRTIO_MEM_SUPPORTED if ARCH_VIRTIO_MEM_SUPPORTED
>      select ACPI_PCI
>      select MEM_DEVICE
>      select DIMM
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8abb385d4e..6c96d71106 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -73,9 +73,11 @@
>  #include "hw/acpi/acpi.h"
>  #include "target/arm/internals.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/memory-device.h"
>  #include "hw/mem/nvdimm.h"
>  #include "hw/acpi/generic_event_device.h"
>  #include "hw/virtio/virtio-iommu.h"
> +#include "hw/virtio/virtio-mem-pci.h"
>  #include "hw/char/pl011.h"
>  #include "qemu/guest-random.h"
>  
> @@ -2286,6 +2288,34 @@ static void virt_memory_plug(HotplugHandler 
> *hotplug_dev,
>                           dev, &error_abort);
>  }
>  
> +static void virt_virtio_md_pci_pre_plug(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (!hotplug_dev2 && dev->hotplugged) {
> +        /*
> +         * Without a bus hotplug handler, we cannot control the plug/unplug
> +         * order. We should never reach this point when hotplugging,
> +         * however, better add a safety net.
> +         */
> +        error_setg(errp, "hotplug of virtio-mem based memory devices not"
> +                   " supported on this bus.");
> +        return;
> +    }
> +    /*
> +     * First, see if we can plug this memory device at all. If that
> +     * succeeds, branch of to the actual hotplug handler.
> +     */
> +    memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev), NULL,
> +                           &local_err);
> +    if (!local_err && hotplug_dev2) {
> +        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
>  static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                              DeviceState *dev, Error **errp)
>  {
> @@ -2293,6 +2323,8 @@ static void 
> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          virt_memory_pre_plug(hotplug_dev, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> +        virt_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>          hwaddr db_start = 0, db_end = 0;
>          char *resv_prop_str;
> @@ -2322,6 +2354,27 @@ static void 
> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> +static void virt_virtio_md_pci_plug(HotplugHandler *hotplug_dev,
> +                                  DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    /*
> +     * Plug the memory device first and then branch off to the actual
> +     * hotplug handler. If that one fails, we can easily undo the memory
> +     * device bits.
> +     */
> +    memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> +    if (hotplug_dev2) {
> +        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
> +        if (local_err) {
> +            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
> +        }
> +    }
> +    error_propagate(errp, local_err);
> +}
> +
>  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);
>      }
nit: while at it we can use "else if" here and below
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> +        virt_virtio_md_pci_plug(hotplug_dev, dev, errp);
> +    }
>      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;
>      }
virt_dimm_unplug_request() is called if the device type is TYPE_PC_DIMM
while here the device type is TYPE_VIRTIO_MEM_PCI.

Shouldn't this error be returned in
virt_machine_device_unplug_request_cb instead or do like it is done in
pc.c, add a virt_virtio_md_pci_unplug_request() helper with the
error_setg there.
>  
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> +        error_setg(&local_err,
> +                   "virtio-mem based memory devices cannot be unplugged.");
> +        goto out;
> +    }
>      hotplug_handler_unplug_request(HOTPLUG_HANDLER(vms->acpi_dev), dev,
>                                     &local_err);
>  out:
> @@ -2413,7 +2474,8 @@ static HotplugHandler 
> *virt_machine_get_hotplug_handler(MachineState *machine,
>                                                          DeviceState *dev)
>  {
>      if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) ||
> -       (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
>          return HOTPLUG_HANDLER(machine);
>      }
>      if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
> index 0eda25c4e1..00dbf2939e 100644
> --- a/hw/virtio/Kconfig
> +++ b/hw/virtio/Kconfig
> @@ -48,6 +48,10 @@ config VIRTIO_PMEM
>      depends on VIRTIO_PMEM_SUPPORTED
>      select MEM_DEVICE
>  
> +config ARCH_VIRTIO_MEM_SUPPORTED
> +    bool
> +    default n
> +
>  config VIRTIO_MEM_SUPPORTED
>      bool
>  
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 7c8ca9f28b..16f9de6ab6 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -53,6 +53,8 @@
>   */
>  #if defined(TARGET_X86_64) || defined(TARGET_I386)
>  #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
> +#elif defined(TARGET_AARCH64)
> +#define VIRTIO_MEM_USABLE_EXTENT (2 * (1024 * MiB))
>  #else
>  #error VIRTIO_MEM_USABLE_EXTENT not defined
>  #endif
> 
Otherwise looks good to me.

Thanks!

Eric