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-da...@redhat.com/ > > Signed-off-by: Jonathan Cameron <jonathan.came...@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
