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

[David Gibson]

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                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature