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

[BALATON Zoltan]

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:

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