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

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

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

On Fri, Jul 09, 2021 at 12:22:15AM +0200, BALATON Zoltan wrote:
> On Thu, 8 Jul 2021, David Gibson wrote:
> > On Fri, Jun 25, 2021 at 03:51:55PM +1000, Alexey Kardashevskiy wrote:
> > [snip]
> > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > new file mode 100644
> > > index 000000000000..a17fd9d2fe94
> > > --- /dev/null
> > > +++ b/hw/ppc/vof.c
> > [snip]
> > > +static int path_offset(const void *fdt, const char *path)
> > > +{
> > > +    g_autofree char *p = NULL;
> > > +    char *at;
> > > +
> > > +    /*
> > > +     * https://www.devicetree.org/open-firmware/bindings/ppc/release/ppc-2_1.html#HDR16
> > > +     *
> > > +     * "Conversion from numeric representation to text representation shall use
> > > +     * the lower case forms of the hexadecimal digits in the range a..f,
> > > +     * suppressing leading zeros".
> > 
> > Huh... that suggests that Zoltan's firmware which passes a caps hex
> > and expects it to work is doing the wrong thing.  We still need to
> > accomodate it, though.
> 
> It's Linux which passes both upper and lower case variants (and all that a
> few line apart in the same file). The Pegasos2 SmartFirmware displays these
> with upper case address parts but accepts both upper and lower case. Here's
> a device tree dump from the original board firmware:

Right, sorry.  s/Zoltan's firmware/Zoltan's obscure platform Linux/

> https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2/attach/PegasosII_OFW-Tree.txt
> 
> Apple's OpenFirmware seems to have lower case addresses:
> 
> http://nandra.segv.jp/NetBSD/G4.dump-device-tree.txt
> 
> but maybe it also accepts upper case? I can't try that now.
> 
> This works for pegasos2 guests I've tried but maybe only because the only
> problematic query is /pci@80000000/ide@C,1. If something wanted to get
> /pci@C0000000/isa@C then that might fail but I think most devices are on
> /pci@80000000 so this problem is unlikely to happen. The most correct way
> would be to convert all parts between @ and / or \0 to lower case but either
> this or the changed version in v23 which does strrcht('@') works for the
> cases I have.
> 
> > [snip]
> > > +
> > > +static void vof_instantiate_rtas(Error **errp)
> > > +{
> > > +    error_setg(errp, "The firmware should have instantiated RTAS");
> > 
> > Since this always fails...
> > 
> > > +}
> > > +
> > > +static uint32_t vof_call_method(MachineState *ms, Vof *vof, uint32_t methodaddr,
> > > +                                uint32_t ihandle, uint32_t param1,
> > > +                                uint32_t param2, uint32_t param3,
> > > +                                uint32_t param4, uint32_t *ret2)
> > > +{
> > > +    uint32_t ret = -1;
> > > +    char method[VOF_MAX_METHODLEN] = "";
> > > +    OfInstance *inst;
> > > +
> > > +    if (!ihandle) {
> > > +        goto trace_exit;
> > > +    }
> > > +
> > > +    inst = (OfInstance *)g_hash_table_lookup(vof->of_instances,
> > > +                                             GINT_TO_POINTER(ihandle));
> > > +    if (!inst) {
> > > +        goto trace_exit;
> > > +    }
> > > +
> > > +    if (readstr(methodaddr, method, sizeof(method))) {
> > > +        goto trace_exit;
> > > +    }
> > > +
> > > +    if (strcmp(inst->path, "/") == 0) {
> > > +        if (strcmp(method, "ibm,client-architecture-support") == 0) {
> > > +            Object *vmo = object_dynamic_cast(OBJECT(ms), TYPE_VOF_MACHINE_IF);
> > > +
> > > +            if (vmo) {
> > > +                VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
> > > +
> > > +                g_assert(vmc->client_architecture_support);
> > > +                ret = vmc->client_architecture_support(ms, first_cpu, param1);
> > > +            }
> > > +
> > > +            *ret2 = 0;
> > > +        }
> > > +    } else if (strcmp(inst->path, "/rtas") == 0) {
> > > +        if (strcmp(method, "instantiate-rtas") == 0) {
> > 
> > ... why do you even need to handle it here?
> 
> This message has helped to catch problem with instantiate-rtas so it's
> useful to have here even if normally it would not get here. I don't remember
> what was the problem, maybe too small rtas-size or similar but getting a
> message instead of crashing did point to the problem.
> 
> 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 --]