qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH qemu v23] spapr: Fix implementation of Open Firmware client i


From: Alexey Kardashevskiy
Subject: Re: [PATCH qemu v23] spapr: Fix implementation of Open Firmware client interface
Date: Fri, 9 Jul 2021 13:43:45 +1000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Thunderbird/89.0



On 09/07/2021 08:34, BALATON Zoltan wrote:
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
This addresses the comments from v22.

The functional changes are (the VOF ones need retesting with Pegasos2):

(VOF) setprop will start failing if the machine class callback
did not handle it;
(VOF) unit addresses are lowered in path_offset();
(SPAPR) /chosen/bootargs is initialized from kernel_cmdline if
the client did not change it.

Fixes: 5c991e5d4378 ("spapr: Implement Open Firmware client interface")
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
include/hw/ppc/spapr.h |   3 +--
pc-bios/vof/vof.h      |   2 --
hw/ppc/spapr.c         |  10 +---------
hw/ppc/spapr_hcall.c   |   5 ++---
hw/ppc/spapr_vof.c     |  32 +++++++++++++++++++++++---------
hw/ppc/vof.c           |  30 +++++++++++++++++-------------
pc-bios/vof/ci.c       |   2 +-
pc-bios/vof/libc.c     |  26 --------------------------
pc-bios/vof/main.c     |   2 +-
MAINTAINERS            |   4 ++--
pc-bios/vof.bin        | Bin 3784 -> 3456 bytes
11 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index a25e69fe4cf4..779f707fb8b9 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -964,8 +964,7 @@ void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
hwaddr spapr_get_rtas_addr(void);
bool spapr_memory_hot_unplug_supported(SpaprMachineState *spapr);

-void spapr_vof_reset(SpaprMachineState *spapr, void *fdt,
-                     target_ulong *stack_ptr, Error **errp);
+void spapr_vof_reset(SpaprMachineState *spapr, void *fdt, Error **errp);
void spapr_vof_quiesce(MachineState *ms);
bool spapr_vof_setprop(MachineState *ms, const char *path, const char *propname,
                       void *val, int vallen);
diff --git a/pc-bios/vof/vof.h b/pc-bios/vof/vof.h
index 2d8958076907..5f12c077f513 100644
--- a/pc-bios/vof/vof.h
+++ b/pc-bios/vof/vof.h
@@ -10,11 +10,9 @@ typedef unsigned short uint16_t;
typedef unsigned long uint32_t;
typedef unsigned long long uint64_t;
#define NULL (0)
-#define PROM_ERROR (-1u)
typedef unsigned long ihandle;
typedef unsigned long phandle;
typedef int size_t;
-typedef void client(void);

/* globals */
extern void _prom_entry(void); /* OF CI entry point (i.e. this firmware) */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9b6d0f58756..3808d4705304 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1645,15 +1645,7 @@ static void spapr_machine_reset(MachineState *machine)

    fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
    if (spapr->vof) {
-        target_ulong stack_ptr = 0;
-
-        spapr_vof_reset(spapr, fdt, &stack_ptr, &error_fatal);
-
-        spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT,
-                                  stack_ptr, spapr->initrd_base,
-                                  spapr->initrd_size);
-        /* VOF is 32bit BE so enforce MSR here */
-        first_ppc_cpu->env.msr &= ~((1ULL << MSR_SF) | (1ULL << MSR_LE));
+        spapr_vof_reset(spapr, fdt, &error_fatal);
        /*
         * Do not pack the FDT as the client may change properties.
         * VOF client does not expect the FDT so we do not load it to the VM.
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 80ae8eaadd34..0e9a5b2e4053 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1080,7 +1080,7 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
    SpaprOptionVector *ov1_guest, *ov5_guest;
    bool guest_radix;
    bool raw_mode_supported = false;
-    bool guest_xive, reset_fdt = false;
+    bool guest_xive;
    CPUState *cs;
    void *fdt;
    uint32_t max_compat = spapr->max_compat_pvr;
@@ -1233,8 +1233,7 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu,
        spapr_setup_hpt(spapr);
    }

-    reset_fdt = spapr->vof != NULL;
-    fdt = spapr_build_fdt(spapr, reset_fdt, fdt_bufsize);
+    fdt = spapr_build_fdt(spapr, spapr->vof != NULL, fdt_bufsize);
    g_free(spapr->fdt_blob);
    spapr->fdt_size = fdt_totalsize(fdt);
    spapr->fdt_initial_size = spapr->fdt_size;
diff --git a/hw/ppc/spapr_vof.c b/hw/ppc/spapr_vof.c
index 1d5bec146c49..951fed0e191d 100644
--- a/hw/ppc/spapr_vof.c
+++ b/hw/ppc/spapr_vof.c
@@ -9,6 +9,7 @@
#include "qapi/error.h"
#include "hw/ppc/spapr.h"
#include "hw/ppc/spapr_vio.h"
+#include "hw/ppc/spapr_cpu_core.h"
#include "hw/ppc/fdt.h"
#include "hw/ppc/vof.h"
#include "sysemu/sysemu.h"
@@ -30,13 +31,19 @@ target_ulong spapr_h_vof_client(PowerPCCPU *cpu, SpaprMachineState *spapr,
void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt)
{
    char *stdout_path = spapr_vio_stdout_path(spapr->vio_bus);
-    int chosen;

    vof_build_dt(fdt, spapr->vof);

-    _FDT(chosen = fdt_path_offset(fdt, "/chosen"));
-    _FDT(fdt_setprop_string(fdt, chosen, "bootargs",
-                            spapr->vof->bootargs ? : ""));
+    if (spapr->vof->bootargs) {
+        int chosen;
+
+        _FDT(chosen = fdt_path_offset(fdt, "/chosen"));
+        /*
+         * If the client did not change "bootargs", spapr_dt_chosen() must have
+         * stored machine->kernel_cmdline in it before getting here.
+         */
+        _FDT(fdt_setprop_string(fdt, chosen, "bootargs", spapr->vof->bootargs));
+    }

    /*
     * SLOF-less setup requires an open instance of stdout for early
@@ -49,20 +56,21 @@ void spapr_vof_client_dt_finalize(SpaprMachineState *spapr, void *fdt)
    }
}

-void spapr_vof_reset(SpaprMachineState *spapr, void *fdt,
-                     target_ulong *stack_ptr, Error **errp)
+void spapr_vof_reset(SpaprMachineState *spapr, void *fdt, Error **errp)
{
+    target_ulong stack_ptr;
    Vof *vof = spapr->vof;
+    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);

    vof_init(vof, spapr->rma_size, errp);

-    *stack_ptr = vof_claim(vof, 0, VOF_STACK_SIZE, VOF_STACK_SIZE);
-    if (*stack_ptr == -1) {
+    stack_ptr = vof_claim(vof, 0, VOF_STACK_SIZE, VOF_STACK_SIZE);
+    if (stack_ptr == -1) {
        error_setg(errp, "Memory allocation for stack failed");
        return;
    }
    /* Stack grows downwards plus reserve space for the minimum stack frame */
