qemu-ppc
[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: BALATON Zoltan
Subject: Re: [PATCH qemu v23] spapr: Fix implementation of Open Firmware client interface
Date: Fri, 9 Jul 2021 00:34:11 +0200 (CEST) 
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. 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).

Regards,
BALATON Zoltan


@@ -919,6 +921,8 @@ static uint32_t vof_client_handle(MachineState *ms, void 
*fdt, Vof *vof,
        ret = -1;
    }

+#undef cmpserv
+
    return ret;
}

diff --git a/pc-bios/vof/ci.c b/pc-bios/vof/ci.c
index 2b56050238a3..fc4821b3e970 100644
--- a/pc-bios/vof/ci.c
+++ b/pc-bios/vof/ci.c
@@ -69,7 +69,7 @@ static int call_ci(const char *service, int nargs, int nret, 
...)
    }

    if (ci_entry((uint32_t)(&args)) < 0) {
-        return PROM_ERROR;
+        return -1;
    }

    return (nret > 0) ? args.args[nargs] : 0;
diff --git a/pc-bios/vof/libc.c b/pc-bios/vof/libc.c
index 00c10e6e7da1..fdbc30f777d4 100644
--- a/pc-bios/vof/libc.c
+++ b/pc-bios/vof/libc.c
@@ -54,32 +54,6 @@ int memcmp(const void *ptr1, const void *ptr2, size_t n)
    return 0;
}

-void *memmove(void *dest, const void *src, size_t n)
-{
-    char *cdest;
-    const char *csrc;
-    int i;
-
-    /* Do the buffers overlap in a bad way? */
-    if (src < dest && src + n >= dest) {
-        /* Copy from end to start */
-        cdest = dest + n - 1;
-        csrc = src + n - 1;
-        for (i = 0; i < n; i++) {
-            *cdest-- = *csrc--;
-        }
-    } else {
-        /* Normal copy is possible */
-        cdest = dest;
-        csrc = src;
-        for (i = 0; i < n; i++) {
-            *cdest++ = *csrc++;
-        }
-    }
-
-    return dest;
-}
-
void *memset(void *dest, int c, size_t size)
{
    unsigned char *d = (unsigned char *)dest;
diff --git a/pc-bios/vof/main.c b/pc-bios/vof/main.c
index 9fc30d2d0957..0f0f6b4cb194 100644
--- a/pc-bios/vof/main.c
+++ b/pc-bios/vof/main.c
@@ -6,7 +6,7 @@ void do_boot(unsigned long addr, unsigned long _r3, unsigned 
long _r4)
    register unsigned long r4 __asm__("r4") = _r4;
    register unsigned long r5 __asm__("r5") = (unsigned long) _prom_entry;

-    ((client *)(uint32_t)addr)();
+    ((void (*)(void))(uint32_t)addr)();
}

void entry_c(void)
diff --git a/MAINTAINERS b/MAINTAINERS
index ce122eeced16..89d71b42b24f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1362,8 +1362,8 @@ F: include/hw/pci-host/mv64361.h

Virtual Open Firmware (VOF)
M: Alexey Kardashevskiy <aik@ozlabs.ru>
-M: David Gibson <david@gibson.dropbear.id.au>
-M: Greg Kurz <groug@kaod.org>
+R: David Gibson <david@gibson.dropbear.id.au>
+R: Greg Kurz <groug@kaod.org>
L: qemu-ppc@nongnu.org
S: Maintained
F: hw/ppc/spapr_vof*
diff --git a/pc-bios/vof.bin b/pc-bios/vof.bin
index 
1ec670be82134adcb5ae128732aff6e371281360..300cb7c7f9d9d77ffa7cbb7f0f26919246ef2d14
 100755
GIT binary patch
delta 151
zcmX>h+aSGxjghy9!GnR}jEw^WLxNM!WMRf~rtUM7dl>VWx??8)VQgaRda${HDTtA&
zvuE-Z<}9X8h0P8uhZvdKV<xk(#WA)0nViCw#?&@t@)@=|rZ$VsKI~hWCdYEJZ`R>H
uz%*HauS<{T1Oo%l10a6Gz`)A@#2gF^j0r&O17wQ;u?!Gv0I>lOTL1tL)F-q6

delta 369
zcmZpWJ|Vk-jghyH!GnR}jEw^WLxNM^WMRf~re2ZBJ&buwJxeD4VQgaR(b(L;6vW8X
zb!GAu<}9YJjLi-#hZvbUmP}@0i(~3=nViCw#?*di@)@=|ruK%-KI~hW>f;$?8toY*
zYPdWc92yuV0NDzSK(SgaFA*RO7I$o9r~rvuYX1KZ5(CLiv}fQz5(BFTit$(~FfagV
z0iZ(-fNFUxwf_GHi38PgSaJf{@(diEUJMK~JsB8)VgeSHnhcB}4M4>LAOnF8VQ_5t
ze*$Pg43IAYlml5L12P2J@X0?olqCgl>7J~^X|b7u>j2Y40hP%oc)IlX1Q;0jG=SIy
dh=FGF1u!r$CIGPykR1cWDL`BR#1%l?005cvU&jCd
reply via email to
[Prev in Thread] Current Thread [Next in Thread]