diff mbox series

[qemu,v23] spapr: Fix implementation of Open Firmware client interface

Message ID 20210708065625.548396-1-aik@ozlabs.ru
State New
Headers show
Series [qemu,v23] spapr: Fix implementation of Open Firmware client interface | expand

Commit Message

Alexey Kardashevskiy July 8, 2021, 6:56 a.m. UTC 
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/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

Comments

BALATON Zoltan July 8, 2021, 10:18 a.m. UTC | #1 
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;

I'll try this later but I think I've seen guests using setprop (Linux also 
does that for some property). How should I allow that? Do I need a new 
callback for this? Could it be allower unless there's a callback that 
could deby it? But that was the previous way I think.

Regards,
BALATON Zoltan

> (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);
> @@ -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
>
>
Alexey Kardashevskiy July 8, 2021, 10:25 a.m. UTC | #2 
On 08/07/2021 20:18, 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;
> 
> I'll try this later but I think I've seen guests using setprop (Linux 
> also does that for some property). How should I allow that? Do I need a 
> new callback for this? Could it be allower unless there's a callback 
> that could deby it? But that was the previous way I think.

A simple defined callback which always returns "true" should do.
BALATON Zoltan July 8, 2021, 10:39 a.m. UTC | #3 
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
> On 08/07/2021 20:18, 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;
>> 
>> I'll try this later but I think I've seen guests using setprop (Linux also 
>> does that for some property). How should I allow that? Do I need a new 
>> callback for this? Could it be allower unless there's a callback that could 
>> deby it? But that was the previous way I think.
>
> A simple defined callback which always returns "true" should do.

Yes but what's the point? That would just effectiverly disable this change 
so if we need that, we could just as well keep the previous behaviour 
which is to allow setprop unless there's a callback that can decide 
otherwise. The spapr machine has such a callback so it already does not 
allow all setprop and if I'll have a callback in pegasos2 returning true 
that will allow what's allowed now so this part of this patch does nothing 
indeed.

Since guests could do all kinds of things that we don't know without 
trying them restricting setprop is a good way to run into problems with 
guests that were not tested that could otherwise just work. Then we'll 
need another patch to enable that guest adding some more properties to the 
list of allowed ones. Why it it a problem to allow this by default in the 
first place and only reject changes for machines that have a callback? 
Then I would not need more empty callbacks in pegasos2.

Regards,
BALATON Zoltan
Alexey Kardashevskiy July 8, 2021, 11:10 a.m. UTC | #4 
On 08/07/2021 20:39, BALATON Zoltan wrote:
> On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
>> On 08/07/2021 20:18, 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;
>>>
>>> I'll try this later but I think I've seen guests using setprop (Linux 
>>> also does that for some property). How should I allow that? Do I need 
>>> a new callback for this? Could it be allower unless there's a 
>>> callback that could deby it? But that was the previous way I think.
>>
>> A simple defined callback which always returns "true" should do.
> 
> Yes but what's the point? That would just effectiverly disable this 
> change so if we need that, we could just as well keep the previous 
> behaviour which is to allow setprop unless there's a callback that can 
> decide otherwise. The spapr machine has such a callback so it already 
> does not allow all setprop and if I'll have a callback in pegasos2 
> returning true that will allow what's allowed now so this part of this 
> patch does nothing indeed.
> 
> Since guests could do all kinds of things that we don't know without 
> trying them restricting setprop is a good way to run into problems with 
> guests that were not tested that could otherwise just work. Then we'll 
> need another patch to enable that guest adding some more properties to 
> the list of allowed ones. Why it it a problem to allow this by default 
> in the first place and only reject changes for machines that have a 
> callback? Then I would not need more empty callbacks in pegasos2.


 From here:
https://patchwork.ozlabs.org/project/qemu-devel/patch/20210625055155.2252896-1-aik@ozlabs.ru/#2714158

