> On Jan 28, 2025, at 11:52 AM, Eric Auger <eric.au...@redhat.com> wrote:
>
> Hi Matthew, Shameer,
>
> On 1/28/25 6:36 PM, Shameerali Kolothum Thodi wrote:
>>
>>> -----Original Message-----
>>> From: Matthew R. Ochs <mo...@nvidia.com>
>>> Sent: Tuesday, January 28, 2025 4:03 PM
>>> To: qemu-devel@nongnu.org; Shameerali Kolothum Thodi
>>> <shameerali.kolothum.th...@huawei.com>; nath...@nvidia.com
>>> Cc: ddut...@redhat.com; eric.au...@redhat.com; nicol...@nvidia.com;
>>> ank...@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.
