Re: [PATCH qemu v2] spapr: Kill SLOF

[David Gibson <david@gibson.dropbear.id.au>]

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

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?

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.

> 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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]