Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface

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

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

On Fri, Jun 04, 2021 at 03:50:28PM +0200, BALATON Zoltan wrote:
> On Fri, 4 Jun 2021, David Gibson wrote:
> > On Sun, May 30, 2021 at 07:33:01PM +0200, BALATON Zoltan wrote:
[snip]
> > > MorphOS checks the name property of the root node ("/") to decide what
> > > platform it runs on so we may need to be able to set this property on /
> > > where it should return "bplan,Pegasos2", therefore the above maybe should do
> > > getprop first and only generate name property if it's not set (or at least
> > > check if we're on the root node and allow setting name property there). (On
> > > Macs the root node is named "device-tree" and this was before found to be
> > > needed for MorphOS.)
> > 
> > Ah.  Hrm.  Have to think about what to do about that.
> 
> This is easy to fix, this seems to allow setting a name property or return a
> default:
> 
> > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> index b47bbd509d..746842593e 100644
> --- a/hw/ppc/vof.c
> +++ b/hw/ppc/vof.c
> @@ -163,14 +163,14 @@ static uint32_t vof_finddevice(const void *fdt,
> uint32_t nodeaddr)
>  static const void *getprop(const void *fdt, int nodeoff, const char *propname,
>                             int *proplen, bool *write0)
>  {
> -    const char *unit, *prop;
> +    const char *unit, *prop = fdt_getprop(fdt, nodeoff, propname, proplen);
> 
>      /*
>       * The "name" property is not actually stored as a property in the FDT,
>       * we emulate it by returning a pointer to the node's name and adjust
>       * proplen to include only the name but not the unit.
>       */
> -    if (strcmp(propname, "name") == 0) {
> +    if (!prop && strcmp(propname, "name") == 0) {
>          prop = fdt_get_name(fdt, nodeoff, proplen);
>          if (!prop) {
>              *proplen = 0;
> @@ -196,7 +196,7 @@ static const void *getprop(const void *fdt, int nodeoff, const char *propname,
>      if (write0) {
>          *write0 = false;
>      }
> -    return fdt_getprop(fdt, nodeoff, propname, proplen);
> +    return prop;
>  }

Kind of a hack, but it'll do for now.

> This allows adding a name property to "/" different from the default but
> this does not yet fix MorphOS booting with VOF on pegasos2. I think it tries
> to query name on / and check if it's called "device-tree" in which case it
> assumes Mac hardware otherwise it goes with pegasos2 so even if we return
> nothing for name it would not matter in this case as we don't use VOF on
> Mac. If we wanted that then this would become a problem so it could be fixed
> now in advance just in case other guests may need it.
> 
> > > Other than the above two problems, I've found that getting the device tree
> > > from vof returns it in reverse order compared to the board firmware if I add
> > > it the expected order. This may or may not be a problem but to avoid it I
> > > can build the tree in reverse order then it comes out right so unless
> > > there's an easy fix this should not cause a problem but may worth a comment
> > > somewhere.
> > 
> > The order of things in the device tree *should* never matter.  If it
> > does, that's definitely a client bug... but of course that doesn't
> > necessarily mean we won't have to work around it in practice.
> 
> I don't know if it matters or not but having the device tree in the same
> order as the firmware ROM helps with comparing it for debugging but I've
> found I can solve this by building the tree in reverse order so no changes
> to VOF is needed for this, just thought adding a comment somewhere may
> clarify it but it's not really a problem.
> 
> I still don't know what's MorphOS is missing, I've tried adding almost all
> misssing properties, checked what hardware is init by the firmware and tried
> to do the same in board reset code and even after that MorphOS still takes a
> different route with VOF and crashes but boots with the board firmware. I'm
> now thinking it may be either different memory organisation or the missing
> name properties that are not returned by nextprop in VOF so they are only
> appearing when explicitely queried whereas with the board firmware they are
> present as properties. With the above patch I could explicitely set it on
> nodes and test if that makes a difference.
> 
> I got to this because adding more missing props or init more devices did not
> make a difference so I'm guessing it may be something else then and the only
> difference I can see compared to board firmware are the different memory
> ranges in claimed (VOF puts itself to 0 for example); and the missing name
> and additional phandle props in the device tree. MorphOS copies the whole
> device tree on startup then later it uses this copy of the device tree after
> shutting down OF with quiesce. I can imagine it may use some name props like
> that on the cpu node without checking assuming it's always there and if
> we're missing that it may cause a NULL dereference. I have no better idea
> what else could be missing so I'll test this next. If it helps I can try to
> come up with a patch to VOF to return these name props or allow setting them
> as above.
> 
> Regards,
> BALATON Zoltan
> 

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