Re: [PATCH] hw/arm/virt: Support larger highmem MMIO regions

[Matt Ochs]

> On Jan 28, 2025, at 11:52 AM, Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Matthew, Shameer,
> 
> On 1/28/25 6:36 PM, Shameerali Kolothum Thodi wrote:
>> 
>>> -----Original Message-----
>>> From: Matthew R. Ochs <mochs@nvidia.com>
>>> Sent: Tuesday, January 28, 2025 4:03 PM
>>> To: qemu-devel@nongnu.org; Shameerali Kolothum Thodi
>>> <shameerali.kolothum.thodi@huawei.com>; nathanc@nvidia.com
>>> Cc: ddutile@redhat.com; eric.auger@redhat.com; nicolinc@nvidia.com;
>>> ankita@nvidia.com
>>> Subject: [PATCH] hw/arm/virt: Support larger highmem MMIO regions
>>> 
>>> The MMIO region size required to support virtualized environments with
>>> large PCI BAR regions can exceed the hardcoded limit configured in QEMU.
>>> For example, a VM with multiple NVIDIA Grace-Hopper GPUs passed
>>> through
>>> requires more MMIO memory than the amount provided by
>>> VIRT_HIGH_PCIE_MMIO
>>> (currently 512GB). Instead of updating VIRT_HIGH_PCIE_MMIO, introduce a
>>> new parameter, highmem-mmio-size, that specifies the MMIO size required
>>> to support the VM configuration.
>>> 
>>> Example usage with 1TB MMIO region size:
>>> -machine virt,gic-version=3,highmem-mmio-size=1099511627776
>> I guess you could do highmem-mmio-size=1024G as well.

Sure, the visit_type_size() helper can understand the common unit suffixes
used for sizes, e.g. 1T. To clarify, I’ll update the example in v2.

>>> +highmem-mmio-size
>>> +  Set extended MMIO memory map size. Must be a power-of-2 and greater
> maybe keep the existing terminology, ie. high PCIE MMIO region.
> extended_memmap
> Would deserve to add some comments on top of extended_memmap[] too.

Thanks for the suggestion, will update the patch to use existing terminology.

>>> +    if (!visit_type_size(v, name, &size, errp))
>>> +        return;
>> Qemu style mandates braces around.

Will fix in v2.

>> 
>>> +
>>> +    if (!is_power_of_2(size)) {
>>> +        error_setg(errp, "highmem_mmio_size is not a power-of-2");
>>> +        return;
>>> +    }
>>> +
>>> +    if (size < extended_memmap[VIRT_HIGH_PCIE_MMIO].size) {
>> Not sure it is better to fallback to default size here instead of setting 
>> error.
> I think if the user sets a value it shall be obeyed

Agreed.

> Note that per the dynamic memory map algo, changing the size will also
> change the base address. See
> 
> virt_set_high_memmap(). By the wayn why do we forbid a smaller size?

That’s a good point, I will remove this check.

>>> 
>>> +    object_class_property_add(oc, "highmem-mmio-size", "size",
>>> +                                   virt_get_highmem_mmio_size,
>>> +                                   virt_set_highmem_mmio_size,
>>> +                                   NULL, NULL);
>>> +    object_class_property_set_description(oc, "highmem-mmio-size",
>>> +                                          "Set extended MMIO memory map 
>>> size");
>>> +
>> I think this probably needs backward compatibility to keep migration happy.
>> Isn't it? See the no_highmem_compact handling.
> I guess if we keep the same value as default we are good. The difference
> with highmem_compact is it was set by default from 7.2 onwards hence
> changing the mmio layout. Here by default you keep the same IIUC.

I’m not sure I see an issue since the code is directly modifying the size value
in the extended_memmap array.