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

[Eric Auger]

Hi,


On 1/28/25 10:28 PM, Matt Ochs wrote:
>> 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.

I meant that if by default we keep the size value equal to 512G (the
existing default value), I don't think we need to care about compats.

Eric
>
>