On 19.05.24 11:24, Daniel Henrique Barboza wrote:
On 5/18/24 16:50, David Hildenbrand wrote:
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.
Thanks for the explanation. I missed the part where the ram_slots were being
used just to solve potential alignment issues and pc-dimms could occupy the same
space being allocated via machine_memory_devices_init().
This patch isn't far off then. If we take care to avoid plugging unaligned
memory
we might not even need this spare area.
Right.
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 was trying to refer to a situation we faced 3+ years ago in the powerpc
machines,
where we were trying to add virtio-mem support there given that virtio-mem
is/was
been seen (as far as I can remember anyways) as a more robust solution than
virtio-balloon + DIMM hotplug for guest memory management from the host point of
view.
I only heard once that ppc support was being worked on, but don't know
of any details what the issue was. PPC+KVM is not really a priority
anymore, so my primary focus for the future is adding s390x support (I
had prototypes but some things are tricky).
Supporting architectures that do their own weird paravirtualized stuff
already or don't really support memory hotplug is a bit more tricky than
the others. PPC with dlpar falls into that category :)
--
Cheers,
David / dhildenb
