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

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

On 6/2/25 19:29, Daniel P. Berrangé wrote:
> On Thu, Feb 06, 2025 at 07:24:55PM +0100, Philippe Mathieu-Daudé wrote:
>> +Michal
>>
>> On 6/2/25 19:06, Daniel P. Berrangé wrote:
>>> On Thu, Feb 06, 2025 at 06:49:38PM +0100, Philippe Mathieu-Daudé wrote:
>>>> 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.
>>>
>>> If it is left as an unspecified choice in the spec, then I would
>>> presume HW vendors are picking an option based on what they
>>> expect "most" common usage to be amongst their customers. IOW,
>>> if we know of typically used guest OS prefer big or little, that
>>> could influence our choice.
>>
>> Please have a look at this thread:
>> https://lore.kernel.org/qemu-devel/d3346a55-584b-427b-8c87-32f7411a9290@amd.com/
> 
> That seems to give a pretty clear choice for qemu defaults
> 
>   "I am not aware about anybody configuring MB to big endian"
> 
> so in that particular case, defaulting to LE would be most sensible.

Or maybe I should stop trying to unify the current binaries, and add
the new data-drive machines in a new binary, as you already suggested:
https://lore.kernel.org/qemu-devel/ZEoyNt0UtSYRt9Go@redhat.com/

But then I'm worried about our users, on how to deprecate the current
microblaze{,el} binaries to have them use the new one seamlessly.

And more importantly, one of the goal is to maintain LESS binaries,
no more.