Re: [PATCH qemu v2] spapr: Kill SLOF

[Alexey Kardashevskiy]

On 09/01/2020 15:07, David Gibson wrote:
> On Wed, Jan 08, 2020 at 03:07:41PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> 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.
> 
> Ok, and those setprops happen before CAS?

Yes.

> In a sense this is kind of a fundamental problem with rebuilding the
> whole DT at CAS time.  Except that strictly speaking it's a problem
> even without that: we just get away with it by accident because CAS
> isn't likely to change the same things that guest setprops do.

> It's still basically unsynchronized mutations by two parties to a
> shared data structure.

True... We may end up not having FDT at all and reuse QOM objects for
that, can even use hashes of QObject pointers as phandles :)



>> 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.
> 
> Hm, right.
> 

-- 
Alexey