-    *stack_ptr += VOF_STACK_SIZE - 0x20;
+    stack_ptr += VOF_STACK_SIZE - 0x20;

    if (spapr->kernel_size &&
        vof_claim(vof, spapr->kernel_addr, spapr->kernel_size, 0) == -1) { @@ -78,6 +86,12 @@ void spapr_vof_reset(SpaprMachineState *spapr, void *fdt,

    spapr_vof_client_dt_finalize(spapr, fdt);

+    spapr_cpu_set_entry_state(first_ppc_cpu, SPAPR_ENTRY_POINT,
+                              stack_ptr, spapr->initrd_base,
+                              spapr->initrd_size);
+    /* VOF is 32bit BE so enforce MSR here */
+    first_ppc_cpu->env.msr &= ~((1ULL << MSR_SF) | (1ULL << MSR_LE));
+
    /*
     * At this point the expected allocation map is:
     *
diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index a17fd9d2fe94..8d307cd048ba 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -144,15 +144,16 @@ static int path_offset(const void *fdt, const char *path)
     * the lower case forms of the hexadecimal digits in the range a..f,
     * suppressing leading zeros".
     */
-    at = strchr(path, '@');
-    if (!at) {
-        return fdt_path_offset(fdt, path);
-    }
-
    p = g_strdup(path);
-    for (at = at - path + p + 1; *at; ++at) {
-        *at = tolower(*at);
+    for (at = strchr(p, '@'); at && *at; ) {
+            if (*at == '/') {
+                at = strchr(at, '@');
+            } else {
+                *at = tolower(*at);
+                ++at;
+            }
    }
+
    return fdt_path_offset(fdt, p);
}

@@ -300,6 +301,7 @@ static uint32_t vof_setprop(MachineState *ms, void *fdt, Vof *vof,
    char trval[64] = "";
    char nodepath[VOF_MAX_PATH] = "";
    Object *vmo = object_dynamic_cast(OBJECT(ms), TYPE_VOF_MACHINE_IF);
+    VofMachineIfClass *vmc;
    g_autofree char *val = NULL;

    if (vallen > VOF_MAX_SETPROPLEN) {
@@ -322,13 +324,13 @@ static uint32_t vof_setprop(MachineState *ms, void *fdt, Vof *vof,
        goto trace_exit;
    }

-    if (vmo) {
-        VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
+    if (!vmo) {
+        goto trace_exit;
+    }

-        if (vmc->setprop &&
-            !vmc->setprop(ms, nodepath, propname, val, vallen)) {
-            goto trace_exit;
-        }
+    vmc = VOF_MACHINE_GET_CLASS(vmo);
+    if (!vmc->setprop || !vmc->setprop(ms, nodepath, propname, val, vallen)) {
+        goto trace_exit;
    }

    ret = fdt_setprop(fdt, offset, propname, val, vallen);

MorphOS still boots but this breaks Linux which changes a few things in the device tree to fix it up to make it look the way it thinks is better.


What are those things? What does the change break precisely? Does the kernel stop booting?
Can you please send output with the trace_vof_setprop tracepoint enabled?


I suggest to drop these two hunks and keep the default to allow setprop if there's no callback. If you want to disable it for a board you can add a callback and spapr already has one which disallows all but some props by default so this will only change pegasos2 where I'd need to add an empty callback to revert this. So at the end this will be ineffective, therefore just drop it unless there's a good reason to keep.

Other than this:

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

with MorphOS and Linux (with above two hunks reverted).

Thanks!


--
Alexey



reply via email to

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