[Skiboot] [PATCH v2 2/2] core: Implement non-FSP dump region opal call

Oliver oohall at gmail.com
Tue May 7 14:27:43 AEST 2019


On Tue, May 7, 2019 at 10:46 AM Joel Stanley <joel at jms.id.au> wrote:
>
> Digging this back up again as it would have been handy for some
> debugging yesterday.
>
> On Mon, 28 May 2018 at 13:19, Benjamin Herrenschmidt
> <benh at kernel.crashing.org> wrote:
> >
> > On Mon, 2018-05-28 at 13:25 +0930, Joel Stanley wrote:
> > > This provides an implementation of the OPAL_REGISTER_DUMP_REGION and
> > > OPAL_UNREGISTER_DUMP_REGION calls that can be used by non-FSP systems,
> > > and performs that registration for all astbmc platforms.
> > >
> > > This pointer will be used by eg. pdbg to fetch the host kernel's log
> > > buffer over FSI.
> > >
> > > We unconditionally clear the value on a reboot (both fast-reboot and
> > > reliable reboot) to reduce the chance of debug tools getting the wrong
> > > buffer.
> >
> > I wouldn't make it BMC specific.
> >
> > Instead, I would suggest you:
> >
> >  - Add "generic" OPAL calls that populate the debug descritor (ie,
> > statically defined using OPAL_CALL).
> >
> >  - Make the above call an optional machine.register_dump_region
> >
> >  - Make FSP machines populate the above to *also* register with the
> > FSP.
> >
> > That way we can also use/create similar tools (or just getmemproc) to
> > get to the kernel dmesg via the debug descriptor on FSP systems.
>
> Oliver, Vasant: are you happy with the patch as-is? Or shall we go
> down the route that benh suggested?

Do what Ben suggested. Having a static opal call for REGISTER_DUMP
makes most of the header file ceremony in this patch redundant and
it's easier to follow what's going on if we don't runtime patch OPAL
call handlers.

> The advantage of the patch as-is is existing kernels that boot on new
> skiboot will register the debug descriptor.

This will be true for either approach.

