Re: [PATCH qemu v2] spapr: Kill SLOF

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

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

On Thu, Jan 09, 2020 at 05:31:24PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> 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 :)

Hm, interesting idea.  I suspect the QOM hierarchy won't be quite
similar enough to the fdt hierarchy (or at least not guaranteed to be)
to make it work, but it's worth thinking about at least.

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