Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 5109 bytes --]

On Sun, 23 May 2021, Alexey Kardashevskiy wrote:
> On 23/05/2021 01:02, BALATON Zoltan wrote:
>> On Sat, 22 May 2021, BALATON Zoltan wrote:
>>> On Sat, 22 May 2021, Alexey Kardashevskiy wrote:
>>>> VOF itself does not prints anything in this patch.
>>> 
>>> However it seems to be needed for linux as the first thing it does seems 
>>> to be getting /chosen/stdout and calls exit if it returns nothing. So I'll 
>>> need this at least for linux. (I think MorphOS may also query it to print 
>>> a banner or some messages but not sure it needs it, at least it does not 
>>> abort right away if not found.)
>>> 
>>>>> but to see Linux output do I need a stdout in VOF or it will just open 
>>>>> the serial with its own driver and use that?
>>>>> So I'm not sure what's the stdout parts in the current vof patch does 
>>>>> and if I need that for anything. I'll try to experiment with it some 
>>>>> more but fixing the ld and Kconfig seems to be enough to get it work for 
>>>>> me.
>>>> 
>>>> So for the client to print something, /chosen/stdout needs to have a 
>>>> valid ihandle.
>>>> The only way to get a valid ihandle is having a valid phandle which 
>>>> vof_client_open() can open.
>>>> A valid phandle is a phandle of any node in the device tree. On spapr we 
>>>> pick some spapr-vty, open it and store in /chosen/stdout.
>>>> 
>>>> From this point output from the client can be seen via a tracepoint.
>> 
>> I've got it now. Looking at the original firmware device tree dump:
>> 
>> https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2/attach/PegasosII_OFW-Dump.txt 
>> 
>> I see that /chosen/stdout points to "screen" which is an alias to 
>> /bootconsole. Just adding an empty /bootconsole node in the device tree and 
>> vof_client_open_store() that as /chosen/stdout works and I get output via 
>> vof_write traces so this is enough for now to test Linux. Properly 
>> connecting a serial backend can thus be postponed.
>> 
>> So with this the Linux kernel does not abort on the first device tree 
>> access but starts to decompress itself then the embedded initrd and crashes 
>> at calling setprop:
>> 
>> [...]
>> vof_client_handle: setprop
>> 
>> Thread 4 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault.
>> (gdb) bt
>> #0  0x0000000000000000 in  ()
>> #1  0x0000555555a5c2bf in vof_setprop
>>      (vof=0x7ffff48e9420, vallen=4, valaddr=<optimized out>, 
>> pname=<optimized out>, nodeph=8, fdt=0x7fff8aaff010, ms=0x5555564f8800)
>>      at ../hw/ppc/vof.c:308
>> #2  0x0000555555a5c2bf in vof_client_handle
>>      (nrets=1, rets=0x7ffff48e93f0, nargs=4, args=0x7ffff48e93c0, 
>> service=0x7ffff48e9460 "setprop",
>>       vof=0x7ffff48e9420, fdt=0x7fff8aaff010, ms=0x5555564f8800) at 
>> ../hw/ppc/vof.c:842
>> #3  0x0000555555a5c2bf in vof_client_call
>>      (ms=0x5555564f8800, vof=vof@entry=0x55555662a3d0, 
>> fdt=fdt@entry=0x7fff8aaff010, args_real=args_real@entry=23580472)
>>      at ../hw/ppc/vof.c:935
>> 
>> loooks like it's trying to set /chosen/linux,initrd-start:
>
> It is not horribly clear why it crashed though.

It crashed becuase I had TYPE_VOF_MACHINE_IF but did not set a setprop 
callback and it tried to call that here. Adding a {return true;} empty 
callback avoids this.

>> (gdb) up
>> #1  0x0000555555a5c2bf in vof_setprop (vof=0x7ffff48e9420, vallen=4, 
>> valaddr=<optimized out>, pname=<optimized out>, nodeph=8,
>>      fdt=0x7fff8aaff010, ms=0x5555564f8800) at ../hw/ppc/vof.c:308
>> 308            if (!vmc->setprop(ms, nodepath, propname, val, vallen)) {
>> (gdb) p nodepath
>> $1 = "/chosen\000\060/rPC,750CXE/", '\000' <repeats 234 times>
>> (gdb) p propname
>> $2 = 
>> "linux,initrd-start\000linux,initrd-end\000linux,cmdline-timeout\000bootarg" 
>> (gdb) p val
>> $3 = <optimized out>
>> 
>> I think I need the callback for setprop in TYPE_VOF_MACHINE_IF. I can copy 
>> spapr_vof_setprop() but some explanation on why that's needed might help. 
>> Ciould I just do fdt_setprop in my callback as vof_setprop() would do 
>> without a machine callback or is there some special handling needed for 
>> these properties?
>
> The short answer is yes, you do not need TYPE_VOF_MACHINE_IF.
>
> The long answer is that we build the FDT on spapr twice:
> 1. at the reset time and
> 2. after "ibm,client-arhitecture-support" (early in the boot the spapr 
> paravirtual client says what it supports - ISA level, MMU features, etc)
>
> Between 1 and 2 the kernel moves initrd and we do not update the QEMU's 
> version of its location, the tree at 2) will have the old values.
>
> So for that reason I have TYPE_VOF_MACHINE_IF. You most definitely do not 
> need it.

I need TYPE_VOF_MACHINE_IF because that has the quiesce callback that I 
need to shut VOF down when the guest is finished with it otherwise it 
would crash later (more on this in next message). But since I shut down 
VOF here I don't need to remember changes to the FDT so I can just use an 
empty setprop callback. (I wouldn't even need that if VOF would check that 
a callback is non-NULL before calling it.)

Regards,
BALATON Zoltan