Re: [PATCH v3 8/9] hw/i386/x86: Make TYPE_X86_MACHINE the owner of smram

[Bernhard Beschow]

Am 7. Februar 2023 18:34:40 UTC schrieb Juan Quintela <quintela@redhat.com>:
>Bernhard Beschow <shentey@gmail.com> wrote:
>v> On Mon, Feb 6, 2023 at 11:06 AM Juan Quintela <quintela@redhat.com> wrote:
>>
>>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>> > On 4/2/23 16:10, Bernhard Beschow wrote:
>>> >> Treat the smram MemoryRegion analoguous to other memory regions such as
>>> >> ram, pci, io, ... , making the used memory regions more explicit when
>>> >> instantiating q35 or i440fx.
>>> >> Note that the q35 device uses these memory regions only during the
>>> >> realize phase which suggests that it shouldn't be the owner of smram.
>>> >
>>> > Few years ago I tried something similar and it wasn't accepted because
>>> > the MR owner name is used in the migration stream, so this was breaking
>>> > migrating from older machines.
>>>
>>> I don't remember the details O:-)
>>>
>>> Migration code, really depends on RAMBlocks names, not memory region
>>> names.  But as far as I remember, that don't matter too much because the
>>> memory region names ends tangled quite a bit with the RAMBlock name, right?
>>>
>>> > Adding David/Juan for double-checking that.
>>>
>>>     trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>>>
>>> You can try to enable this trace and see that every section has the same
>>> name with and without your change (i.e. that memory region name is not
>>> seen by the migration stream).
>>>
>>> But that is the only help that I can came with.
>>>
>>> The code that you are changing (smram) is something that I don't know
>>> about to give you more help.
>>>
>>> Looking at the patch, it looks that the name was before and now the
>>> "sram", so perhaps it could help.  But I don't know.
>>>
>>> In the i440fx you say that you only use it until realize, so you should
>>> be safe.
>>>
>>> For q35, it is not clear to me.
>>>
>>> If the trace don't show new names, I will just try:
>>> - migrate a i440fx machine from binary without your patch to one with
>>>   your patch
>>> - the same for q35.
>>>
>>> And depending on the result, we can go from there.
>>>
>>
>> Thanks for the pointers, Juan!
>>
>> I took some inspiration and created four migration files,
>> {pc,q35}-{before,after}.mig by running `qemu-system-x86_64 -M {pc,q35} -S`
>> with qemu built from master and from my branch. Then I basically ran
>>  `./scripts/analyze-migration.py -d desc -f *.mig > *.json` on the four
>> files and compared the diffs. Both diffs were empty. AFAIU this proves that
>> there is no binary change, right?
>
>We have two options here:
>- you are right (my opinion)
>- you got a bug in analyze-migration.py script and you have a new job.

I take option one ;)

>But I think you can send this patch.

The patches still need reviews.

Best regards,
Bernhard
>
>Later, Juan.
>
>> Best regards,
>> Bernhard
>>>
>>>
>>> Later, Juan.
>>>
>>>
>