Re: [PATCH v5 3/4] xen/ppc: Implement early serial printk on pseries

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 7/26/23 10:45 AM, Jan Beulich wrote:
> On 26.07.2023 17:42, Shawn Anastasio wrote:
>> On 7/26/23 10:32 AM, Jan Beulich wrote:
>>> On 24.07.2023 17:06, Shawn Anastasio wrote:
>>>> On 7/24/23 7:40 AM, Jan Beulich wrote:
>>>>> On 21.07.2023 19:02, Shawn Anastasio wrote:
>>>>>> On typical Power VMs (e.g. QEMU's -M pseries), a variety of services
>>>>>> including an early serial console are provided by Open Firmware.
>>>>>> Implement the required interfaces to call into Open Firmware and write
>>>>>> to the serial console.
>>>>>>
>>>>>> Since Open Firmware runs in 32-bit Big Endian mode and Xen runs in
>>>>>> 64-bit Little Endian mode, a thunk is required to save/restore
>>>>>> any potentially-clobbered registers as well as to perform the
>>>>>> required endianness switch. Thankfully, linux already has such
>>>>>> a routine, which was imported into ppc64/of-call.S.
>>>>>>
>>>>>> Support for bare metal (PowerNV) will be implemented in a future
>>>>>> patch.
>>>>>>
>>>>>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>>>>>
>>>>> While I've committed the earlier two patches, I had to back out this
>>>>> one. In my environment (gcc13) the build fails due an unresolved
>>>>> reference to memset() out of boot-of.c (supposedly from of_call()).
>>>>
>>>> Does removing the `{ 0 }` initializer to `struct of_service s` on line
>>>> 43 resolve this?
>>>
>>> Yes, that's what's causing the call (and removing, whether or not correct,
>>> helps).
>>
>> Thanks for confirming. Removing it should be fine since the code
>> manually initializes all of the other fields of the struct. The only
>> behavioral difference is that the members of `ofs_args` at indices >=
>> nargs would be left uninitialized. This shouldn't be an issue though
>> since we're guarding reads of the array on `nargs` and `nrets` and thus
>> only read explicitly initialized values (and of course, firmware would
>> do the same).
>>
>> Naturally we can't avoid memset calls forever. I have lib/ building
>> locally, but if we could get this series in without having to make those
>> changes here that'd be great.
> 
> Are you suggesting I should put in this patch almost as is, with just
> that initializer dropped?

Yes. I've tested the change locally and it still behaves correctly, so
if dropping it removes the memset invocation on your toolchain then I'd
say the patch is fine to go with that initializer dropped.

I could also submit a v6 with the initializer dropped, if you'd be more
comfortable with that.

> Jan

Thanks,
Shawn