On 05/12/2020 05:32, Greg Kurz wrote:
On Tue, 13 Oct 2020 13:19:11 +1100 Alexey Kardashevskiy <a...@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=df5be5be8735FWIW 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 <a...@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 447My 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
