Re: [PATCH 1/4] target/ppc: Add IBM PPE42 family of processors

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 2025-08-26 14:12, BALATON Zoltan wrote:
> On Tue, 26 Aug 2025, Harsh Prateek Bora wrote:
>> On 8/25/25 20:03, BALATON Zoltan wrote:
>>> On Mon, 25 Aug 2025, Harsh Prateek Bora wrote:
>>>> On 8/25/25 17:52, Thomas Huth wrote:
>>>>> On 25/08/2025 14.08, Harsh Prateek Bora wrote:
>>>>>> On 8/25/25 17:28, Thomas Huth wrote:
>>>>>>> As I said, qemu-system-ppc64 is currently a full superset of
>>>>>>> qemu-system- ppc. The ppc64 binary contains all the 32-bit code, you
>>>>>>> can perfectly run a "g3beige" or "bamboo" machine with
>>>>>>> qemu-system-ppc64, too. By disabling the ppe42 code in the ppc64
>>>>>>> binary, this would now introduce an execption to that unwritten rule,
>>>>>>> so I'd expect that we'd not rather want to do this now.
>>>>>>
>>>>>> My understanding is that above holds true only for default builds which
>>>>>> builds all targets. We certainly do not build 32 bit ppc code when using
>>>>>> --configure target-list=ppc64-softmmu. (we have ppc-softmmu for 32 bit
>>>>>> though)
>>>
>>> We do build 32-bit machines in ppc64-softmmu but leave out 64-bit from
>>> ppc-softmmu so it's only one way.
>>>
>>>>> Just give it a try:
>>>>>
>>>>>    ./configure --target-list=ppc64-softmmu --disable-docs
>>>>>    make -j$(nproc)
>>>>>    ./qemu-system-ppc64 -M g3beige
>>>>>
>>>>> ... works perfectly fine for me.
>>>>>
>>>> This would work because the respective code is not restricted with #ifndef
>>>> TARGET_PPC64.
>>>>
>>>> However, there are instance like below in hw/ppc/mac_oldworld.c:
>>>>
>>>> static void heathrow_class_init(ObjectClass *oc, const void *data)
>>>> {
>>>>     MachineClass *mc = MACHINE_CLASS(oc);
>>>>     FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(oc);
>>>>
>>>>     mc->desc = "Heathrow based PowerMac";
>>>>     mc->init = ppc_heathrow_init;
>>>>     mc->block_default_type = IF_IDE;
>>>>     /* SMP is not supported currently */
>>>>     mc->max_cpus = 1;
>>>> #ifndef TARGET_PPC64
>>>>     mc->is_default = true;
>>>> #endif
>>>
>>> This is only because the default machine for ppc64-softmmu is different
>>> than for ppc-softmmu as the 64-bit machines don't exist in ppc-softmmu but
>>> ppc64-softmmu had different default machine before machines from
>>> qemu-system-ppc got included in qemu-system-ppc64 so it kept that. (Looks
>>> like the default used to be mac_newworld before commit 159f8286b760dea but
>>> wasn't changed to match but to something else.) The default machines are
>>> arbitrary, we could make "none" the default and always require users to
>>> supply -machine but that would break existing command lines so this wasn't
>>> done.
>>>
>>>> Similarly, we have multiple instances with #else block for #ifdef
>>>> TARGET_PPC64 which doesnt get compiled with ppc64-softmmu, but only with
>>>> ppc-softmmu meant for 32-bit targets. See target/ppc/excp_helper.c for
>>>> example.
>>>
>>> This is again leaving out 64-bit code from ppc-softmmu but as Thomas says
>>> 32-bit machines are always included in qemu-softmmu-ppc64. I can't find the
>>> commit which changed this, previously we had these to be separate and since
>>> some types are different in ppc64-softmmu it wasn't clear if that could
>>> cause any problems for 32-bit CPUs and machines so ppc-softmmu was kept
>>> until that's cleaned up which never happened. There are also some
>>> pecularities in some machines like mac_newworld that behaves differently in
>>> qemu-system-ppc and qemu-system-ppc64 and the potentially lower performance
>>> of qemu-system-ppc64 in emulating 32-bit machines which is why we still
>>> have ppc-softmmu.
>>>
>>
>> Ok, I see. So, if we wish to keep the 32-bit machines supported with
>> qemu-system-ppc64, we will have them co-exist with run-time checks for !ppc64
>> and/or "unlikely" operations in the hot path routines, which wouldnt be
>> needed otherwise. I hope we can deal with run-time checks for !ppc64 if such
>> ops increase.
> 
> I guess this will only get worse with the single binary that will also add
> if target == checks and removes some of the optimisations that assumed
> endianness and word size. It would be good if somebody could do some tests
> to check performance issues which could be made part of the test suite so
> we at least notice these changes. I'm not sure what would be a good set of
> benchmarks, I've used STREAM to check memory access and exceptions/MMU and
> lame some.waw some.mp3 to check FPU and vector instructions speed before
> but maybe there are better tools.
> 
> I've also tried adding function pointers to CPU class for different
> exception models to remove the switch from the beginning of excp handling
> functions that call out to PPC64 specific versions but I found that to be
> slower than the current switch at least on x86_64 host.
>

Having performance tests would be a great addition to the QEMU test 
suite, whether it's for the single binary or any other change.

Concerning the single binary, we have no plan to modify ppc target code 
for now. As well, most of other architecture already handle 32 vs 64bits 
and endianness with runtime paths, and it has not been a problem so far.

I still think it's better to write proper code and eventually optimize 
parts identified by proper profiling. But impacting architecture and use 
preprocessor heavily before any proof that is needed has been found is 
premature optimization IMHO.

Regards,
Pierrick