qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Gibson
Subject: Re: [PATCH qemu v20] spapr: Implement Open Firmware client interface
Date: Mon, 7 Jun 2021 13:02:56 +1000

On Fri, Jun 04, 2021 at 03:27:12PM +0200, BALATON Zoltan wrote:
> On Fri, 4 Jun 2021, David Gibson wrote:
> > On Tue, Jun 01, 2021 at 04:12:44PM +0200, BALATON Zoltan wrote:
> > > On Tue, 1 Jun 2021, Alexey Kardashevskiy wrote:
> > > > On 31/05/2021 23:07, BALATON Zoltan wrote:
> > > > > On Sun, 30 May 2021, BALATON Zoltan wrote:
> > > > > > On Thu, 20 May 2021, Alexey Kardashevskiy wrote:
> > > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..a283b7d251a7
> > > > > > > --- /dev/null
> > > > > > > +++ b/hw/ppc/vof.c
> > > > > > > @@ -0,0 +1,1021 @@
> > > > > > > +/*
> > > > > > > + * QEMU PowerPC Virtual Open Firmware.
> > > > > > > + *
> > > > > > > + * This implements client interface from OpenFirmware
> > > > > > > IEEE1275 on the QEMU
> > > > > > > + * side to leave only a very basic firmware in the VM.
> > > > > > > + *
> > > > > > > + * Copyright (c) 2021 IBM Corporation.
> > > > > > > + *
> > > > > > > + * SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "qemu-common.h"
> > > > > > > +#include "qemu/timer.h"
> > > > > > > +#include "qemu/range.h"
> > > > > > > +#include "qemu/units.h"
> > > > > > > +#include "qapi/error.h"
> > > > > > > +#include <sys/ioctl.h>
> > > > > > > +#include "exec/ram_addr.h"
> > > > > > > +#include "exec/address-spaces.h"
> > > > > > > +#include "hw/ppc/vof.h"
> > > > > > > +#include "hw/ppc/fdt.h"
> > > > > > > +#include "sysemu/runstate.h"
> > > > > > > +#include "qom/qom-qobject.h"
> > > > > > > +#include "trace.h"
> > > > > > > +
> > > > > > > +#include <libfdt.h>
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * OF 1275 "nextprop" description suggests is it 32 bytes max but
> > > > > > > + * LoPAPR defines "ibm,query-interrupt-source-number" which
> > > > > > > is 33 chars long.
> > > > > > > + */
> > > > > > > +#define OF_PROPNAME_LEN_MAX 64
> > > > > > > +
> > > > > > > +#define VOF_MAX_PATH        256
> > > > > > > +#define VOF_MAX_SETPROPLEN  2048
> > > > > > > +#define VOF_MAX_METHODLEN   256
> > > > > > > +#define VOF_MAX_FORTHCODE   256
> > > > > > > +#define VOF_VTY_BUF_SIZE    256
> > > > > > > +
> > > > > > > +typedef struct {
> > > > > > > +    uint64_t start;
> > > > > > > +    uint64_t size;
> > > > > > > +} OfClaimed;
> > > > > > > +
> > > > > > > +typedef struct {
> > > > > > > +    char *path; /* the path used to open the instance */
> > > > > > > +    uint32_t phandle;
> > > > > > > +} OfInstance;
> > > > > > > +
> > > > > > > +#define VOF_MEM_READ(pa, buf, size) \
> > > > > > > +    address_space_read_full(&address_space_memory, \
> > > > > > > +    (pa), MEMTXATTRS_UNSPECIFIED, (buf), (size))
> > > > > > > +#define VOF_MEM_WRITE(pa, buf, size) \
> > > > > > > +    address_space_write(&address_space_memory, \
> > > > > > > +    (pa), MEMTXATTRS_UNSPECIFIED, (buf), (size))
> > > > > > > +
> > > > > > > +static int readstr(hwaddr pa, char *buf, int size)
> > > > > > > +{
> > > > > > > +    if (VOF_MEM_READ(pa, buf, size) != MEMTX_OK) {
> > > > > > > +        return -1;
> > > > > > > +    }
> > > > > > > +    if (strnlen(buf, size) == size) {
> > > > > > > +        buf[size - 1] = '\0';
> > > > > > > +        trace_vof_error_str_truncated(buf, size);
> > > > > > > +        return -1;
> > > > > > > +    }
> > > > > > > +    return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static bool cmpservice(const char *s, unsigned nargs, unsigned 
> > > > > > > nret,
> > > > > > > +                       const char *s1, unsigned nargscheck,
> > > > > > > unsigned nretcheck)
> > > > > > > +{
> > > > > > > +    if (strcmp(s, s1)) {
> > > > > > > +        return false;
> > > > > > > +    }
> > > > > > > +    if ((nargscheck && (nargs != nargscheck)) ||
> > > > > > > +        (nretcheck && (nret != nretcheck))) {
> > > > > > > +        trace_vof_error_param(s, nargscheck, nretcheck, nargs, 
> > > > > > > nret);
> > > > > > > +        return false;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    return true;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void prop_format(char *tval, int tlen, const void *prop, 
> > > > > > > int len)
> > > > > > > +{
> > > > > > > +    int i;
> > > > > > > +    const unsigned char *c;
> > > > > > > +    char *t;
> > > > > > > +    const char bin[] = "...";
> > > > > > > +
> > > > > > > +    for (i = 0, c = prop; i < len; ++i, ++c) {
> > > > > > > +        if (*c == '\0' && i == len - 1) {
> > > > > > > +            strncpy(tval, prop, tlen - 1);
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +        if (*c < 0x20 || *c >= 0x80) {
> > > > > > > +            break;
> > > > > > > +        }
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    for (i = 0, c = prop, t = tval; i < len; ++i, ++c) {
> > > > > > > +        if (t >= tval + tlen - sizeof(bin) - 1 - 2 - 1) {
> > > > > > > +            strcpy(t, bin);
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +        if (i && i % 4 == 0 && i != len - 1) {
> > > > > > > +            strcat(t, " ");
> > > > > > > +            ++t;
> > > > > > > +        }
> > > > > > > +        t += sprintf(t, "%02X", *c & 0xFF);
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int get_path(const void *fdt, int offset, char *buf, int 
> > > > > > > len)
> > > > > > > +{
> > > > > > > +    int ret;
> > > > > > > +
> > > > > > > +    ret = fdt_get_path(fdt, offset, buf, len - 1);
> > > > > > > +    if (ret < 0) {
> > > > > > > +        return ret;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    buf[len - 1] = '\0';
> > > > > > > +
> > > > > > > +    return strlen(buf) + 1;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int phandle_to_path(const void *fdt, uint32_t ph,
> > > > > > > char *buf, int len)
> > > > > > > +{
> > > > > > > +    int ret;
> > > > > > > +
> > > > > > > +    ret = fdt_node_offset_by_phandle(fdt, ph);
> > > > > > > +    if (ret < 0) {
> > > > > > > +        return ret;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    return get_path(fdt, ret, buf, len);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static uint32_t vof_finddevice(const void *fdt, uint32_t 
> > > > > > > nodeaddr)
> > > > > > > +{
> > > > > > > +    char fullnode[VOF_MAX_PATH];
> > > > > > > +    uint32_t ret = -1;
> > > > > > > +    int offset;
> > > > > > > +
> > > > > > > +    if (readstr(nodeaddr, fullnode, sizeof(fullnode))) {
> > > > > > > +        return (uint32_t) ret;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    offset = fdt_path_offset(fdt, fullnode);
> > > > > > > +    if (offset >= 0) {
> > > > > > > +        ret = fdt_get_phandle(fdt, offset);
> > > > > > > +    }
> > > > > > > +    trace_vof_finddevice(fullnode, ret);
> > > > > > > +    return (uint32_t) ret;
> > > > > > > +}
> > > > > > 
> > > > > > The Linux init function that runs on pegasos2 here:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/kernel/prom_init.c?h=v4.14.234#n2658
> > > > > > 
> > > > > > calls finddevice once with isa@c and next with isa@C (small and
> > > > > > capital C) both of which works with the board firmware but with
> > > > > > vof the comparison is case sensitive and one of these fails so I
> > > > > > can't make it work. I don't know if this is a problem in libfdt
> > > > > > or the vof_finddevice above should do something else to get case
> > > > > > insensitive comparison.
> > > > > 
> > > > > This fixes the issue with Linux but I'm not sure if there's any
> > > > > better solution or would it break anything else.
> > > > 
> > > > The bit after "@" is an address and needs to be case insensitive and
> > > > I'll fix this indeed. I'm not so sure about the part before "@", I
> > > > cannot imagine what could break if I made search insensitive to case. Hm
> > > > :-/
> > > 
> > > Fixing the match in the address part is probably enough as the name sent 
> > > by
> > > guests is probably always lower case
> > 
> > I'm confused, I thought you just said that it looked for both isa@c
> > and isa@C, which seems to contradict guests always using lower case.
> 
> I mean the part before the @ sign (that is the name part, "isa" above) is
> always lower case. I haven't seen guests trying to query that with other
> than lower case

Ah, I see.  Yes, I think you can count on that, because I believe even
in traditional OF the part before the @ *is* case-sensitive.  At least
there are certainly conventions about how the vendor is capitalized,
so I assume it is.

> but the part after @ can be different even in the same guest
> code just a few lines apart as in the Linux kernel. So fixing the comparison
> to e.g. do toupper in the address part after @ should work I think even if
> we continue to do case sensitive comparison in the name part. Alexey said
> he'll fix that so there's no problem.

Yeah, that will probably work fine in practice.  It's not technically
correct in all cases, because how you're supposed to do the comparison
depends on the bus type.

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]