> On Jan 29, 2025, at 2:15 AM, Eric Auger <eric.au...@redhat.com> wrote:
> Hi Shameer,
>
> On 1/29/25 9:10 AM, Shameerali Kolothum Thodi wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.au...@redhat.com>
>>> Sent: Wednesday, January 29, 2025 7:56 AM
>>> To: Matt Ochs <mo...@nvidia.com>; Shameerali Kolothum Thodi
>>> <shameerali.kolothum.th...@huawei.com>
>>> Cc: qemu-devel@nongnu.org; Nathan Chen <nath...@nvidia.com>;
>>> ddut...@redhat.com; Nicolin Chen <nicol...@nvidia.com>; Ankit Agrawal
>>> <ank...@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?
I can’t really think of a use-case, so not including the check would likely
fall under keeping the code smaller/simpler. Then again, I don’t have a
strong opinion on not including it.
>>
>>>>>>> + 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.
I tested migration and encountered I/O issues (hangs, external aborts, etc) when
the values were different, but no issues when the values were the same.
I don’t think the backward compatibility check alone would help here.
Are there migration facilities that evaluate source and destination
configuration
prior to migrating? If not, it’s up to the orchestrator of the migration to
ensure
the source and destination values are the same.
