Re: [PATCH v5 11/16] hw/microblaze: Support various endianness for s3adsp1800 machines

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 6/2/25 18:12, Daniel P. Berrangé wrote:
> On Thu, Feb 06, 2025 at 04:04:20PM +0100, Philippe Mathieu-Daudé wrote:
>> On 6/2/25 15:31, Daniel P. Berrangé wrote:
>>> On Thu, Feb 06, 2025 at 02:53:58PM +0100, Philippe Mathieu-Daudé wrote:
>>>> Hi Daniel,
>>>>
>>>> On 6/2/25 14:20, Daniel P. Berrangé wrote:
>>>>> On Thu, Feb 06, 2025 at 02:10:47PM +0100, 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.
>>>>>
>>>>> Does it makes sense to expose these as different machine types ?
>>>>>
>>>>> If all the HW is identical in both cases, it feels like the
>>>>> endianness could just be a bool property of the machine type,
>>>>> rather than a new machine type.
>>>>
>>>> Our test suites expect "qemu-system-foo -M bar" to work out of
>>>> the box, we can not have non-default properties.
>>>>
>>>> (This is related to the raspberry pi discussion in
>>>> https://lore.kernel.org/qemu-devel/20250204002240.97830-1-philmd@linaro.org/).
>>>>
>>>> My plan is to deprecate 'petalogix-s3adsp1800', so once we
>>>> remove it we can merge both qemu-system-microblaze and
>>>> qemu-system-microblazeel into a single binary.
>>>>
>>>> If you don't want to add more machines, what should be the
>>>> endianness of the 'petalogix-s3adsp1800' machine in a binary
>>>> with no particular endianness? Either we add for explicit
>>>> endianness (fixing test suites) or we add one machine for
>>>> each endianness; I fail to see other options not too
>>>> confusing for our users.
>>>
>>> We would pick an arbitrary endianness of our choosing
>>> I guess. How does this work in physical machines ? Is
>>> the choice of endianess a firmware setting, or a choice
>>> by the vendor when manufacturing in some way ?
>>
>> Like MIPS*, SH4* and Xtensa*, it is a jumper on the board
>> (wired to a CPU pin which is sampled once at cold reset).
> 
> That makes me thing even more it is just a machine type property.

I'm happy with a machine property, this was even my first approach
using OnOffAuto until I ran the test-suite and have qom-introspection
failing.

What should be the default?

Per the SH4 datasheet:

   Bit 31—Endian Flag (ENDIAN): Samples the value of the endian
   specification external pin (MD5) in a power-on reset by the
   RESET pin. The endian mode of all spaces is determined by this
   bit. ENDIAN is a read-only bit.

There is no default per the spec, and I believe using one is
a mistake.

> None the less, since you've already taken this pattern of
> dual machine types for BE & LE on  MIPS/SH4/XTensa, I think
> we should stick with your precedent. Consistent modelling
> of endian handling across all machine types is most important
> IMHO
> 
> 
> With regards,
> Daniel