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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 05/12/2020 05:32, Greg Kurz wrote:
> On Tue, 13 Oct 2020 13:19:11 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> The PAPR platform which describes an OS environment that's presented by
>> a combination of a hypervisor and firmware. The features it specifies
>> require collaboration between the firmware and the hypervisor.
>>
>> Since the beginning, the runtime component of the firmware (RTAS) has
>> been implemented as a 20 byte shim which simply forwards it to
>> a hypercall implemented in qemu. The boot time firmware component is
>> SLOF - but a build that's specific to qemu, and has always needed to be
>> updated in sync with it. Even though we've managed to limit the amount
>> of runtime communication we need between qemu and SLOF, there's some,
>> and it has become increasingly awkward to handle as we've implemented
>> new features.
>>
>> This implements a boot time OF client interface (CI) which is
>> enabled by a new "x-vof" pseries machine option (stands for "Virtual Open
>> Firmware). When enabled, QEMU implements the custom H_OF_CLIENT hcall
>> which implements Open Firmware Client Interface (OF CI). This allows
>> using a smaller stateless firmware which does not have to manage
>> the device tree.
>>
>> The new "vof.bin" firmware image is included with source code under
>> pc-bios/. It also includes RTAS blob.
>>
>> This implements a handful of CI methods just to get -kernel/-initrd
>> working. In particular, this implements the device tree fetching and
>> simple memory allocator - "claim" (an OF CI memory allocator) and updates
>> "/memory@0/available" to report the client about available memory.
>>
>> This implements changing some device tree properties which we know how
>> to deal with, the rest is ignored. To allow changes, this skips
>> fdt_pack() when x-vof=on as not packing the blob leaves some room for
>> appending.
>>
>> In absence of SLOF, this assigns phandles to device tree nodes to make
>> device tree traversing work.
>>
>> When x-vof=on, this adds "/chosen" every time QEMU (re)builds a tree.
>>
>> This adds basic instances support which are managed by a hash map
>> ihandle -> [phandle].
>>
>> Before the guest started, the used memory is:
>> 0..4000 - the initial firmware
>> 10000..180000 - stack
>>
>> This OF CI does not implement "interpret".
>>
>> With this basic support, this can only boot into kernel directly.
> 
> Maybe worth erroring out if -kernel is missing then.


In my working tree, this new small firmware can open a disk, find/load 
grub and jump to it, all using OF and without drivers/kernel. It got 
nak'd straight away though so you may be right :)



> 
> eg.
> 
> void spapr_of_client_machine_init(SpaprMachineState *spapr)
> {
>      if (!spapr->kernel_size) {
>          error_report("The 'x-vof' machine property requires '-kernel'");
>          exit(EXIT_FAILURE);
>      }
>      spapr_register_hypercall(KVMPPC_H_OF_CLIENT, spapr_h_of_client);
> }
> 
>> However this is just enough for the petitboot kernel and initradmdisk to
>> boot from any possible source. Note this requires reasonably recent guest
>> kernel with:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df5be5be8735
>>
> 
> FWIW it worked flawlessly with the vmlinuz and initramfs of a recent
> rhel8 guest.
> 
> The patch is huge and I never find time to do a full review...  so instead
> of postponing again and again, I post what I have noted so far.

This is a bare minimum really...


