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

[BALATON Zoltan <balaton@eik.bme.hu>]

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