On 5/20/24 15:51, Björn Töpel wrote:
Daniel/David,
Daniel Henrique Barboza <dbarb...@ventanamicro.com> writes:
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.
I'm a bit lost here, so please bare with me. We don't require the 1 GiB
alignment on RV AFAIU. I'm having a hard time figuring out what missing
in my patch.
Forget about the 1 GiB size. This is something that we won't need to deal with
because we don't align in 1 Gib.
Let's say for example that we want to support pc-dimm hotplug of 256 slots like
the
'virt' ARM machine does. Let's also say that we will allow users to hotplug any
DIMM size they want, taking care of any alignment issues by ourselves.
In hw/riscv/boot.c I see that our alignment sizes are 4Mb for 32 bits and 2Mb
for
64 bits. Forget 32 bits a bit and let's say that our alignment is 2Mb.
So, in a worst case scenario, an user could hotplug 256 slots, all of them
unaligned,
and then we would need to align each one of them by adding 2Mb. So, to account
for
this alignment fixup, we would need 256 * 2Mb RAM reserved for it.
What I said about "If we take care to avoid plugging unaligned memory we might
not even
need this spare area" is a possible design where we would force every
hotplugged DIMM
to always be memory aligned, avoiding the need for this spare RAM for
alignment. This
would put a bit of extra straing in users/management apps to always deliver
aligned
DIMMs.
In hindsight this is not needed. It's fairly easy to reserve ACPI_MAX_RAM_SLOTS
* (2Mb/4Mb)
and let users hotplug whatever DIMM size they want.
Hope this explains the situation a bit. If we agree on allocating this spare
RAM for
hotplugged mem alignment, we'll probalby need a pre-patch to do a little
handling of
ms->ram_slots, limiting it to ACPI_MAX_RAM_SLOTS (ram_slots can be changed via
command
line). Then it's a matter of reserving ms->ram_slots * align_size when
calculating
device_memory_size.
Thanks,
Daniel
[...]
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.
...and also for testing the PC-DIMM ACPI patching path. ;-)
Cheers,
Björn