> 
> Please find some comments below.
> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
> 
> [...]
> 
>> @@ -1646,22 +1650,36 @@ static void spapr_machine_reset(MachineState *machine)
>>   
>>       fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
>>   
>> -    rc = fdt_pack(fdt);
>> -
>> -    /* Should only fail if we've built a corrupted tree */
>> -    assert(rc == 0);
>> -
>> -    /* Load the fdt */
>>       qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>> -    cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
>> +
>>       g_free(spapr->fdt_blob);
>>       spapr->fdt_size = fdt_totalsize(fdt);
>>       spapr->fdt_initial_size = spapr->fdt_size;
>>       spapr->fdt_blob = fdt;
> 
> It is a bit confusing that these are set here and...
> 
>>   
>>       /* Set up the entry state */
>> -    spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT, 0, fdt_addr, 0);
>>       first_ppc_cpu->env.gpr[5] = 0;
>> +    if (spapr->vof) {
>> +        target_ulong stack_ptr = 0;
>> +
>> +        spapr_setup_of_client(spapr, &stack_ptr);
>> +        spapr_of_client_dt_finalize(spapr);
>> +        spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT,
>> +                                  stack_ptr, spapr->initrd_base,
>> +                                  spapr->initrd_size);
>> +    } else {
>> +        /* Load the fdt */
>> +        rc = fdt_pack(spapr->fdt_blob);
>> +        /* Should only fail if we've built a corrupted tree */
>> +        assert(rc == 0);
>> +
>> +        spapr->fdt_size = fdt_totalsize(spapr->fdt_blob);
>> +        spapr->fdt_initial_size = spapr->fdt_size;
> 
> ... overwritten there. I guess this is because fdt_pack() has an
> impact on fdt_totalsize(), right ? Could this be consolidated
> in an helper that optionally calls fdt_pack() ?
> 
>> +        cpu_physical_memory_write(fdt_addr, spapr->fdt_blob, spapr->fdt_size);
>> +
>> +        spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT,
>> +                                  0, fdt_addr, 0);
>> +    }

I can set spapr->fdt_initial_size here, once, and simplify other bits 
above as well.


>>   
>>       spapr->fwnmi_system_reset_addr = -1;
>>       spapr->fwnmi_machine_check_addr = -1;
> 
> [...]
> 
>> @@ -3296,6 +3332,11 @@ static void spapr_instance_init(Object *obj)
>>                                       stringify(KERNEL_LOAD_ADDR)
>>                                       " for -kernel is the default");
>>       spapr->kernel_addr = KERNEL_LOAD_ADDR;
>> +    object_property_add_bool(obj, "x-vof", spapr_get_vof, spapr_set_vof);
>> +    object_property_set_description(obj, "x-vof",
>> +                                    "Enable Virtual Open Firmware");
>> +    spapr->vof = false;
> 
> We usually don't initialize to false or 0 since QOM already
> does memset(0) on the instance.


True, this is a leftover, will fix.

> 
>> +
>>       /* The machine class defines the default interrupt controller mode */
>>       spapr->irq = smc->irq;
>>       object_property_add_str(obj, "ic-mode", spapr_get_ic_mode,
> 
> [...]
> 
>> diff --git a/hw/ppc/spapr_of_client.c b/hw/ppc/spapr_of_client.c
>> new file mode 100644
>> index 000000000000..04b1543696b0
>> --- /dev/null
>> +++ b/hw/ppc/spapr_of_client.c
>> @@ -0,0 +1,1011 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include <sys/ioctl.h>
>> +#include "qapi/error.h"
>> +#include "hw/ppc/spapr.h"
>> +#include "hw/ppc/spapr_vio.h"
>> +#include "hw/ppc/fdt.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qom/qom-qobject.h"
>> +#include "trace.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
>> +
>> +/* Copied from SLOF, and 4K is definitely not enough for GRUB */
>> +#define OF_STACK_SIZE       0x8000
>> +
>> +/* 0..10000 is reserved for the VOF fw */
>> +#define OF_STACK_ADDR       0x10000
>> +
>> +#define ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
>> +
>> +typedef struct {
>> +    uint64_t start;
>> +    uint64_t size;
>> +} SpaprOfClaimed;
>> +
>> +typedef struct {
>> +    char *params;
>> +    char *path; /* the path used to open the instance */
>> +    uint32_t phandle;
>> +} SpaprOfInstance;
>> +
>> +/* Defined as Big Endian */
>> +struct prom_args {
>> +    uint32_t service;
>> +    uint32_t nargs;
>> +    uint32_t nret;
>> +    uint32_t args[10];
>> +} QEMU_PACKED;
>> +
> 
> The QEMU_PACKED breaks build with gcc-10.2.1-6.fc32.x86_64:
> 
> ../../hw/ppc/spapr_of_client.c: In function ‘spapr_h_of_client’:
> ../../hw/ppc/spapr_of_client.c:793:35: error: taking address of packed member of ‘struct prom_args’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
>    793 |                                   &pargs.args[nargs + 1]);
>        |                                   ^~~~~~~~~~~~~~~~~~~~~~
> ../../hw/ppc/spapr_of_client.c:800:38: error: taking address of packed member of ‘struct prom_args’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
>    800 |                                      &pargs.args[nargs + 1]);
>        |                                      ^~~~~~~~~~~~~~~~~~~~~~
> 
> Fixable by dropping QEMU_PACKED and adding:
> 
> QEMU_BUILD_BUG_ON(sizeof(struct prom_args) != 13 * 4);


