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
signature.asc
Description: PGP signature