===

 >>> +    if (vmo) {
 >>> +        VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
 >>> +
 >>> +        if (vmc->setprop &&
 >>> +            !vmc->setprop(ms, nodepath, propname, val, vallen)) {
 >>> +            goto trace_exit;
 >>
 >> This defaults to allowing the setprop if the machine doesn't provide a
 >> setprop callback.  I think it would be safer to default to prohibiting
 >> all setprops except those the machine explicitly allows.
 >
 >
 > Mmmm... I can imagine the client using the device tree as a temporary
 > storage. I'd rather add a trace for such cases.

If they do, I think that's something we'll need to consider and
account for that platform, rather than something we want to allow to
begin with.

===
BALATON Zoltan July 8, 2021, 1 p.m. UTC | #5 
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
> On 08/07/2021 20:39, BALATON Zoltan wrote:
>> On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
>>> On 08/07/2021 20:18, 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;
>>>> 
>>>> I'll try this later but I think I've seen guests using setprop (Linux 
>>>> also does that for some property). How should I allow that? Do I need a 
>>>> new callback for this? Could it be allower unless there's a callback that 
>>>> could deby it? But that was the previous way I think.
>>> 
>>> A simple defined callback which always returns "true" should do.
>> 
>> Yes but what's the point? That would just effectiverly disable this change 
>> so if we need that, we could just as well keep the previous behaviour which 
>> is to allow setprop unless there's a callback that can decide otherwise. 
>> The spapr machine has such a callback so it already does not allow all 
>> setprop and if I'll have a callback in pegasos2 returning true that will 
>> allow what's allowed now so this part of this patch does nothing indeed.
>> 
>> Since guests could do all kinds of things that we don't know without trying 
>> them restricting setprop is a good way to run into problems with guests 
>> that were not tested that could otherwise just work. Then we'll need 
>> another patch to enable that guest adding some more properties to the list 
>> of allowed ones. Why it it a problem to allow this by default in the first 
>> place and only reject changes for machines that have a callback? Then I 
>> would not need more empty callbacks in pegasos2.
>
>
> From here:
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20210625055155.2252896-1-aik@ozlabs.ru/#2714158
>
> ===
>
>>>> +    if (vmo) {
>>>> +        VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
>>>> +
>>>> +        if (vmc->setprop &&
>>>> +            !vmc->setprop(ms, nodepath, propname, val, vallen)) {
>>>> +            goto trace_exit;
>>>
>>> This defaults to allowing the setprop if the machine doesn't provide a
>>> setprop callback.  I think it would be safer to default to prohibiting
>>> all setprops except those the machine explicitly allows.
>>
>>
>> Mmmm... I can imagine the client using the device tree as a temporary
>> storage. I'd rather add a trace for such cases.
>
> If they do, I think that's something we'll need to consider and
> account for that platform, rather than something we want to allow to
> begin with.

I've seen that, yet I don't understand why. If I'll just add an empty 
callback in pegasos2 to disable it then we're back to where we were 
before. So my question is why do we want to explicitely enable setprop for 
every guest when we encounter it one by one (especially if this works on 
other OF implementations so guests are free to change the device tree 
therefore we don't know in advance what are allowable properties). If you 
don't want it for spapr I think you already have the callback for it that 
disallows it for all but at a few properties but why change the default 
for other machines that don't have a callback? If I can still add an empty 
callback that could well be the default just to avoid more boilerplate in 
board code.

Regards,
BALATON Zoltan
BALATON Zoltan July 8, 2021, 10:34 p.m. UTC | #6 
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
>
>
David Gibson July 9, 2021, 12:52 a.m. UTC | #7 
On Thu, Jul 08, 2021 at 04:56:25PM +1000, 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>

Applied to ppc-for-6.1, thanks.

> ---
>  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);
> @@ -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
>
David Gibson July 9, 2021, 12:54 a.m. UTC | #8 
On Thu, Jul 08, 2021 at 03:00:22PM +0200, BALATON Zoltan wrote:
> On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
> > On 08/07/2021 20:39, BALATON Zoltan wrote:
> > > On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
> > > > On 08/07/2021 20:18, 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;
> > > > > 
> > > > > I'll try this later but I think I've seen guests using
> > > > > setprop (Linux also does that for some property). How should
> > > > > I allow that? Do I need a new callback for this? Could it be
> > > > > allower unless there's a callback that could deby it? But
> > > > > that was the previous way I think.
> > > > 
> > > > A simple defined callback which always returns "true" should do.
> > > 
> > > Yes but what's the point? That would just effectiverly disable this
> > > change so if we need that, we could just as well keep the previous
> > > behaviour which is to allow setprop unless there's a callback that
> > > can decide otherwise. The spapr machine has such a callback so it
> > > already does not allow all setprop and if I'll have a callback in
> > > pegasos2 returning true that will allow what's allowed now so this
> > > part of this patch does nothing indeed.
> > > 
> > > Since guests could do all kinds of things that we don't know without
> > > trying them restricting setprop is a good way to run into problems
> > > with guests that were not tested that could otherwise just work.
> > > Then we'll need another patch to enable that guest adding some more
> > > properties to the list of allowed ones. Why it it a problem to allow
> > > this by default in the first place and only reject changes for
> > > machines that have a callback? Then I would not need more empty
> > > callbacks in pegasos2.
> > 
> > 
> > From here:
> > https://patchwork.ozlabs.org/project/qemu-devel/patch/20210625055155.2252896-1-aik@ozlabs.ru/#2714158
> > 
> > ===
> > 
> > > > > +    if (vmo) {
> > > > > +        VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
> > > > > +
> > > > > +        if (vmc->setprop &&
> > > > > +            !vmc->setprop(ms, nodepath, propname, val, vallen)) {
> > > > > +            goto trace_exit;
> > > > 
> > > > This defaults to allowing the setprop if the machine doesn't provide a
> > > > setprop callback.  I think it would be safer to default to prohibiting
> > > > all setprops except those the machine explicitly allows.
> > > 
> > > 
> > > Mmmm... I can imagine the client using the device tree as a temporary
> > > storage. I'd rather add a trace for such cases.
> > 
> > If they do, I think that's something we'll need to consider and
> > account for that platform, rather than something we want to allow to
> > begin with.
> 
> I've seen that, yet I don't understand why. If I'll just add an empty
> callback in pegasos2 to disable it then we're back to where we were before.
> So my question is why do we want to explicitely enable setprop for every
> guest when we encounter it one by one (especially if this works on other OF
> implementations so guests are free to change the device tree therefore we
> don't know in advance what are allowable properties). If you don't want it
> for spapr I think you already have the callback for it that disallows it for
> all but at a few properties but why change the default for other machines
> that don't have a callback? If I can still add an empty callback that could
> well be the default just to avoid more boilerplate in board code.

Because I think hitting the failure and deciphering that we need to
add setprop logic is likely to be less pain in the long run, than
allowing setprop by default, then discovering things break because the
guest expected that setprop to have some semantic effect beyond just
changing the dt, and we never even realized it was doing it.
Alexey Kardashevskiy July 9, 2021, 3:43 a.m. UTC | #9 
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!
BALATON Zoltan July 9, 2021, 1:28 p.m. UTC | #10 
On Fri, 9 Jul 2021, Alexey Kardashevskiy wrote:
> On 09/07/2021 08:34, BALATON Zoltan wrote:
>> 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?

It's fixing up some props that on Pegasos2 firmware are not how Linux 
expects them. Without this it's not booting. Attached is the trace output 
with VOF v23 as it is now (nosetprop) and another one after the patch that 
adds setprop callback to pegasos2 I'm sending separately. That patch 
restores Linux boot but I still think all this boilerplate would not be 
needed if we kept the default to allow setprop and that results in overall 
simpler code. If something breaks becuase of enabling setprop by default 
(which normally works on real Open Firmware) it's easy enough to debug by 
enabling vof_setprop trace points so I don't see this adds any value other 
than making board code more complex.

Regards,
BALATON Zoltan
via_superio_cfg: unimplemented register 0xf2
via_superio_cfg: unimplemented register 0xf4
via_superio_cfg: unimplemented register 0xf6
via_superio_cfg: unimplemented register 0xf7
vof_finddevice "/chosen" => ph=0x3
vof_finddevice "/chosen" => ph=0x3
vof_write ih=0x1 [58] "
zImage starting: loaded at 0x00400000 (sp: 0x00cccfa0)
"
vof_finddevice "/chosen" => ph=0x3
vof_write ih=0x1 [42] "Allocating 0x8a1ec0 bytes for kernel ...
"
vof_finddevice "/openprom" => ph=0x2
vof_write ih=0x1 [29] "OF version = 'Pegasos2,1.1'
"
vof_write ih=0x1 [67] "Trying to claim from 0x400000 to 0xcd8dfc (0x8d8dfc) got ffffffff
"
vof_write ih=0x1 [51] "gunzipping (0x00d00000 <- 0x0040c000:0x00761c62)..."
vof_write ih=0x1 [21] "done 0x743000 bytes
"
vof_write ih=0x1 [48] "Attached initrd image at 0x00762000-0x00ccb90d
"
vof_write ih=0x1 [42] "Allocating 0x56990d bytes for initrd ...
"
vof_write ih=0x1 [60] "Relocating initrd 0x15a2000 <- 0x00762000 (0x56990d bytes)
"
vof_write ih=0x1 [25] "initrd head: 0x1f8b0808
"
vof_setprop ph=0x3 "linux,initrd-start" [] len=4 => ret=8
vof_setprop ph=0x3 "linux,initrd-end" [] len=4 => ret=8
vof_write ih=0x1 [22] "
Linux/PowerPC load: "
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x3 "bootargs" [] len=1 => ret=8
vof_write ih=0x1 [25] "Finalizing device tree..."
vof_write ih=0x1 [35] " using OF tree (promptr=0000010c)
"
vof_finddevice "/chosen" => ph=0x3
vof_finddevice "/" => ph=0x1
vof_finddevice "/openprom" => ph=0x2
vof_write ih=0x1 [21] "OF stdout device is: "
vof_write ih=0x1 [9] "/failsafe"
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x3 "linux,stdout-path" [] len=10 => ret=8
vof_setprop ph=0x3 "linux,stdout-package" [] len=4 => ret=8
vof_write ih=0x1 [18] "Preparing to boot "
vof_write ih=0x1 [141] "Linux version 3.16.0-6-powerpc (debian-kernel@lists.debian.org) (gcc version 4.8.4 (Debian 4.8.4-1) ) #1 Debian 3.16.56-1+deb8u1 (2018-05-08)"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [23] "Detected machine type: "
vof_write ih=0x1 [8] "00000500"
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x3 "linux,initrd-start" [] len=8 => ret=8
vof_setprop ph=0x3 "linux,initrd-end" [] len=8 => ret=8
vof_write ih=0x1 [14] "command line: "
vof_write ih=0x1 [31] "console=ttyS0,9600 console=tty0"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "memory layout at init:"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  memory_limit : "
vof_write ih=0x1 [8] "00000000"
vof_write ih=0x1 [16] " (16 MB aligned)"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  alloc_bottom : "
vof_write ih=0x1 [8] "01b0c000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  alloc_top    : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  alloc_top_hi : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  rmo_top      : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  ram_top      : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_finddevice "/rtas" => ph=0x7
vof_write ih=0x1 [24] "instantiating rtas at 0x"
vof_write ih=0x1 [8] "1ffff000"
vof_write ih=0x1 [3] "..."
vof_finddevice "/rtas" => ph=0x7
vof_write ih=0x1 [5] " done"
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x7 "linux,rtas-base" [] len=4 => ret=6
vof_setprop ph=0x7 "linux,rtas-entry" [] len=4 => ret=6
vof_finddevice "/pci@80000000/isa@c" => ph=0xc
vof_write ih=0x1 [41] "Fixing up missing ISA range on Pegasos..."
vof_write ih=0x1 [2] "
"
vof_setprop ph=0xc "ranges" [] len=24 => ret=20
vof_finddevice "/pci@80000000/ide@C,1" => ph=0x14
vof_write ih=0x1 [37] "Fixing up IDE interrupt on Pegasos..."
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x14 "interrupts" [] len=8 => ret=22
vof_write ih=0x1 [38] "Fixing up IDE class-code on Pegasos..."
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x14 "class-code" [] len=4 => ret=22
vof_finddevice "/" => ph=0x1
vof_write ih=0x1 [25] "copying OF device tree..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "Building dt strings..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [24] "Building dt structure..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "Device tree strings 0x"
vof_write ih=0x1 [8] "01b0d000"
vof_write ih=0x1 [6] " -> 0x"
vof_write ih=0x1 [8] "01b0d3c4"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "Device tree struct  0x"
vof_write ih=0x1 [8] "01b0e000"
vof_write ih=0x1 [6] " -> 0x"
vof_write ih=0x1 [8] "01b10000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [18] "Calling quiesce..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [24] "returning from prom_init"
vof_write ih=0x1 [2] "
"
[    0.000000] Using CHRP machine description
[    0.000000] Total memory = 512MB; using 1024kB for hash table (at cff00000)
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Initializing cgroup subsys cpuacct
[    0.000000] Linux version 3.16.0-6-powerpc (debian-kernel@lists.debian.org) (gcc version 4.8.4 (Debian 4.8.4-1) ) #1 Debian 3.16.56-1+deb8u1 (2018-05-08)
[    0.000000] OF: no ranges; cannot translate
[    0.000000] chrp type = 6 [Genesi Pegasos]
[    0.000000] Pegasos l2cr : L2 cache was not active, activating
[    0.000000] PCI bus 0 controlled by /pci@80000000 at 80000000
[    0.000000] PCI host bridge /pci@80000000 (primary) ranges:
[    0.000000]   IO 0x00000000fe000000..0x00000000fe00ffff -> 0x0000000000000000
[    0.000000]  MEM 0x0000000080000000..0x00000000bfffffff -> 0x0000000080000000 
[    0.000000] PCI bus 0 controlled by /pci@c0000000 at c0000000
[    0.000000] PCI host bridge /pci@c0000000  ranges:
[    0.000000]   IO 0x00000000f8000000..0x00000000f800ffff -> 0x0000000000000000
[    0.000000]  MEM 0x00000000c0000000..0x00000000dfffffff -> 0x00000000c0000000 
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x00000000-0x1fffffff]
[    0.000000]   Normal   empty
[    0.000000]   HighMem  empty
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x00000000-0x1fffffff]
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 130048
[    0.000000] Kernel command line: console=ttyS0,9600 console=tty0
[    0.000000] PID hash table entries: 2048 (order: 1, 8192 bytes)
[    0.000000] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
[    0.000000] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
[    0.000000] Sorting __ex_table...
[    0.000000] Memory: 504272K/524288K available (5164K kernel code, 424K rwdata, 1428K rodata, 404K init, 1403K bss, 20016K reserved, 0K highmem)
[    0.000000] Kernel virtual memory layout:
[    0.000000]   * 0xfffcf000..0xfffff000  : fixmap
[    0.000000]   * 0xff800000..0xffc00000  : highmem PTEs
[    0.000000]   * 0xff7e0000..0xff800000  : early ioremap
[    0.000000]   * 0xe1000000..0xff7e0000  : vmalloc & ioremap
[    0.000000] NR_IRQS:512 nr_irqs:512 16
[    0.000000] i8259 legacy interrupt controller initialized
[    0.000000] OF: no ranges; cannot translate
[    0.000406] clocksource: timebase mult[1e000005] shift[24] registered
[    0.003930] Console: colour dummy device 80x25
[    0.004989] console [tty0] enabled
[    0.007659] pid_max: default: 32768 minimum: 301
[    0.008162] Security Framework initialized
[    0.010590] AppArmor: AppArmor disabled by boot time parameter
[    0.010625] Yama: disabled by default; enable with sysctl kernel.yama.*
[    0.011708] Mount-cache hash table entries: 1024 (order: 0, 4096 bytes)
[    0.011740] Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes)
[    0.019167] Initializing cgroup subsys memory
[    0.019393] Initializing cgroup subsys devices
[    0.019499] Initializing cgroup subsys freezer
[    0.019575] Initializing cgroup subsys net_cls
[    0.019649] Initializing cgroup subsys blkio
[    0.019710] Initializing cgroup subsys perf_event
[    0.019766] Initializing cgroup subsys net_prio
[    0.020123] ftrace: allocating 17989 entries in 53 pages
[    0.057417] devtmpfs: initialized
[    0.061791] futex hash table entries: 256 (order: -1, 3072 bytes)
[    0.066297] NET: Registered protocol family 16
[    0.073546] PCI: Probing PCI hardware
[    0.074472] PCI host bridge to bus 0000:00
[    0.074791] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    0.074891] pci_bus 0000:00: root bus resource [mem 0x80000000-0xbfffffff]
[    0.075017] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.075849] PCI host bridge to bus 0001:01
[    0.075923] pci_bus 0001:01: root bus resource [io  0xffff0000-0xffffffff] (bus address [0x0000-0xffff])
[    0.075953] pci_bus 0001:01: root bus resource [mem 0xc0000000-0xdfffffff]
[    0.075982] pci_bus 0001:01: root bus resource [bus 01-ff]
[    0.086162] vgaarb: loaded
[    0.087185] SCSI subsystem initialized
[    0.093231] Switched to clocksource timebase
[    0.118401] NET: Registered protocol family 2
[    0.122249] TCP established hash table entries: 4096 (order: 2, 16384 bytes)
[    0.122322] TCP bind hash table entries: 4096 (order: 2, 16384 bytes)
[    0.122465] TCP: Hash tables configured (established 4096 bind 4096)
[    0.123058] TCP: reno registered
[    0.123120] UDP hash table entries: 256 (order: 0, 4096 bytes)
[    0.123220] UDP-Lite hash table entries: 256 (order: 0, 4096 bytes)
[    0.124289] NET: Registered protocol family 1
[    0.129293] rtasd: No event-scan on system
[    0.129387] Thermal assist unit using timers, shrink_timer: 500 jiffies
[    0.132230] audit: initializing netlink subsys (disabled)
[    0.133018] audit: type=2000 audit(946598400.115:1): initialized
[    0.136327] zbud: loaded
[    0.136975] VFS: Disk quotas dquot_6.5.2
[    0.137100] Dquot-cache hash table entries: 1024 (order 0, 4096 bytes)
[    0.137915] msgmni has been set to 986
[    0.149128] alg: No test for stdrng (krng)
[    0.149329] Block layer SCSI generic (bsg) driver version 0.4 loaded (major 252)
[    0.149767] io scheduler noop registered
[    0.149814] io scheduler deadline registered
[    0.150022] io scheduler cfq registered (default)
[    0.152161] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.156251] console [ttyS0] disabled
[    0.177973] serial8250.0: ttyS0 at I/O 0x2f8 (irq = 0, base_baud = 115200) is a 16550A
[    0.184670] console [ttyS0] enabled
[    0.185257] pmac_zilog: 0.6 (Benjamin Herrenschmidt <benh@kernel.crashing.org>)
[    0.185422] Serial: MPC52xx PSC UART driver
[    0.185649] Generic non-volatile memory driver v1.1
[    0.185921] Linux agpgart interface v0.103
[    0.186842] Warning: no ADB interface detected
[    0.187742] mousedev: PS/2 mouse device common for all mice
[    0.189167] rtc-generic rtc-generic: rtc core: registered rtc-generic as rtc0
[    0.189672] ledtrig-cpu: registered to indicate activity on CPUs
[    0.190173] TCP: cubic registered
[    0.190376] NET: Registered protocol family 10
[    0.194667] mip6: Mobile IPv6
[    0.194808] NET: Registered protocol family 17
[    0.195066] mpls_gso: MPLS GSO support
[    0.196061] registered taskstats version 1
[    0.198442] rtc-generic rtc-generic: hctosys: unable to read the hardware clock
[    0.202483] List of all partitions:
[    0.202603] No filesystem could mount root, tried: 
[    0.202801] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
[    0.203154] CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0-6-powerpc #1 Debian 3.16.56-1+deb8u1
[    0.203362] Call Trace:
[    0.203486] [df83bdc0] [c000966c] show_stack+0xf8/0x1b0 (unreliable)
[    0.203678] [df83be10] [c0504124] panic+0xdc/0x240
[    0.203762] [df83be70] [c06779f4] mount_root+0x0/0x68
[    0.203830] [df83bed0] [c0677bb0] prepare_namespace+0x154/0x19c
[    0.203973] [df83bef0] [c0677604] kernel_init_freeable+0x1e4/0x1f8
[    0.204050] [df83bf30] [c0004ab0] kernel_init+0x24/0x114
[    0.204118] [df83bf40] [c0015420] ret_from_kernel_thread+0x5c/0x64
[    0.204502] ---[ end Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
via_superio_cfg: unimplemented register 0xf2
via_superio_cfg: unimplemented register 0xf4
via_superio_cfg: unimplemented register 0xf6
via_superio_cfg: unimplemented register 0xf7
vof_finddevice "/chosen" => ph=0x3
vof_finddevice "/chosen" => ph=0x3
vof_write ih=0x1 [58] "
zImage starting: loaded at 0x00400000 (sp: 0x00cccfa0)
"
vof_finddevice "/chosen" => ph=0x3
vof_write ih=0x1 [42] "Allocating 0x8a1ec0 bytes for kernel ...
"
vof_finddevice "/openprom" => ph=0x2
vof_write ih=0x1 [29] "OF version = 'Pegasos2,1.1'
"
vof_write ih=0x1 [67] "Trying to claim from 0x400000 to 0xcd8dfc (0x8d8dfc) got ffffffff
"
vof_write ih=0x1 [51] "gunzipping (0x00d00000 <- 0x0040c000:0x00761c62)..."
vof_write ih=0x1 [21] "done 0x743000 bytes
"
vof_write ih=0x1 [48] "Attached initrd image at 0x00762000-0x00ccb90d
"
vof_write ih=0x1 [42] "Allocating 0x56990d bytes for initrd ...
"
vof_write ih=0x1 [60] "Relocating initrd 0x15a2000 <- 0x00762000 (0x56990d bytes)
"
vof_write ih=0x1 [25] "initrd head: 0x1f8b0808
"
vof_setprop ph=0x3 "linux,initrd-start" [015A2000] len=4 => ret=4
vof_setprop ph=0x3 "linux,initrd-end" [01B0B90D] len=4 => ret=4
vof_write ih=0x1 [22] "
Linux/PowerPC load: "
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x3 "bootargs" [] len=1 => ret=1
vof_write ih=0x1 [25] "Finalizing device tree..."
vof_write ih=0x1 [35] " using OF tree (promptr=0000010c)
"
vof_finddevice "/chosen" => ph=0x3
vof_finddevice "/" => ph=0x1
vof_finddevice "/openprom" => ph=0x2
vof_write ih=0x1 [21] "OF stdout device is: "
vof_write ih=0x1 [9] "/failsafe"
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x3 "linux,stdout-path" [/failsafe] len=10 => ret=10
vof_setprop ph=0x3 "linux,stdout-package" [00000008] len=4 => ret=4
vof_write ih=0x1 [18] "Preparing to boot "
vof_write ih=0x1 [141] "Linux version 3.16.0-6-powerpc (debian-kernel@lists.debian.org) (gcc version 4.8.4 (Debian 4.8.4-1) ) #1 Debian 3.16.56-1+deb8u1 (2018-05-08)"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [23] "Detected machine type: "
vof_write ih=0x1 [8] "00000500"
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x3 "linux,initrd-start" [00000000 015A2000] len=8 => ret=8
vof_setprop ph=0x3 "linux,initrd-end" [00000000 01B0B90D] len=8 => ret=8
vof_write ih=0x1 [14] "command line: "
vof_write ih=0x1 [31] "console=ttyS0,9600 console=tty0"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "memory layout at init:"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  memory_limit : "
vof_write ih=0x1 [8] "00000000"
vof_write ih=0x1 [16] " (16 MB aligned)"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  alloc_bottom : "
vof_write ih=0x1 [8] "01b0c000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  alloc_top    : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  alloc_top_hi : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  rmo_top      : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [17] "  ram_top      : "
vof_write ih=0x1 [8] "20000000"
vof_write ih=0x1 [2] "
"
vof_finddevice "/rtas" => ph=0x7
vof_write ih=0x1 [24] "instantiating rtas at 0x"
vof_write ih=0x1 [8] "1ffff000"
vof_write ih=0x1 [3] "..."
vof_finddevice "/rtas" => ph=0x7
vof_write ih=0x1 [5] " done"
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x7 "linux,rtas-base" [1FFFF000] len=4 => ret=4
vof_setprop ph=0x7 "linux,rtas-entry" [1FFFF000] len=4 => ret=4
vof_finddevice "/pci@80000000/isa@c" => ph=0xc
vof_write ih=0x1 [41] "Fixing up missing ISA range on Pegasos..."
vof_write ih=0x1 [2] "
"
vof_setprop ph=0xc "ranges" [00000001 00000000 01006000 00000000 00000000 00010000] len=24 => ret=24
vof_finddevice "/pci@80000000/ide@C,1" => ph=0x14
vof_write ih=0x1 [37] "Fixing up IDE interrupt on Pegasos..."
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x14 "interrupts" [0000000E 00000000] len=8 => ret=8
vof_write ih=0x1 [38] "Fixing up IDE class-code on Pegasos..."
vof_write ih=0x1 [2] "
"
vof_setprop ph=0x14 "class-code" [0001018A] len=4 => ret=4
vof_finddevice "/" => ph=0x1
vof_write ih=0x1 [25] "copying OF device tree..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "Building dt strings..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [24] "Building dt structure..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "Device tree strings 0x"
vof_write ih=0x1 [8] "01b0d000"
vof_write ih=0x1 [6] " -> 0x"
vof_write ih=0x1 [8] "01b0d430"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [22] "Device tree struct  0x"
vof_write ih=0x1 [8] "01b0e000"
vof_write ih=0x1 [6] " -> 0x"
vof_write ih=0x1 [8] "01b10000"
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [18] "Calling quiesce..."
vof_write ih=0x1 [2] "
"
vof_write ih=0x1 [24] "returning from prom_init"
vof_write ih=0x1 [2] "
"
Linux/PPC 3.16.0
arch: exitUnknown RTAS token 3 (args=0, rets=8)
               Have fun!    mv64361_write: Unimplemented register write 0x380 = 0
mv64361_write: Unimplemented register write 0x268 = f200
mv64361_write: Unimplemented register write 0x2220 = f2000002
mv64361_write: Unimplemented register write 0x2224 = 30000
mv64361_read: Unimplemented register read 0x2290
mv64361_write: Unimplemented register write 0x2290 = 0
[    0.000000] Using CHRP machine description
[    0.000000] Total memory = 512MB; using 1024kB for hash table (at cff00000)
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Initializing cgroup subsys cpu
[    0.000000] Initializing cgroup subsys cpuacct
[    0.000000] Linux version 3.16.0-6-powerpc (debian-kernel@lists.debian.org) (gcc version 4.8.4 (Debian 4.8.4-1) ) #1 Debian 3.16.56-1+deb8u1 (2018-05-08)
[    0.000000] Found initrd at 0xc15a2000:0xc1b0b90d
[    0.000000] chrp type = 6 [Genesi Pegasos]
[    0.000000] Pegasos l2cr : L2 cache was not active, activating
[    0.000000] PCI bus 0 controlled by /pci@80000000 at 80000000
[    0.000000] PCI host bridge /pci@80000000 (primary) ranges:
[    0.000000]   IO 0x00000000fe000000..0x00000000fe00ffff -> 0x0000000000000000
[    0.000000]  MEM 0x0000000080000000..0x00000000bfffffff -> 0x0000000080000000 
[    0.000000] PCI bus 0 controlled by /pci@c0000000 at c0000000
[    0.000000] PCI host bridge /pci@c0000000  ranges:
[    0.000000]   IO 0x00000000f8000000..0x00000000f800ffff -> 0x0000000000000000
[    0.000000]  MEM 0x00000000c0000000..0x00000000dfffffff -> 0x00000000c0000000 
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x00000000-0x1fffffff]
[    0.000000]   Normal   empty
[    0.000000]   HighMem  empty
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x00000000-0x1fffffff]
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 130048
[    0.000000] Kernel command line: console=ttyS0,9600 console=tty0
[    0.000000] PID hash table entries: 2048 (order: 1, 8192 bytes)
[    0.000000] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
[    0.000000] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
[    0.000000] Sorting __ex_table...
[    0.000000] Memory: 504272K/524288K available (5164K kernel code, 424K rwdata, 1428K rodata, 404K init, 1403K bss, 20016K reserved, 0K highmem)
[    0.000000] Kernel virtual memory layout:
[    0.000000]   * 0xfffcf000..0xfffff000  : fixmap
[    0.000000]   * 0xff800000..0xffc00000  : highmem PTEs
[    0.000000]   * 0xff7e0000..0xff800000  : early ioremap
[    0.000000]   * 0xe1000000..0xff7e0000  : vmalloc & ioremap
[    0.000000] NR_IRQS:512 nr_irqs:512 16
[    0.000000] i8259 legacy interrupt controller initialized
[    0.000424] clocksource: timebase mult[1e000005] shift[24] registered
[    0.004107] Console: colour dummy device 80x25
[    0.007217] console [tty0] enabled
[    0.007916] pid_max: default: 32768 minimum: 301
[    0.008387] Security Framework initialized
[    0.011148] AppArmor: AppArmor disabled by boot time parameter
[    0.011185] Yama: disabled by default; enable with sysctl kernel.yama.*
[    0.012046] Mount-cache hash table entries: 1024 (order: 0, 4096 bytes)
[    0.012078] Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes)
[    0.019923] Initializing cgroup subsys memory
[    0.020157] Initializing cgroup subsys devices
[    0.020263] Initializing cgroup subsys freezer
[    0.020342] Initializing cgroup subsys net_cls
[    0.020421] Initializing cgroup subsys blkio
[    0.020560] Initializing cgroup subsys perf_event
[    0.020618] Initializing cgroup subsys net_prio
[    0.020947] ftrace: allocating 17989 entries in 53 pages
[    0.056866] devtmpfs: initialized
[    0.061199] futex hash table entries: 256 (order: -1, 3072 bytes)
[    0.065690] NET: Registered protocol family 16
[    0.073135] PCI: Probing PCI hardware
[    0.074037] PCI host bridge to bus 0000:00
[    0.074354] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    0.074428] pci_bus 0000:00: root bus resource [mem 0x80000000-0xbfffffff]
[    0.074587] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.081087] PCI host bridge to bus 0001:01
[    0.081132] pci_bus 0001:01: root bus resource [io  0xffff0000-0xffffffff] (bus address [0x0000-0xffff])
[    0.081163] pci_bus 0001:01: root bus resource [mem 0xc0000000-0xdfffffff]
[    0.081199] pci_bus 0001:01: root bus resource [bus 01-ff]
[    0.083556] pci 0000:00:01.0: BAR 0: assigned [mem 0x80000000-0x80ffffff pref]
[    0.083742] pci 0000:00:01.0: BAR 2: assigned [mem 0x81000000-0x81003fff]
[    0.083822] pci 0000:00:01.0: BAR 1: assigned [io  0x1000-0x10ff]
[    0.083867] pci 0000:00:0c.2: BAR 4: assigned [io  0x1400-0x141f]
[    0.083891] pci 0000:00:0c.3: BAR 4: assigned [io  0x1420-0x143f]
[    0.083911] pci 0000:00:0c.1: BAR 4: assigned [io  0x1440-0x144f]
[    0.084009] pci 0000:00:0c.1: BAR 0: assigned [io  0x1450-0x1457]
[    0.084088] pci 0000:00:0c.1: BAR 2: assigned [io  0x1458-0x145f]
[    0.084162] pci 0000:00:0c.1: BAR 1: assigned [io  0x1460-0x1463]
[    0.084237] pci 0000:00:0c.1: BAR 3: assigned [io  0x1464-0x1467]
[    0.093647] vgaarb: device added: PCI:0000:00:01.0,decodes=io+mem,owns=none,locks=none
[    0.093690] vgaarb: loaded
[    0.093724] vgaarb: bridge control possible 0000:00:01.0
[    0.094694] SCSI subsystem initialized
[    0.100563] Switched to clocksource timebase
[    0.125541] NET: Registered protocol family 2
[    0.129321] TCP established hash table entries: 4096 (order: 2, 16384 bytes)
[    0.129395] TCP bind hash table entries: 4096 (order: 2, 16384 bytes)
[    0.129539] TCP: Hash tables configured (established 4096 bind 4096)
[    0.130129] TCP: reno registered
[    0.130188] UDP hash table entries: 256 (order: 0, 4096 bytes)
[    0.130291] UDP-Lite hash table entries: 256 (order: 0, 4096 bytes)
[    0.131359] NET: Registered protocol family 1
[    0.131799] pci 0000:00:0c.1: Fixing VIA IDE, force legacy mode on
[    0.132248] pci 0000:00:0c.2: enabling device (0000 -> 0001)
[    0.132893] pci 0000:00:0c.3: enabling device (0000 -> 0001)
[    0.137062] Unpacking initramfs...
[    0.365013] Freeing initrd memory: 5540K (c15a2000 - c1b0b000)
[    0.365853] Thermal assist unit using timers, shrink_timer: 500 jiffies
[    0.366910] ------------[ cut here ]------------
[    0.366941] WARNING: at /build/linux-aecbOt/linux-3.16.56/kernel/resource.c:638
[    0.366984] Modules linked in:
[    0.367156] CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0-6-powerpc #1 Debian 3.16.56-1+deb8u1
[    0.367229] task: df81f800 ti: df83a000 task.ti: df83a000
[    0.367250] NIP: c0047678 LR: c004765c CTR: 00000000
[    0.367273] REGS: df83bd50 TRAP: 0700   Not tainted  (3.16.0-6-powerpc Debian 3.16.56-1+deb8u1)
[    0.367294] MSR: 00029032 <EE,ME,IR,DR,RI>  CR: 84000028  XER: 00000000
[    0.367389] 
[    0.367389] GPR00: c004765c df83be00 df81f800 c06df368 c06df368 c05ce55c f1003fff c06df37c 
[    0.367389] GPR08: f1003fff 00000001 c06df368 00000000 84000084 00000000 c0004a8c 00000000 
[    0.367389] GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 c07435b0 0000006a 
[    0.367389] GPR24: c066f814 c0740000 df9992e0 c06e0000 c06e0000 c06df058 c06e1150 c06df368 
[    0.367564] NIP [c0047678] __insert_resource+0x4c/0x1b0
[    0.367587] LR [c004765c] __insert_resource+0x30/0x1b0
[    0.367628] Call Trace:
[    0.367690] [df83be00] [c004765c] __insert_resource+0x30/0x1b0 (unreliable)
[    0.367754] [df83be10] [c00484b8] insert_resource+0x1c/0x3c
[    0.367771] [df83be20] [c03600f0] platform_device_add+0xb8/0x294
[    0.367786] [df83be40] [c0360c24] platform_add_devices+0x48/0xac
[    0.367800] [df83be60] [c0684230] mv643xx_eth_add_pds+0x40/0x15c
[    0.367814] [df83be80] [c00043c0] do_one_initcall+0xd0/0x240
[    0.367828] [df83bef0] [c067757c] kernel_init_freeable+0x15c/0x1f8
[    0.367842] [df83bf30] [c0004ab0] kernel_init+0x24/0x114
[    0.367855] [df83bf40] [c0015420] ret_from_kernel_thread+0x5c/0x64
[    0.367901] Instruction dump:
[    0.367978] 7c7e1b78 7c9f2378 7fc3f378 7fe4fb78 4bfffacd 7c6a1b79 7d49fa78 7f8af040 
[    0.368012] 7d290034 5529d97e 41820060 419e013c <0f090000> 2f890000 409e0154 810a0000 
[    0.368110] ---[ end trace bd39b5951481421b ]---
[    0.368225] platform orion-mdio: failed to claim resource 0
[    0.369422] Device 'mv643xx_eth.0' does not have a release() function, it is broken and must be fixed.
[    0.369461] ------------[ cut here ]------------
[    0.369474] WARNING: at /build/linux-aecbOt/linux-3.16.56/drivers/base/core.c:250
[    0.369488] Modules linked in:
[    0.369544] CPU: 0 PID: 1 Comm: swapper Tainted: G        W     3.16.0-6-powerpc #1 Debian 3.16.56-1+deb8u1
[    0.369564] task: df81f800 ti: df83a000 task.ti: df83a000
[    0.369576] NIP: c0358730 LR: c0358730 CTR: c02ba250
[    0.369587] REGS: df83bd50 TRAP: 0700   Tainted: G        W      (3.16.0-6-powerpc Debian 3.16.56-1+deb8u1)
[    0.369602] MSR: 00029032 <EE,ME,IR,DR,RI>  CR: 22008082  XER: 00000000
[    0.369626] 
[    0.369626] GPR00: c0358730 df83be00 df81f800 0000005a 00000000 00000000 ffffffff 000000a9 
[    0.369626] GPR08: c073143c c0730000 c073143c 00000000 22008022 00000000 c0004a8c 00000000 
[    0.369626] GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 c07435b0 0000006a 
[    0.369626] GPR24: c066f814 c0740000 df9992e0 c06ba190 df9992e0 df973100 c06df1e8 c06df1f0 
[    0.369714] NIP [c0358730] device_release+0xa8/0xb8
[    0.369726] LR [c0358730] device_release+0xa8/0xb8
[    0.369736] Call Trace:
[    0.369745] [df83be00] [c0358730] device_release+0xa8/0xb8 (unreliable)
[    0.369763] [df83be20] [c0273c34] kobject_cleanup+0xa4/0x1d8
[    0.369777] [df83be40] [c0360c48] platform_add_devices+0x6c/0xac
[    0.369791] [df83be60] [c0684230] mv643xx_eth_add_pds+0x40/0x15c
[    0.369805] [df83be80] [c00043c0] do_one_initcall+0xd0/0x240
[    0.369819] [df83bef0] [c067757c] kernel_init_freeable+0x15c/0x1f8
[    0.369833] [df83bf30] [c0004ab0] kernel_init+0x24/0x114
[    0.369846] [df83bf40] [c0015420] ret_from_kernel_thread+0x5c/0x64
[    0.369856] Instruction dump:
[    0.369865] 813f0148 2f890000 419e0010 81290020 2f890000 409effb0 809f0024 2f840000 
[    0.369889] 419e0018 3c60c061 38634bc4 481ac1b9 <0fe00000> 4bffff9c 809f0000 4bffffe8 
[    0.369925] ---[ end trace bd39b5951481421c ]---
[    0.372440] audit: initializing netlink subsys (disabled)
[    0.373132] audit: type=2000 audit(0.359:1): initialized
[    0.376291] zbud: loaded
[    0.376978] VFS: Disk quotas dquot_6.5.2
[    0.377080] Dquot-cache hash table entries: 1024 (order 0, 4096 bytes)
[    0.377911] msgmni has been set to 997
[    0.389705] alg: No test for stdrng (krng)
[    0.389905] Block layer SCSI generic (bsg) driver version 0.4 loaded (major 252)
[    0.390340] io scheduler noop registered
[    0.390392] io scheduler deadline registered
[    0.390646] io scheduler cfq registered (default)
[    0.392210] aty128fb 0000:00:01.0: enabling device (0000 -> 0003)
[    0.393109] aty128fb 0000:00:01.0: BAR 6: can't assign [??? 0x00000000 flags 0x20000000] (bogus alignment)
[    0.393158] aty128fb: ROM failed to map
[    0.393178] aty128fb: BIOS not located, guessing timings.
[    0.393444] aty128fb: Rage128 PF PRO AGP [chip rev 0x0] 16M 128-bit SDR SGRAM (1:1)
[    0.401235] Console: switching to colour frame buffer device 128x48
[    0.403022] fb0: ATY Rage128 frame buffer device on Rage128 PF PRO AGP
[    0.404434] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.407667] console [ttyS0] disabled
[    0.435242] serial8250.0: ttyS0 at I/O 0x2f8 (irq = 0, base_baud = 115200) is a 16550A
[    0.447914] console [ttyS0] enabled
[    0.448657] pmac_zilog: 0.6 (Benjamin Herrenschmidt <benh@kernel.crashing.org>)
[    0.449591] Serial: MPC52xx PSC UART driver
[    0.449838] Generic non-volatile memory driver v1.1
[    0.450153] Linux agpgart interface v0.103
[    0.451202] Warning: no ADB interface detected
[    0.452177] mousedev: PS/2 mouse device common for all mice
Unknown RTAS token 3 (args=0, rets=8)
[    0.453110] rtc-generic rtc-generic: rtc core: registered rtc-generic as rtc0
[    0.453719] ledtrig-cpu: registered to indicate activity on CPUs
[    0.454166] TCP: cubic registered
[    0.454394] NET: Registered protocol family 10
[    0.459003] mip6: Mobile IPv6
[    0.459164] NET: Registered protocol family 17
[    0.459361] mpls_gso: MPLS GSO support
[    0.460440] registered taskstats version 1
Unknown RTAS token 3 (args=0, rets=8)
[    0.463366] rtc-generic rtc-generic: hctosys: unable to read the hardware clock
[    0.498713] Freeing unused kernel memory: 404K (c0674000 - c06d9000)
[    0.728938] systemd-udevd[39]: starting version 215
[    0.756964] random: systemd-udevd: uninitialized urandom read (16 bytes read, 0 bits of entropy available)
[    0.904127] usbcore: registered new interface driver usbfs
[    0.916960] scsi0 : pata_via
[    0.927063] scsi1 : pata_via
[    0.928595] ata1: PATA max UDMA/100 cmd 0x1450 ctl 0x1460 bmdma 0x1440 irq 14
[    0.930546] ata2: PATA max UDMA/100 cmd 0x1458 ctl 0x1464 bmdma 0x1448 irq 15
[    0.938068] usbcore: registered new interface driver hub
[    0.948648] usbcore: registered new device driver usb
[    0.952328] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[    0.957914] uhci_hcd: USB Universal Host Controller Interface driver
[    0.970422] uhci_hcd 0000:00:0c.2: UHCI Host Controller
[    0.971894] uhci_hcd 0000:00:0c.2: new USB bus registered, assigned bus number 1
[    0.992030] uhci_hcd 0000:00:0c.2: detected 2 ports
[    1.000502] uhci_hcd 0000:00:0c.2: irq 9, io base 0x00001400
[    1.026201] usb usb1: New USB device found, idVendor=1d6b, idProduct=0001
[    1.027311] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    1.028353] usb usb1: Product: UHCI Host Controller
[    1.029414] usb usb1: Manufacturer: Linux 3.16.0-6-powerpc uhci_hcd
[    1.030443] usb usb1: SerialNumber: 0000:00:0c.2
[    1.066854] hub 1-0:1.0: USB hub found
[    1.078820] hub 1-0:1.0: 2 ports detected
[    1.105333] uhci_hcd 0000:00:0c.3: UHCI Host Controller
[    1.106425] uhci_hcd 0000:00:0c.3: new USB bus registered, assigned bus number 2
[    1.107438] uhci_hcd 0000:00:0c.3: detected 2 ports
[    1.128547] uhci_hcd 0000:00:0c.3: irq 9, io base 0x00001420
[    1.138930] usb usb2: New USB device found, idVendor=1d6b, idProduct=0001
[    1.139948] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    1.141457] usb usb2: Product: UHCI Host Controller
[    1.142404] usb usb2: Manufacturer: Linux 3.16.0-6-powerpc uhci_hcd
[    1.143332] usb usb2: SerialNumber: 0000:00:0c.3
[    1.173033] hub 2-0:1.0: USB hub found
[    1.176448] hub 2-0:1.0: 2 ports detected
[    1.247902] ata2.00: ATAPI: QEMU DVD-ROM, 2.5+, max UDMA/100
[    1.250402] ata2.00: configured for UDMA/100
[    1.260948] scsi 1:0:0:0: CD-ROM            QEMU     QEMU DVD-ROM     2.5+ PQ: 0 ANSI: 5
[    1.290800] sr0: scsi3-mmc drive: 4x/4x cd/rw xa/form2 tray
[    1.294224] cdrom: Uniform CD-ROM driver Revision: 3.20
[    1.306671] sr 1:0:0:0: Attached scsi generic sg0 type 5
BALATON Zoltan July 9, 2021, 1:34 p.m. UTC | #11 
On Fri, 9 Jul 2021, David Gibson wrote:
> On Thu, Jul 08, 2021 at 03:00:22PM +0200, BALATON Zoltan wrote:
>> On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
>>> On 08/07/2021 20:39, BALATON Zoltan wrote:
>>>> On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
>>>>> On 08/07/2021 20:18, 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;
>>>>>>
>>>>>> I'll try this later but I think I've seen guests using
>>>>>> setprop (Linux also does that for some property). How should
>>>>>> I allow that? Do I need a new callback for this? Could it be
>>>>>> allower unless there's a callback that could deby it? But
>>>>>> that was the previous way I think.
>>>>>
>>>>> A simple defined callback which always returns "true" should do.
>>>>
>>>> Yes but what's the point? That would just effectiverly disable this
>>>> change so if we need that, we could just as well keep the previous
>>>> behaviour which is to allow setprop unless there's a callback that
>>>> can decide otherwise. The spapr machine has such a callback so it
>>>> already does not allow all setprop and if I'll have a callback in
>>>> pegasos2 returning true that will allow what's allowed now so this
>>>> part of this patch does nothing indeed.
>>>>
>>>> Since guests could do all kinds of things that we don't know without
>>>> trying them restricting setprop is a good way to run into problems
>>>> with guests that were not tested that could otherwise just work.
>>>> Then we'll need another patch to enable that guest adding some more
>>>> properties to the list of allowed ones. Why it it a problem to allow
>>>> this by default in the first place and only reject changes for
>>>> machines that have a callback? Then I would not need more empty
>>>> callbacks in pegasos2.
>>>
>>>
>>> From here:
>>> https://patchwork.ozlabs.org/project/qemu-devel/patch/20210625055155.2252896-1-aik@ozlabs.ru/#2714158
>>>
>>> ===
>>>
>>>>>> +    if (vmo) {
>>>>>> +        VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
>>>>>> +
>>>>>> +        if (vmc->setprop &&
>>>>>> +            !vmc->setprop(ms, nodepath, propname, val, vallen)) {
>>>>>> +            goto trace_exit;
>>>>>
>>>>> This defaults to allowing the setprop if the machine doesn't provide a
>>>>> setprop callback.  I think it would be safer to default to prohibiting
>>>>> all setprops except those the machine explicitly allows.
>>>>
>>>>
>>>> Mmmm... I can imagine the client using the device tree as a temporary
>>>> storage. I'd rather add a trace for such cases.
>>>
>>> If they do, I think that's something we'll need to consider and
>>> account for that platform, rather than something we want to allow to
>>> begin with.
>>
>> I've seen that, yet I don't understand why. If I'll just add an empty
>> callback in pegasos2 to disable it then we're back to where we were before.
>> So my question is why do we want to explicitely enable setprop for every
>> guest when we encounter it one by one (especially if this works on other OF
>> implementations so guests are free to change the device tree therefore we
>> don't know in advance what are allowable properties). If you don't want it
>> for spapr I think you already have the callback for it that disallows it for
>> all but at a few properties but why change the default for other machines
>> that don't have a callback? If I can still add an empty callback that could
>> well be the default just to avoid more boilerplate in board code.
>
> Because I think hitting the failure and deciphering that we need to
> add setprop logic is likely to be less pain in the long run, than
> allowing setprop by default, then discovering things break because the
> guest expected that setprop to have some semantic effect beyond just
> changing the dt, and we never even realized it was doing it.

What semantic effect does setprop have on real OpenFirmware besides 
changing the device tree? I don't see the advantage in this (see in my 
reply to Alexey) but sent a patch to add the callback to pegasos2 so you 
can pick that if you want to keep this default or alternatively you can 
revert the two hunks from VOF fix v23 to get back to prevous default. Both 
would fix Linux on pegasos2 with VOF.

Regards,
BALATON Zoltan
Alexey Kardashevskiy July 9, 2021, 1:46 p.m. UTC | #12 
On 09/07/2021 23:28, BALATON Zoltan wrote:
> On Fri, 9 Jul 2021, Alexey Kardashevskiy wrote:
>> On 09/07/2021 08:34, BALATON Zoltan wrote:
>>> 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?
> 
> It's fixing up some props that on Pegasos2 firmware are not how Linux 
> expects them.

Why does it need to fix them then? You are building the FDT in QEMU, 
built it in the way Linux like and then you do not depend on the kernel 
fixing them up. What do I miss?

 From traces I see that (besides PCI) it mostly sets props for 
linux-initrd/bootargs which you rather need to handle to keep the 
machine's properties and the FDT in sync.


> Without this it's not booting. Attached is the trace 
> output with VOF v23 as it is now (nosetprop) and another one after the 
> patch that adds setprop callback to pegasos2 I'm sending separately. 
> That patch restores Linux boot but I still think all this boilerplate 
> would not be needed if we kept the default to allow setprop and that 
> results in overall simpler code. If something breaks becuase of enabling 
> setprop by default (which normally works on real Open Firmware) it's 
> easy enough to debug by enabling vof_setprop trace points so I don't see 
> this adds any value other than making board code more complex.
BALATON Zoltan July 9, 2021, 3:35 p.m. UTC | #13 
On Fri, 9 Jul 2021, Alexey Kardashevskiy wrote:
> On 09/07/2021 23:28, BALATON Zoltan wrote:
>> On Fri, 9 Jul 2021, Alexey Kardashevskiy wrote:
>>> On 09/07/2021 08:34, BALATON Zoltan wrote:
>>>> 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?
>> 
>> It's fixing up some props that on Pegasos2 firmware are not how Linux 
>> expects them.
>
> Why does it need to fix them then? You are building the FDT in QEMU, built it 
> in the way Linux like and then you do not depend on the kernel fixing them 
> up. What do I miss?

The SmartFirmware used on the real hardware has some quirks that Linux 
handles by fixing up the device tree. The board firmware has the device 
tree the way I build it. I want to be compatible with how things work with 
actual board firmware (thus replacing the non-distributable firmware blob 
with VOF but still allowing using a ROM image if needed) not just get 
things work with VOF by whatever hacks needed to get Linux boot. Keeping 
compatibility with board firmware is useful so other guests can work too 
and avoid possible conflicts with different assumptions from different 
guests that may need different hacks if not using board ROM and also it's 
simpler to debug by just comparing with what the real firmware has. These 
fix ups are needed with the board firmware so they are also needed with 
VOF which emulates the board firmware now, at least to the point of simple 
CI and RTAS that's enough for some guests to boot (currently Linux and 
MorphOS which are the two guests that work well enough with current state 
of emulation anyway; AmigaOS needs some graphics driver or better Radeon 
emulation then it would need more stuff from VOF as it calls methods to 
access disk blocks which may be something you did in your further patches 
for GRUB so I can get back to it when you get to GRUB support but that's 
not urgent as there's no ouput without a graphics driver in AmigaOS so not 
really useful to fix VOF for it now).

> From traces I see that (besides PCI) it mostly sets props for 
> linux-initrd/bootargs which you rather need to handle to keep the machine's 
> properties and the FDT in sync.

How do I handle that and what do I need to keep in sync? The pegasos2 does 
not support -initrd option as initrd is embedded in kernel and -append is 
just passed through to bootags. I don't use these in QEMU at all so the 
guest can do whatever it wants with them.

Regards,
BALATON Zoltan
diff mbox series

Patch

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);
@@ -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*