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

[David Hildenbrand]

Hi,


> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 4fdb66052587..16c2bdbfe6b6 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) {
> > @@ -1553,6 +1556,25 @@ static void virt_machine_init(MachineState *machine)
> >        memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> >                                    mask_rom);
> >    
> > +    device_memory_base = ROUND_UP(s->memmap[VIRT_DRAM].base + machine->ram_size,
> > +                                  GiB);
> > +    device_memory_size = machine->maxram_size - machine->ram_size;
> > +
> > +    if (riscv_is_32bit(&s->soc[0])) {
> > +        hwaddr memtop = device_memory_base + ROUND_UP(device_memory_size, GiB);
> > +
> > +        if (memtop > UINT32_MAX) {
> > +            error_report("Memory exceeds 32-bit limit by %lu bytes",
> > +                         memtop - UINT32_MAX);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }
> > +
> > +    if (device_memory_size > 0) {
> > +        machine_memory_devices_init(machine, device_memory_base,
> > +                                    device_memory_size);
> > +    }
> > +
>
> I think we need a design discussion before proceeding here. You're allocating 
> all
> available memory as a memory device area, but in theory we might also support
> pc-dimm hotplugs (which would be the equivalent of adding physical RAM dimms to
> the board.) in the future too. If you're not familiar with this feature you can
> check it out the docs in [1].

Note that DIMMs are memory devices as well. You can plug into the memory 
device area both, ACPI-based memory devices (DIMM, NVDIMM) or 
virtio-based memory devices (virtio-mem, virtio-pmem).

> As an example, the 'virt' ARM board (hw/arm/virt.c) reserves a space for this
> type of hotplug by checking how much 'ram_slots' we're allocating for it:
>
> device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB;

Note that we increased the region size to be able to fit most requests 
even if alignment of memory devices is weird. See below.

In sane setups, this is usually not required (adding a single additional 
GB for some flexiility might be good enough).

> Other boards do the same with ms->ram_slots. We should consider doing it as 
> well,
> now, even if we're not up to the point of supporting pc-dimm hotplug, to avoid
> having to change the memory layout later in the road and breaking existing
> setups.
>
> If we want to copy the ARM board, ram_slots is capped to ACPI_MAX_RAM_SLOTS 
> (256).
> Each RAM slot is considered to be a 1GiB dimm, i.e. we would reserve 256GiB for
> them.

This only reserves some *additional* space to fixup weird alignment of 
memory devices. *not* the actual space for these devices.

We don't consider each DIMM to be 1 GiB in size, but add an additional 1 
GiB in case we have to align DIMMs in physical address space.

I *think* this dates back to old x86 handling where we aligned the 
address of each DIMM to be at a 1 GiB boundary. So if you would have 
plugged two 128 MiB DIMMs, you'd have required more than 256 MiB of 
space in the area after aligning inside the memory device area.

> Note: I do not have the visibility of discussions on the memory management 
> space,
> and I might be missing details such as "we don't care about pc-dimm hotplug
> anymore, it's legacy, we're going to support only virtio-md-pci from now on". We
> had a situation like that with virtio-balloon and virtio-mem in the past, and 
> I'm
> not sure if this might fall in the same category.

Not sure if I got your comment right, but virtio-mem was never supposed 
to be a virtio-balloon replacement (especially of the 
free-page-reporting and memory stats part).

> I see that David Hildenbrand is also CCed in the patch so he'll let us know if
> I'm out of line with what I'm asking.

Supporting PC-DIMMs might be required at some point when dealing with 
OSes that don't support virtio-mem and friends.

--
Cheers,

David / dhildenb