Re: [PATCH v3 15/17] hw/microblaze: Support various endianness for s3adsp1800 machines

[Thomas Huth <thuth@redhat.com>]

On 11/11/2024 12.59, Philippe Mathieu-Daudé wrote:
> On 11/11/24 07:56, Thomas Huth wrote:
>> On 08/11/2024 16.43, Philippe Mathieu-Daudé wrote:
>>> Introduce an abstract machine parent class which defines
>>> the 'little_endian' property. Duplicate the current machine,
>>> which endian is tied to the binary endianness, to one big
>>> endian and a little endian machine; updating the machine
>>> description. Keep the current default machine for each binary.
>>>
>>> 'petalogix-s3adsp1800' machine is aliased as:
>>> - 'petalogix-s3adsp1800-be' on big-endian binary,
>>> - 'petalogix-s3adsp1800-le' on little-endian one.
>>>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/microblaze/petalogix_s3adsp1800_mmu.c | 62 +++++++++++++++++++-----
>>>   1 file changed, 51 insertions(+), 11 deletions(-)
>> ...
>>>   static const TypeInfo petalogix_s3adsp1800_machine_types[] = {
>>>       {
>>>           .name           = TYPE_PETALOGIX_S3ADSP1800_MACHINE,
>>>           .parent         = TYPE_MACHINE,
>>> -        .class_init     = petalogix_s3adsp1800_machine_class_init,
>>> +        .abstract       = true,
>>> +        .class_size     = sizeof(PetalogixS3adsp1800MachineClass),
>>> +    },
>>> +    {
>>> +        .name           = MACHINE_TYPE_NAME("petalogix-s3adsp1800-be"),
>>> +        .parent         = TYPE_PETALOGIX_S3ADSP1800_MACHINE,
>>> +        .class_init     = petalogix_s3adsp1800_machine_class_init_be,
>>> +    },
>>> +    {
>>> +        .name           = MACHINE_TYPE_NAME("petalogix-s3adsp1800-le"),
>>> +        .parent         = TYPE_PETALOGIX_S3ADSP1800_MACHINE,
>>> +        .class_init     = petalogix_s3adsp1800_machine_class_init_le,
>>>       },
>>>   };
>>
>> Do we really want additional machine types for this? Can't we simply let 
>> the user set the machine property instead? (otherwise, all tests that run 
>> for each machine types (see qtest_cb_for_every_machine) will now be 
>> executed three times instead of only once). IMHO it should be sufficient 
>> to have a machine property for this and add proper documentation for the 
>> machine.
> 
> Machine property was my first approach but then I figured when merging
> the 2 binaries in one, it is confusing for the CLI users.
> 
> Having 3 more tests until we unify the endianness binary doesn't seem
> a high price to pay to me. Besides, not we are not exercising the same
> code path. We need to prove the tests are really duplicated so we can
> merge the binaries. If you really insist I can modify qtests to skip
> these machines meanwhile.

Ok, I don't insist, if unify the two endianness binaries into one in the 
end, that's a much bigger win, I think, so let's keep this patch as it is.

  Thomas