Re: [PATCH v2 1/3] hw/riscv/virt: Add memory hotplugging and virtio-md-pci support

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 5/21/24 07:56, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> Virtio-based memory devices (virtio-mem/virtio-pmem) allows for
> dynamic resizing of virtual machine memory, and requires proper
> hotplugging (add/remove) support to work.
> 
> Add device memory support for RISC-V "virt" machine, and enable
> virtio-md-pci with the corresponding missing hotplugging callbacks.
> 
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>   hw/riscv/Kconfig       |  2 +
>   hw/riscv/virt.c        | 83 +++++++++++++++++++++++++++++++++++++++++-
>   hw/virtio/virtio-mem.c |  5 ++-
>   3 files changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index a2030e3a6ff0..08f82dbb681a 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -56,6 +56,8 @@ config RISCV_VIRT
>       select PLATFORM_BUS
>       select ACPI
>       select ACPI_PCI
> +    select VIRTIO_MEM_SUPPORTED
> +    select VIRTIO_PMEM_SUPPORTED
>   
>   config SHAKTI_C
>       bool
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4fdb66052587..443902f919d2 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -53,6 +53,8 @@
>   #include "hw/pci-host/gpex.h"
>   #include "hw/display/ramfb.h"
>   #include "hw/acpi/aml-build.h"
> +#include "hw/mem/memory-device.h"
> +#include "hw/virtio/virtio-mem-pci.h"
>   #include "qapi/qapi-visit-common.h"
>   #include "hw/virtio/virtio-iommu.h"
>   
> @@ -1407,6 +1409,7 @@ static void virt_machine_init(MachineState *machine)
>       DeviceState *mmio_irqchip, *virtio_irqchip, *pcie_irqchip;
>       int i, base_hartid, hart_count;
>       int socket_count = riscv_socket_count(machine);
> +    hwaddr device_memory_base, device_memory_size;
>   
>       /* Check socket count limit */
>       if (VIRT_SOCKETS_MAX < socket_count) {
> @@ -1420,6 +1423,12 @@ static void virt_machine_init(MachineState *machine)
>           exit(1);
>       }
>   
> +    if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
> +        error_report("unsupported amount of memory slots: %"PRIu64,
> +                     machine->ram_slots);
> +        exit(EXIT_FAILURE);
> +    }
> +
>       /* Initialize sockets */
>       mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
>       for (i = 0; i < socket_count; i++) {
> @@ -1553,6 +1562,37 @@ static void virt_machine_init(MachineState *machine)
>       memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
>                                   mask_rom);
>   
> +    /* device memory */
> +    device_memory_base = ROUND_UP(s->memmap[VIRT_DRAM].base + machine->ram_size,
> +                                  GiB);
> +    device_memory_size = machine->maxram_size - machine->ram_size;
> +    if (device_memory_size > 0) {
> +        /*
> +         * Each DIMM is aligned based on the backend's alignment value.
> +         * Assume max 1G hugepage alignment per slot.
> +         */
> +        device_memory_size += machine->ram_slots * GiB;

We don't need to align to 1GiB. This calc can use 2MiB instead (or 4MiB if we're
running 32 bits).

> +
> +        if (riscv_is_32bit(&s->soc[0])) {
> +            hwaddr memtop = device_memory_base + ROUND_UP(device_memory_size,
> +                                                          GiB);

Same here - alignment is 2/4 MiB.

> +
> +            if (memtop > UINT32_MAX) {
> +                error_report("memory exceeds 32-bit limit by %lu bytes",
> +                             memtop - UINT32_MAX);
> +                exit(EXIT_FAILURE);
> +            }
> +        }
> +
> +        if (device_memory_base + device_memory_size < device_memory_size) {
> +            error_report("unsupported amount of device memory");
> +            exit(EXIT_FAILURE);
> +        }

Took another look and found this a bit strange. These are all unsigned vars, so
if (unsigned a + unsigned b < unsigned b) will always be 'false'. The compiler is
probably cropping this out.

The calc we need to do is to ensure that the extra ram_slots * alignment will fit into
the VIRT_DRAM block, i.e. maxram_size + (ram_slots * alignment) < memmap[VIRT_DRAM].size.


TBH I'm starting to have second thoughts about letting users hotplug whatever they want.
It seems cleaner to just force the 2/4 Mb alignment in pre_plug() and be done with it,
no need to allocate ram_slots * alignment and doing all these extra checks.

As I sent in an earlier email, users must already comply to the alignment of the host
memory when plugging pc-dimms, so I'm not sure our value/proposition with all this
extra code is worth it - the alignment will most likely be forced by the host memory
backend, so might as well force ourselves in pre_plug().


Thanks,


Daniel


> +
> +        machine_memory_devices_init(machine, device_memory_base,
> +                                    device_memory_size);
> +    }
> +
>       /*
>        * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the
>        * device tree cannot be altered and we get FDT_ERR_NOSPACE.
> @@ -1712,12 +1752,21 @@ 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_VIRTIO_IOMMU_PCI)) {
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI) ||
> +        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
>           return HOTPLUG_HANDLER(machine);
>       }
>       return NULL;
>   }
>   
> +static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                            DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> +        virtio_md_pci_pre_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
> +    }
> +}
> +
>   static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                           DeviceState *dev, Error **errp)
>   {
> @@ -1735,6 +1784,35 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>       if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
>           create_fdt_virtio_iommu(s, pci_get_bdf(PCI_DEVICE(dev)));
>       }
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> +        virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
> +    }
> +}
> +
> +static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                                  DeviceState *dev,
> +                                                  Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> +        virtio_md_pci_unplug_request(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev),
> +                                     errp);
> +    } else {
> +        error_setg(errp,
> +                   "device unplug request for unsupported device type: %s",
> +                   object_get_typename(OBJECT(dev)));
> +    }
> +}
> +
> +static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> +        virtio_md_pci_unplug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
> +    } else {
> +        error_setg(errp, "virt: device unplug for unsupported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>   }
>   
>   static void virt_machine_class_init(ObjectClass *oc, void *data)
> @@ -1757,7 +1835,10 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>       assert(!mc->get_hotplug_handler);
>       mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>   
> +    hc->pre_plug = virt_machine_device_pre_plug_cb;
>       hc->plug = virt_machine_device_plug_cb;
> +    hc->unplug_request = virt_machine_device_unplug_request_cb;
> +    hc->unplug = virt_machine_device_unplug_cb;
>   
>       machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>   #ifdef CONFIG_TPM
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index ffd119ebacb7..6636e5e1089c 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -51,7 +51,8 @@ static uint32_t virtio_mem_default_thp_size(void)
>   {
>       uint32_t default_thp_size = VIRTIO_MEM_MIN_BLOCK_SIZE;
>   
> -#if defined(__x86_64__) || defined(__arm__) || defined(__powerpc64__)
> +#if defined(__x86_64__) || defined(__arm__) || defined(__powerpc64__) \
> +    || defined(__riscv__)
>       default_thp_size = 2 * MiB;
>   #elif defined(__aarch64__)
>       if (qemu_real_host_page_size() == 4 * KiB) {
> @@ -161,7 +162,7 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb)
>    * necessary (as the section size can change). But it's more likely that the
>    * section size will rather get smaller and not bigger over time.
>    */
> -#if defined(TARGET_X86_64) || defined(TARGET_I386)
> +#if defined(TARGET_X86_64) || defined(TARGET_I386)  || defined(TARGET_RISCV)
>   #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
>   #elif defined(TARGET_ARM)
>   #define VIRTIO_MEM_USABLE_EXTENT (2 * (512 * MiB))