Re: [PATCH qemu v2] spapr: Kill SLOF

[Alexey Kardashevskiy]

On 07/01/2020 16:26, David Gibson wrote:

>>>>>> +static uint32_t client_setprop(SpaprMachineState *sm,
>>>>>> +                               uint32_t nodeph, uint32_t pname,
>>>>>> +                               uint32_t valaddr, uint32_t vallen)
>>>>>> +{
>>>>>> +    char propname[64];
>>>>>> +    uint32_t ret = -1;
>>>>>> +    int proplen = 0;
>>>>>> +    const void *prop;
>>>>>> +
>>>>>> +    readstr(pname, propname);
>>>>>> +    if (vallen == sizeof(uint32_t) &&
>>>>>> +        ((strncmp(propname, "linux,rtas-base", 15) == 0) ||
>>>>>> +         (strncmp(propname, "linux,rtas-entry", 16) == 0))) {
>>>>>> +
>>>>>> +        sm->rtas_base = readuint32(valaddr);
>>>>>> +        prop = fdt_getprop_namelen(sm->fdt_blob,
>>>>>> +                                   
>>>>>> fdt_node_offset_by_phandle(sm->fdt_blob,
>>>>>> +                                                              nodeph),
>>>>>> +                                   propname, strlen(propname), 
>>>>>> &proplen);
>>>>>> +        if (proplen == vallen) {
>>>>>> +            *(uint32_t *) prop = cpu_to_be32(sm->rtas_base);
>>>>>> +            ret = proplen;
>>>>>> +        }
>>>>>
>>>>> Is there a particular reason to restrict this to the rtas properties,
>>>>> rather than just allowing the guest to fdt_setprop() something
>>>>> arbitrary?
>>>>
>>>> The FDT is flatten and I am not quite sure if libfdt can handle updating
>>>> properties if the length has changed.
>>>
>>> fdt_setprop() will handle updating properties with changed length (in
>>> fact there's a special fdt_setprop_inplace() optimized for the case
>>> where you don't need that).  It's not particularly efficient, but it
>>> should work fine for the cases we have here.  In fact, I think you're
>>> already relying on this for the code that adds the phandles to
>>> everything.
>>
>> Well, I used to add phandles before calling fdt_pack() so it is not exactly 
>> the same.
> 
> Ah, right, that's why adding the phandles worked.
> 
>>> One complication is that it can return FDT_ERR_NOSPACE if there isn't
>>> enough buffer for the increased thing.  We could either trap that,
>>> resize and retry, or we could leave a bunch of extra space.  The
>>> latter would be basically equivalent to not doing fdt_pack() on the
>>> blob in the nobios case.
>>
>>
>> This is what I ended up doing.
>>
>>
>>>> Also, more importantly, potentially property changes like this may have
>>>> to be reflected in the QEMU device tree so I allowed only the properties
>>>> which I know how to deal with.
>>>
>>> That's a reasonable concern, but the nice thing about not having SLOF
>>> is that there's only one copy of the device tree - the blob in qemu.
>>> So a setprop() on that is automatically a setprop() everywhere (this
>>> is another reason not to write the fdt into guest memory in the nobios
>>> case - it will become stale as soon as the client changes anything).
>>
>>
>> True to a degree. It is "setprop" to the current fdt blob which we do not
>> analyze after we build the fdt. We either need to do parse the tree before
>> we rebuild it as CAS so we do not lose the updates or do selective changes
>> to the QEMUs objects from the "setprop" handler (this is what I do
>> now).
> 
> Hrm.. do those setprops happen before CAS?

Yes, vmlinux/zimage call "setprop" for "linux,initrd-start",
"linux,initrd-end", "bootargs", "linux,stdout-path"; vmlinux sets
properties if linux,initrd-* came from r3/r4 and zImage sets properties
no matter how it discovered them - from r3/r4 or the device tree.

btw we write them as "cells" (==4bytes long) in qemu but vminux changes
them to 8 bytes and zImage keeps it 4 bytes. Not a problem but an
interesting fact, this is why I had to allow extending the properties in
"setprop" :)


>  I would have thought we'd
> call CAS before instantiating RTAS.

This is correct but I do not think the order is mandatory.


-- 
Alexey