or (pargs.args + nargs + 1) and keep QEMU_PACKED.


> 
>> +static void readstr(hwaddr pa, char *buf, int size)
>> +{
>> +    cpu_physical_memory_read(pa, buf, size);
>> +    if (buf[size - 1] != '\0') {
>> +        buf[size - 1] = '\0';
>> +        if (strlen(buf) == size - 1) {
>> +            trace_spapr_of_client_error_str_truncated(buf, size);
>> +        }
>> +    }
>> +}
>> +
>> +static bool cmpservice(const char *s, size_t len,
>> +                       unsigned nargs, unsigned nret,
>> +                       const char *s1, size_t len1,
>> +                       unsigned nargscheck, unsigned nretcheck)
>> +{
>> +    if (strcmp(s, s1)) {
>> +        return false;
>> +    }
>> +    if ((nargscheck && (nargs != nargscheck)) ||
>> +        (nretcheck && (nret != nretcheck))) {
> 
> Parenthesitis : != has precedence over &&.


I love my braces :)

> 
>> +        trace_spapr_of_client_error_param(s, nargscheck, nretcheck, nargs,
>> +                                          nret);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static void split_path(const char *fullpath, char **node, char **unit,
>> +                       char **part)
>> +{
>> +    const char *c, *p = NULL, *u = NULL;
>> +
>> +    *node = *unit = *part = NULL;
>> +
>> +    if (fullpath[0] == '\0') {
>> +        *node = g_strdup(fullpath);
>> +        return;
>> +    }
>> +
>> +    for (c = fullpath + strlen(fullpath) - 1; c > fullpath; --c) {
>> +        if (*c == '/') {
>> +            break;
>> +        }
>> +        if (*c == ':') {
>> +            p = c + 1;
>> +            continue;
>> +        }
>> +        if (*c == '@') {
>> +            u = c + 1;
>> +            continue;
>> +        }
>> +    }
>> +
>> +    if (p && u && p < u) {
>> +        p = NULL;
>> +    }
>> +
>> +    if (u && p) {
>> +        *node = g_strndup(fullpath, u - fullpath - 1);
>> +        *unit = g_strndup(u, p - u - 1);
>> +        *part = g_strdup(p);
>> +    } else if (!u && p) {
>> +        *node = g_strndup(fullpath, p - fullpath - 1);
>> +        *part = g_strdup(p);
>> +    } else if (!p && u) {
>> +        *node = g_strndup(fullpath, u - fullpath - 1);
>> +        *unit = g_strdup(u);
>> +    } else {
>> +        *node = g_strdup(fullpath);
>> +    }
> 
> It looks like this function could be simplified by using g_strsplit_set().


Nah, I want not just split but also know what each part is - a node or a 
unit or a partition.

> 
>> +}
>> +
>> +static void prop_format(char *tval, int tlen, const void *prop, int len)
>> +{
>> +    int i;
>> +    const 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) {
> 
> This breaks build with gcc-10.2.1-6.fc32.x86_64:
> 
> ../../hw/ppc/spapr_of_client.c: In function ‘prop_format’:
> ../../hw/ppc/spapr_of_client.c:131:29: error: comparison is always false due to limited range of data type [-Werror=type-limits]
>    131 |         if (*c < 0x20 || *c >= 0x80) {
>        |                             ^~
> 
> Fixable by making 'c' a 'const unsigned char'.


Done.


> 
>> +            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 of_client_fdt_path_offset(const void *fdt, const char *node,
>> +                                     const char *unit)
>> +{
>> +    int offset;
>> +
>> +    offset = fdt_path_offset(fdt, node);
>> +
>> +    if (offset < 0 && unit) {
>> +        char *tmp = g_strdup_printf("%s@%s", node, unit);
>> +
>> +        offset = fdt_path_offset(fdt, tmp);
>> +        g_free(tmp);
> 
> CODING_STYLE advocates the use of g_autofree instead of manually
> calling g_free().

Oh. Ok.


>> +    }
>> +
>> +    return offset;
>> +}
>> +
>> +static uint32_t of_client_finddevice(const void *fdt, uint32_t nodeaddr)
>> +{
>> +    char *node, *unit, *part;
> 
> If you do this:
> 
>      g_autofree *node = NULL, *unit = NULL, *part = NULL;


Did you mean
        g_autofree char *node = NULL, *unit = NULL, *part = NULL;
?

> 
>> +    char fullnode[1024];
>> +    uint32_t ret = -1;
>> +    int offset;
>> +
>> +    readstr(nodeaddr, fullnode, sizeof(fullnode));
>> +
>> +    split_path(fullnode, &node, &unit, &part);
>> +    offset = of_client_fdt_path_offset(fdt, node, unit);
>> +    if (offset >= 0) {
>> +        ret = fdt_get_phandle(fdt, offset);
>> +    }
>> +    trace_spapr_of_client_finddevice(fullnode, ret);
>> +    g_free(node);
>> +    g_free(unit);
>> +    g_free(part);
> 
> You can drop these ^^

Ok.

> You should be able to work out something similar with g_auto(GStrv) if
> you decide to use g_strsplit_set().
> 
>> +    return (uint32_t) ret;
>> +}
>> +
> 
> [...]
> 
>> +static uint32_t of_client_setprop(SpaprMachineState *spapr,
>> +                                  uint32_t nodeph, uint32_t pname,
>> +                                  uint32_t valaddr, uint32_t vallen)
>> +{
>> +    char propname[OF_PROPNAME_LEN_MAX + 1];
>> +    uint32_t ret = -1;
>> +    int offset;
>> +    char trval[64] = "";
>> +
>> +    readstr(pname, propname, sizeof(propname));
>> +    /*
>> +     * We only allow changing properties which we know how to update on
>> +     * the QEMU side.
>> +     */
>> +    if (vallen == sizeof(uint32_t)) {
>> +        uint32_t val32 = ldl_be_phys(first_cpu->as, valaddr);
>> +
>> +        if ((strcmp(propname, "linux,rtas-base") == 0) ||
>> +            (strcmp(propname, "linux,rtas-entry") == 0)) {
> 
> What about :
> 
>          if (!strcmp(propname, "linux,rtas-base") ||
>              !strcmp(propname, "linux,rtas-entry")) {



[fstn1-p1 qemu-killslof]$ git grep 'strcmp.*==.*0' | wc -l
426
[fstn1-p1 qemu-killslof]$ git grep '!strcmp' | wc -l
447

My team is losing but not by much :) I prefer "==" (literally - "equal) 
to  "!" with "cmp" which is "does not compare" (makes little sense).


> 
>> +            spapr->rtas_base = val32;
>> +        } else if (strcmp(propname, "linux,initrd-start") == 0) {
>> +            spapr->initrd_base = val32;
>> +        } else if (strcmp(propname, "linux,initrd-end") == 0) {
>> +            spapr->initrd_size = val32 - spapr->initrd_base;
>> +        } else {
>> +            goto trace_exit;
>> +        }
> 
> [...]
> 
>> +static uint32_t spapr_of_client_open(SpaprMachineState *spapr, const char *path)
>> +{
>> +    int offset;
>> +    uint32_t ret = 0;
>> +    SpaprOfInstance *inst = NULL;
>> +    char *node, *unit, *part;
>> +
>> +    if (spapr->of_instance_last == 0xFFFFFFFF) {
>> +        /* We do not recycle ihandles yet */
>> +        goto trace_exit;
> 
> And g_free() is passed uninitialized pointers.
> 
> A typical use case for the g_auto magic.

g_autofree, you mean?


> 
>> +    }
>> +
>> +    split_path(path, &node, &unit, &part);
>> +
>> +    offset = of_client_fdt_path_offset(spapr->fdt_blob, node, unit);
>> +    if (offset < 0) {
>> +        trace_spapr_of_client_error_unknown_path(path);
>> +        goto trace_exit;
>> +    }
>> +
>> +    inst = g_new0(SpaprOfInstance, 1);
>> +    inst->phandle = fdt_get_phandle(spapr->fdt_blob, offset);
>> +    g_assert(inst->phandle);
>> +    ++spapr->of_instance_last;
>> +
>> +    inst->path = g_strdup(path);
>> +    inst->params = part;
>> +    g_hash_table_insert(spapr->of_instances,
>> +                        GINT_TO_POINTER(spapr->of_instance_last),
>> +                        inst);
>> +    ret = spapr->of_instance_last;
>> +
>> +trace_exit:
>> +    trace_spapr_of_client_open(path, inst ? inst->phandle : 0, ret);
>> +    g_free(node);
>> +    g_free(unit);
> 
> If you don't switch to g_auto, it seems you should at least add:
> 
>      g_free(part);
> 
>> +
>> +    return ret;
>> +}
>> +
>> +static uint32_t of_client_open(SpaprMachineState *spapr, uint32_t pathaddr)
>> +{
>> +    char path[256];
>> +
>> +    readstr(pathaddr, path, sizeof(path));
>> +
>> +    return spapr_of_client_open(spapr, path);
>> +}
>> +
>> +static void of_client_close(SpaprMachineState *spapr, uint32_t ihandle)
>> +{
>> +    if (!g_hash_table_remove(spapr->of_instances, GINT_TO_POINTER(ihandle))) {
>> +        trace_spapr_of_client_error_unknown_ihandle_close(ihandle);
>> +    }
>> +}
>> +
>> +static uint32_t of_client_instance_to_package(SpaprMachineState *spapr,
>> +                                              uint32_t ihandle)
>> +{
>> +    gpointer instp = g_hash_table_lookup(spapr->of_instances,
>> +                                         GINT_TO_POINTER(ihandle));
>> +    uint32_t ret = -1;
>> +
>> +    if (instp) {
>> +        ret = ((SpaprOfInstance *)instp)->phandle;
>> +    }
>> +    trace_spapr_of_client_instance_to_package(ihandle, ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static uint32_t of_client_package_to_path(const void *fdt, uint32_t phandle,
>> +                                          uint32_t buf, uint32_t len)
>> +{
>> +    uint32_t ret = -1;
>> +    char tmp[256] = "";
>> +
>> +    if (0 == fdt_get_path(fdt, fdt_node_offset_by_phandle(fdt, phandle), tmp,
>> +                          sizeof(tmp))) {
> 
> Quite an usual way to check for nullity. Any reason not to
> just s/0 == /!/ and save some indentation ?


Ok.

> 
>> +        tmp[sizeof(tmp) - 1] = 0;
>> +        ret = MIN(len, strlen(tmp) + 1);
>> +        cpu_physical_memory_write(buf, tmp, ret);
>> +    }
>> +
>> +    trace_spapr_of_client_package_to_path(phandle, tmp, ret);
>> +
>> +    return ret;
>> +}
>> +
>> +static uint32_t of_client_instance_to_path(SpaprMachineState *spapr,
>> +                                           uint32_t ihandle, uint32_t buf,
>> +                                           uint32_t len)
>> +{
>> +    uint32_t ret = -1;
>> +    uint32_t phandle = of_client_instance_to_package(spapr, ihandle);
>> +    char tmp[256] = "";
>> +
>> +    if (phandle != -1) {
>> +        if (0 == fdt_get_path(spapr->fdt_blob,
> 
> ditto

ok!


> 
>> +                              fdt_node_offset_by_phandle(spapr->fdt_blob,
>> +                                                         phandle),
>> +                              tmp, sizeof(tmp))) {
>> +            tmp[sizeof(tmp) - 1] = 0;
>> +            ret = MIN(len, strlen(tmp) + 1);
>> +            cpu_physical_memory_write(buf, tmp, ret);
>> +        }
>> +    }
>> +    trace_spapr_of_client_instance_to_path(ihandle, phandle, tmp, ret);
>> +
>> +    return ret;
>> +}
>> +
> 
> [...]
> 
>> +static uint32_t of_client_call_method(SpaprMachineState *spapr,
>> +                                      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[256] = "";
>> +    SpaprOfInstance *inst = NULL;
> 
> It doesn't seem that inst needs to be initialized.

True.

> 
>> +
>> +    if (!ihandle) {
>> +        goto trace_exit;
>> +    }
>> +
>> +    inst = (SpaprOfInstance *) g_hash_table_lookup(spapr->of_instances,
>> +                                                   GINT_TO_POINTER(ihandle));
>> +    if (!inst) {
>> +        goto trace_exit;
>> +    }
>> +
>> +    readstr(methodaddr, method, sizeof(method));
>> +
>> +    if (strcmp(inst->path, "/") == 0) {
>> +        if (strcmp(method, "ibm,client-architecture-support") == 0) {
>> +            ret = do_client_architecture_support(POWERPC_CPU(first_cpu), spapr,
>> +                                                 param1, FDT_MAX_SIZE);
>> +            *ret2 = 0;
>> +        }
>> +    } else if (strcmp(inst->path, "/rtas") == 0) {
>> +        if (strcmp(method, "instantiate-rtas") == 0) {
>> +            of_client_instantiate_rtas(spapr, param1);
>> +            ret = 0;
>> +            *ret2 = param1; /* rtasbase */
> 
> rtas-base ?

Yup.


> 
>> +        }
>> +    } else {
>> +        trace_spapr_of_client_error_unknown_method(method);
>> +    }
>> +
>> +trace_exit:
>> +    trace_spapr_of_client_method(ihandle, method, param1, ret, *ret2);
>> +
>> +    return ret;
>> +}
>> +
>> +static uint32_t of_client_call_interpret(SpaprMachineState *spapr,
>> +                                         uint32_t cmdaddr, uint32_t param1,
>> +                                         uint32_t param2, uint32_t *ret2)
>> +{
>> +    uint32_t ret = -1;
>> +    char cmd[256] = "";
>> +
>> +    readstr(cmdaddr, cmd, sizeof(cmd));
>> +    trace_spapr_of_client_interpret(cmd, param1, param2, ret, *ret2);
>> +
>> +    return ret;
>> +}
>> +
>> +static void of_client_quiesce(SpaprMachineState *spapr)
>> +{
>> +    int rc = fdt_pack(spapr->fdt_blob);
>> +
>> +    assert(rc == 0);
>> +
>> +    spapr->fdt_size = fdt_totalsize(spapr->fdt_blob);
>> +    spapr->fdt_initial_size = spapr->fdt_size;
> 
> Same code pattern  as in spapr_machine_reset(). A helper ?


No, not for 2 lines of code. And I changed the similar chunk above so it 
is not that similar anymore.


> 
>> +    of_client_clamed_dump(spapr->claimed);
>> +}
>> +
>> +static target_ulong spapr_h_of_client(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> +                                      target_ulong opcode, target_ulong *args)
>> +{
>> +    target_ulong of_client_args = ppc64_phys_to_real(args[0]);
>> +    struct prom_args pargs = { 0 };
>> +    char service[64];
>> +    unsigned nargs, nret;
>> +    int i, servicelen;
>> +
>> +    cpu_physical_memory_read(of_client_args, &pargs, sizeof(pargs));
>> +    nargs = be32_to_cpu(pargs.nargs);
>> +    nret = be32_to_cpu(pargs.nret);
>> +    readstr(be32_to_cpu(pargs.service), service, sizeof(service));
>> +    servicelen = strlen(service);
>> +
>> +    if (nargs >= ARRAY_SIZE(pargs.args)) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +#define cmpserv(s, a, r) \
>> +    cmpservice(service, servicelen, nargs, nret, (s), sizeof(s), (a), (r))
>> +
>> +    if (cmpserv("finddevice", 1, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_finddevice(spapr->fdt_blob,
>> +                                 be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("getprop", 4, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_getprop(spapr->fdt_blob,
>> +                              be32_to_cpu(pargs.args[0]),
>> +                              be32_to_cpu(pargs.args[1]),
>> +                              be32_to_cpu(pargs.args[2]),
>> +                              be32_to_cpu(pargs.args[3]));
>> +    } else if (cmpserv("getproplen", 2, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_getproplen(spapr->fdt_blob,
>> +                                 be32_to_cpu(pargs.args[0]),
>> +                                 be32_to_cpu(pargs.args[1]));
>> +    } else if (cmpserv("setprop", 4, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_setprop(spapr,
>> +                              be32_to_cpu(pargs.args[0]),
>> +                              be32_to_cpu(pargs.args[1]),
>> +                              be32_to_cpu(pargs.args[2]),
>> +                              be32_to_cpu(pargs.args[3]));
>> +    } else if (cmpserv("nextprop", 3, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_nextprop(spapr->fdt_blob,
>> +                               be32_to_cpu(pargs.args[0]),
>> +                               be32_to_cpu(pargs.args[1]),
>> +                               be32_to_cpu(pargs.args[2]));
>> +    } else if (cmpserv("peer", 1, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_peer(spapr->fdt_blob,
>> +                           be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("child", 1, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_child(spapr->fdt_blob,
>> +                            be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("parent", 1, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_parent(spapr->fdt_blob,
>> +                             be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("open", 1, 1)) {
>> +        pargs.args[nargs] = of_client_open(spapr, be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("close", 1, 0)) {
>> +        of_client_close(spapr, be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("instance-to-package", 1, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_instance_to_package(spapr,
>> +                                          be32_to_cpu(pargs.args[0]));
>> +    } else if (cmpserv("package-to-path", 3, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_package_to_path(spapr->fdt_blob,
>> +                                      be32_to_cpu(pargs.args[0]),
>> +                                      be32_to_cpu(pargs.args[1]),
>> +                                      be32_to_cpu(pargs.args[2]));
>> +    } else if (cmpserv("instance-to-path", 3, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_instance_to_path(spapr,
>> +                                       be32_to_cpu(pargs.args[0]),
>> +                                       be32_to_cpu(pargs.args[1]),
>> +                                       be32_to_cpu(pargs.args[2]));
>> +    } else if (cmpserv("claim", 3, 1)) {
>> +        pargs.args[nargs] =
>> +            of_client_claim(spapr,
>> +                            be32_to_cpu(pargs.args[0]),
>> +                            be32_to_cpu(pargs.args[1]),
>> +                            be32_to_cpu(pargs.args[2]));
>> +    } else if (cmpserv("release", 2, 0)) {
>> +        pargs.args[nargs] =
>> +            of_client_release(spapr,
>> +                              be32_to_cpu(pargs.args[0]),
>> +                              be32_to_cpu(pargs.args[1]));
>> +    } else if (cmpserv("call-method", 0, 0)) {
>> +        pargs.args[nargs] =
>> +            of_client_call_method(spapr,
>> +                                  be32_to_cpu(pargs.args[0]),
>> +                                  be32_to_cpu(pargs.args[1]),
>> +                                  be32_to_cpu(pargs.args[2]),
>> +                                  be32_to_cpu(pargs.args[3]),
>> +                                  be32_to_cpu(pargs.args[4]),
>> +                                  be32_to_cpu(pargs.args[5]),
>> +                                  &pargs.args[nargs + 1]);
>> +    } else if (cmpserv("interpret", 0, 0)) {
>> +        pargs.args[nargs] =
>> +            of_client_call_interpret(spapr,
>> +                                     be32_to_cpu(pargs.args[0]),
>> +                                     be32_to_cpu(pargs.args[1]),
>> +                                     be32_to_cpu(pargs.args[2]),
>> +                                     &pargs.args[nargs + 1]);
>> +    } else if (cmpserv("milliseconds", 0, 1)) {
>> +        pargs.args[nargs] = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
>> +    } else if (cmpserv("quiesce", 0, 0)) {
>> +        of_client_quiesce(spapr);
>> +    } else if (cmpserv("exit", 0, 0)) {
>> +        error_report("Stopped as the VM requested \"exit\"");
>> +        vm_stop(RUN_STATE_PAUSED); /* Or qemu_system_guest_panicked(NULL); ? */
> 
> qemu_system_guest_panicked(NULL) seems more appropriate IMHO.

Why exactly? The guest did not crash, that tiny firmware or grub just 
came to a logical end of execution when it could not find a next boot 
target.

> 
>> +    } else {
>> +        trace_spapr_of_client_error_unknown_service(service, nargs, nret);
>> +        pargs.args[nargs] = -1;
>> +    }
>> +
>> +    for (i = 0; i < nret; ++i) {
>> +        pargs.args[nargs + i] = be32_to_cpu(pargs.args[nargs + i]);
>> +    }
>> +
>> +    cpu_physical_memory_write(of_client_args, &pargs,
>> +                              sizeof(uint32_t) * (3 + nargs + nret));
>> +
>> +    return H_SUCCESS;
>> +}
>> +
> 
> That's all for now.

Thanks! I'll repost in a sec. But I still wonder on what terms this is 
going to be allowed in the QEMU tree at all.




-- 
Alexey