>
> Cheers,
>
> Joel
>
>
> >
> > Cheers,
> > Ben.
> >
> > > Signed-off-by: Joel Stanley <joel at jms.id.au>
> > > ---
> > > v2:
> > >  Rename dump.h to dump-region.h
> > >  Add comment to init.c
> > > ---
> > >  core/Makefile.inc         |  2 +-
> > >  core/dump-region.c        | 61 +++++++++++++++++++++++++++++++++++++++
> > >  core/init.c               |  4 +++
> > >  include/dump-region.h     | 26 +++++++++++++++++
> > >  platforms/astbmc/common.c |  4 +++
> > >  5 files changed, 96 insertions(+), 1 deletion(-)
> > >  create mode 100644 core/dump-region.c
> > >  create mode 100644 include/dump-region.h
> > >
> > > diff --git a/core/Makefile.inc b/core/Makefile.inc
> > > index d36350590edb..2d2f74778f4c 100644
> > > --- a/core/Makefile.inc
> > > +++ b/core/Makefile.inc
> > > @@ -9,7 +9,7 @@ CORE_OBJS += vpd.o hostservices.o platform.o nvram.o nvram-format.o hmi.o
> > >  CORE_OBJS += console-log.o ipmi.o time-utils.o pel.o pool.o errorlog.o
> > >  CORE_OBJS += timer.o i2c.o rtc.o flash.o sensor.o ipmi-opal.o
> > >  CORE_OBJS += flash-subpartition.o bitmap.o buddy.o pci-quirk.o powercap.o psr.o
> > > -CORE_OBJS += pci-dt-slot.o direct-controls.o cpufeatures.o
> > > +CORE_OBJS += pci-dt-slot.o direct-controls.o cpufeatures.o dump-region.o
> > >
> > >  ifeq ($(SKIBOOT_GCOV),1)
> > >  CORE_OBJS += gcov-profiling.o
> > > diff --git a/core/dump-region.c b/core/dump-region.c
> > > new file mode 100644
> > > index 000000000000..e42486f99ebf
> > > --- /dev/null
> > > +++ b/core/dump-region.c
> > > @@ -0,0 +1,61 @@
> > > +/* Copyright 2018 IBM Corp.
> > > + *
> > > + * Licensed under the Apache License, Version 2.0 (the "License");
> > > + * you may not use this file except in compliance with the License.
> > > + * You may obtain a copy of the License at
> > > + *
> > > + *   http://www.apache.org/licenses/LICENSE-2.0
> > > + *
> > > + * Unless required by applicable law or agreed to in writing, software
> > > + * distributed under the License is distributed on an "AS IS" BASIS,
> > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > > + * implied.
> > > + * See the License for the specific language governing permissions and
> > > + * limitations under the License.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "DUMP: " fmt
> > > +
> > > +#include <opal.h>
> > > +#include <skiboot.h>
> > > +
> > > +#include <dump-region.h>
> > > +
> > > +static int64_t bmc_opal_register_dump_region(uint32_t id,
> > > +                                          uint64_t addr, uint64_t size)
> > > +{
> > > +     if (id != OPAL_DUMP_REGION_LOG_BUF) {
> > > +             prerror("Unsupported log region id %02x\n", id);
> > > +             return OPAL_UNSUPPORTED;
> > > +     }
> > > +
> > > +     if (size <= 0) {
> > > +             prerror("Invalid log size %lld\n", size);
> > > +             return OPAL_PARAMETER;
> > > +     }
> > > +
> > > +     prlog(PR_INFO, "Registered log buf at 0x%016llx\n", addr);
> > > +     debug_descriptor.log_buf_phys = addr;
> > > +
> > > +     return OPAL_SUCCESS;
> > > +}
> > > +
> > > +static int64_t bmc_opal_unregister_dump_region(uint32_t id)
> > > +{
> > > +     if (id != OPAL_DUMP_REGION_LOG_BUF) {
> > > +             prlog(PR_DEBUG, "Unsupported log region id %02x\n", id);
> > > +             return OPAL_UNSUPPORTED;
> > > +     }
> > > +     prlog(PR_INFO, "Unregistered log buf\n");
> > > +     debug_descriptor.log_buf_phys = 0;
> > > +
> > > +     return OPAL_SUCCESS;
> > > +}
> > > +
> > > +void bmc_dump_region_init(void)
> > > +{
> > > +     opal_register(OPAL_REGISTER_DUMP_REGION,
> > > +                   bmc_opal_register_dump_region, 3);
> > > +     opal_register(OPAL_UNREGISTER_DUMP_REGION,
> > > +                   bmc_opal_unregister_dump_region, 1);
> > > +};
> > > diff --git a/core/init.c b/core/init.c
> > > index 3b887a24d11c..c1c352858e06 100644
> > > --- a/core/init.c
> > > +++ b/core/init.c
> > > @@ -563,6 +563,10 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
> > >
> > >       mem_dump_free();
> > >
> > > +     /* Zero out memory location before the next kernel starts, in case the
> > > +      * previous kernel did not unregister */
> > > +     debug_descriptor.log_buf_phys = 0;
> > > +
> > >       /* Take processours out of nap */
> > >       cpu_set_sreset_enable(false);
> > >       cpu_set_ipi_enable(false);
> > > diff --git a/include/dump-region.h b/include/dump-region.h
> > > new file mode 100644
> > > index 000000000000..b629e66a85e6
> > > --- /dev/null
> > > +++ b/include/dump-region.h
> > > @@ -0,0 +1,26 @@
> > > +/* Copyright 2018 IBM Corp.
> > > + *
> > > + * Licensed under the Apache License, Version 2.0 (the "License");
> > > + * you may not use this file except in compliance with the License.
> > > + * You may obtain a copy of the License at
> > > + *
> > > + *   http://www.apache.org/licenses/LICENSE-2.0
> > > + *
> > > + * Unless required by applicable law or agreed to in writing, software
> > > + * distributed under the License is distributed on an "AS IS" BASIS,
> > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > > + * implied.
> > > + * See the License for the specific language governing permissions and
> > > + * limitations under the License.
> > > + */
> > > +
> > > +#ifndef __DUMP_H
> > > +#define __DUMP_H
> > > +
> > > +/*
> > > + * Initialise dump region registration OPAL calls for bmc systems.
> > > + */
> > > +void bmc_dump_region_init(void);
> > > +
> > > +#endif
> > > +
> > > diff --git a/platforms/astbmc/common.c b/platforms/astbmc/common.c
> > > index 30c2714f94b9..ca8ec7e8c0ce 100644
> > > --- a/platforms/astbmc/common.c
> > > +++ b/platforms/astbmc/common.c
> > > @@ -26,6 +26,7 @@
> > >  #include <bt.h>
> > >  #include <errorlog.h>
> > >  #include <lpc.h>
> > > +#include <dump-region.h>
> > >
> > >  #include "astbmc.h"
> > >
> > > @@ -156,6 +157,9 @@ void astbmc_init(void)
> > >
> > >       /* Add BMC firmware info to device tree */
> > >       ipmi_dt_add_bmc_info();
> > > +
> > > +     /* Enable dump region OPAL calls */
> > > +     bmc_dump_region_init();
> > >  }
> > >
> > >  int64_t astbmc_ipmi_power_down(uint64_t request)


More information about the Skiboot mailing list