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

[Eric Auger]

Hi Shameer,

On 1/29/25 9:10 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: Wednesday, January 29, 2025 7:56 AM
>> To: Matt Ochs <mochs@nvidia.com>; Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>> Cc: qemu-devel@nongnu.org; Nathan Chen <nathanc@nvidia.com>;
>> ddutile@redhat.com; Nicolin Chen <nicolinc@nvidia.com>; Ankit Agrawal
>> <ankita@nvidia.com>
>> Subject: Re: [PATCH] hw/arm/virt: Support larger highmem MMIO regions
>
>>>>>> +    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.
> Is there really a use case where a user will want a smaller size than default?
>
>>>>>> +    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.
> Yeah. If it has default size , it should be Ok I guess. But what if,
>
> Your source is something like,
>
> ./qemu-new -machine virt-9.1,...,highmem-mmio-size=XXX
>
> and has a target VM started with
>
> ./qemu-9.1  -machine virt,...
>
> The migration will be still successful but will have memory map differences, 
> right?
>
> Or the above is not considered as a valid use case and the onus of making
> sure we are using it correctly with default size is on the user. 
To me when you migrate the qemu cmd line is supposed to be the same on
both src and dst. But worth to be tested.

Eric
>
> Thanks,
> SHameer
>
>
>
>