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

[Björn Töpel]

Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:

> On 5/20/24 15:51, Björn Töpel wrote:
>> Daniel/David,
>> 
>> Daniel Henrique Barboza <dbarboza@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 for the elaborate explaination! I'll take a stab at it in v2.


Björn