On 30.11.21 01:33, Gavin Shan wrote:
> This supports virtio-mem-pci device on "virt" platform, by simply
> following the implementation on x86.
Thanks for picking this up!
>
> * The patch was written by David Hildenbrand <da...@redhat.com>
> modified by Jonathan Cameron <jonathan.came...@huawei.com>
Maybe replace this section by
Co-developed-by: David Hildenbrand <da...@redhat.com>
Co-developed-by: Jonathan Cameron <jonathan.came...@huawei.com>
>
> * This implements the hotplug handlers to support virtio-mem-pci
> device hot-add, while the hot-remove isn't supported as we have
> on x86.
>
> * The block size is 1GB on ARM64 instead of 128MB on x86.
See below, isn't it actually 512 MiB nowadays?
>
> * It has been passing the tests with various combinations like 64KB
> and 4KB page sizes on host and guest, different memory device
> backends like normal, transparent huge page and HugeTLB, plus
> migration.
Perfect. A note that hugetlbfs isn't fully supported/safe to use until
we have preallocation support in QEMU (WIP).
>
> Signed-off-by: Gavin Shan <gs...@redhat.com>
> ---
> hw/arm/Kconfig | 1 +
> hw/arm/virt.c | 68 +++++++++++++++++++++++++++++++++++++++++-
> hw/virtio/virtio-mem.c | 2 ++
> 3 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 2d37d29f02..15aff8efb8 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -27,6 +27,7 @@ config ARM_VIRT
> select DIMM
> select ACPI_HW_REDUCED
> select ACPI_APEI
> + select VIRTIO_MEM_SUPPORTED
>
> config CHEETAH
> bool
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 369552ad45..f4599a5ef0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -71,9 +71,11 @@
> #include "hw/arm/smmuv3.h"
> #include "hw/acpi/acpi.h"
> #include "target/arm/internals.h"
> +#include "hw/mem/memory-device.h"
> #include "hw/mem/pc-dimm.h"
> #include "hw/mem/nvdimm.h"
> #include "hw/acpi/generic_event_device.h"
> +#include "hw/virtio/virtio-mem-pci.h"
> #include "hw/virtio/virtio-iommu.h"
> #include "hw/char/pl011.h"
> #include "qemu/guest-random.h"
> @@ -2480,6 +2482,63 @@ 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 on x86,
> + * however, better add a safety net.
> + */
> + error_setg(errp, "hotplug of virtio 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_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_virtio_md_pci_unplug_request(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + /* We don't support hot unplug of virtio based memory devices */
> + error_setg(errp, "virtio based memory devices cannot be unplugged.");
> +}
> +
> +
> static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> @@ -2513,6 +2572,8 @@ static void
> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
> qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
> g_free(resv_prop_str);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> + virt_virtio_md_pci_pre_plug(hotplug_dev, dev, errp);
> }
> }
>
> @@ -2538,6 +2599,8 @@ static void virt_machine_device_plug_cb(HotplugHandler
> *hotplug_dev,
> vms->iommu = VIRT_IOMMU_VIRTIO;
> vms->virtio_iommu_bdf = pci_get_bdf(pdev);
> create_virtio_iommu_dt_bindings(vms);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> + virt_virtio_md_pci_plug(hotplug_dev, dev, errp);
> }
> }
>
> @@ -2588,6 +2651,8 @@ static void
> virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> {
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> virt_dimm_unplug_request(hotplug_dev, dev, errp);
> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
> + virt_virtio_md_pci_unplug_request(hotplug_dev, dev, errp);
> } else {
> error_setg(errp, "device unplug request for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> @@ -2611,7 +2676,8 @@ static HotplugHandler
> *virt_machine_get_hotplug_handler(MachineState *machine,
> MachineClass *mc = MACHINE_GET_CLASS(machine);
>
> if (device_is_dynamic_sysbus(mc, dev) ||
> - (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/virtio-mem.c b/hw/virtio/virtio-mem.c
> index d5a578142b..3033692a83 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -126,6 +126,8 @@ static uint64_t virtio_mem_default_block_size(RAMBlock
> *rb)
> */
> #if defined(TARGET_X86_64) || defined(TARGET_I386)
> #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
> +#elif defined(TARGET_ARM)
> +#define VIRTIO_MEM_USABLE_EXTENT (2 * (1024 * MiB))
Can we make this 512 MiB ?
arch/arm64/include/asm/sparsemem.h
/*
* Section size must be at least 512MB for 64K base
* page size config. Otherwise it will be less than
* (MAX_ORDER - 1) and the build process will fail.
*/
#ifdef CONFIG_ARM64_64K_PAGES
#define SECTION_SIZE_BITS 29
#else
/*
* Section size must be at least 128MB for 4K base
* page size config. Otherwise PMD based huge page
* entries could not be created for vmemmap mappings.
* 16K follows 4K for simplicity.
*/
#define SECTION_SIZE_BITS 27
#endif /* CONFIG_ARM64_64K_PAGES */
Apart from that, LGTM -- thanks!
--
Thanks,
David / dhildenb
