| Message ID | 20180524013720.1589-3-joel@jms.id.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | BMC dumping | expand |
On 05/24/2018 07:07 AM, 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. Cool! > > 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. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > core/Makefile.inc | 2 +- > core/dump-region.c | 61 +++++++++++++++++++++++++++++++++++++++ I'm bit concerned about the filename here. I've patchset to add MPIPL support in OPAL. I've added core/opal-dump.c file. Also note that I've renamed hw/fsp/fsp-mdst-table.c as fsp-sysdump.c. So having multiple files with *dump* may confuse. But I can't think of any better name here. May be I should rename mpipl stuff as opal-mpipl.c or opal-fadump.c > core/init.c | 2 ++ > include/dump.h | 26 +++++++++++++++++ Better rename this as dump-region.h or just move declaration to skiboot.h itself? > platforms/astbmc/common.c | 4 +++ > 5 files changed, 94 insertions(+), 1 deletion(-) > create mode 100644 core/dump-region.c > create mode 100644 include/dump.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..6337e8bf0728 > --- /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.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..1ef7d800ae9a 100644 > --- a/core/init.c > +++ b/core/init.c > @@ -563,6 +563,8 @@ void __noreturn load_and_boot_kernel(bool is_reboot) > > mem_dump_free(); > May be add comment here explaining why you are clearing log_buf_phsy here? > + debug_descriptor.log_buf_phys = 0; > + -Vasant
On 25 May 2018 at 15:46, Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote: > On 05/24/2018 07:07 AM, 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. > > > Cool! > >> >> 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. >> >> Signed-off-by: Joel Stanley <joel@jms.id.au> >> --- >> core/Makefile.inc | 2 +- >> core/dump-region.c | 61 +++++++++++++++++++++++++++++++++++++++ > > > I'm bit concerned about the filename here. I've patchset to add MPIPL > support in OPAL. > I've added core/opal-dump.c file. Also note that I've renamed > hw/fsp/fsp-mdst-table.c as > fsp-sysdump.c. So having multiple files with *dump* may confuse. But I can't > think of > any better name here. May be I should rename mpipl stuff as opal-mpipl.c or > opal-fadump.c I've named it after the opal call that it implements. I don't think there's anything wrong with having fsp-sysdump (or even fsp-dump-region), as it's pretty clear what is going on. I'd suggest we name things after the opal/skiboot features they implement, and if there are external features that use these features that's fine, but it doesn't need to influcence the naming. (For example, I didn't call this file pdbg-dmesg, even though that's what the first user will be). An enhancement that could be made here is to have the one dump-region.c that registers the debug descriptor for all platforms, and if it's on FSP also calls into the mdst/sysdump platform code. I played with this idea but the code didn't come out very clean, so it would require more effort >> core/init.c | 2 ++ >> include/dump.h | 26 +++++++++++++++++ > > > Better rename this as dump-region.h or just move declaration to skiboot.h > itself? I'll rename it. >> diff --git a/core/init.c b/core/init.c >> index 3b887a24d11c..1ef7d800ae9a 100644 >> --- a/core/init.c >> +++ b/core/init.c >> @@ -563,6 +563,8 @@ void __noreturn load_and_boot_kernel(bool is_reboot) >> mem_dump_free(); >> > > > May be add comment here explaining why you are clearing log_buf_phsy here? Ok. Thanks for the review. Cheers, Joel
On 05/28/2018 09:16 AM, Joel Stanley wrote: > On 25 May 2018 at 15:46, Vasant Hegde <hegdevasant@linux.vnet.ibm.com> wrote: >> On 05/24/2018 07:07 AM, Joel Stanley wrote: .../... > > I've named it after the opal call that it implements. > > I don't think there's anything wrong with having fsp-sysdump (or even > fsp-dump-region), as it's pretty clear what is going on. > > I'd suggest we name things after the opal/skiboot features they > implement, and if there are external features that use these features > that's fine, but it doesn't need to influcence the naming. (For > example, I didn't call this file pdbg-dmesg, even though that's what > the first user will be). > > An enhancement that could be made here is to have the one > dump-region.c that registers the debug descriptor for all platforms, > and if it's on FSP also calls into the mdst/sysdump platform code. I > played with this idea but the code didn't come out very clean, so it > would require more effort I had thought about this when I reviewed this patch. But the way we use dump region is completely different here. Also we don't run pdgb on FSP anyway. So current code is fine for now. And if we need, we can create platform specific hooks later. > >>> core/init.c | 2 ++ >>> include/dump.h | 26 +++++++++++++++++ >> >> >> Better rename this as dump-region.h or just move declaration to skiboot.h >> itself? > > I'll rename it. Cool. -Vasant
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..6337e8bf0728 --- /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.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..1ef7d800ae9a 100644 --- a/core/init.c +++ b/core/init.c @@ -563,6 +563,8 @@ void __noreturn load_and_boot_kernel(bool is_reboot) mem_dump_free(); + 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.h b/include/dump.h new file mode 100644 index 000000000000..b629e66a85e6 --- /dev/null +++ b/include/dump.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 eaebfde5f38e..82069c841cf4 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.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)
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. Signed-off-by: Joel Stanley <joel@jms.id.au> --- core/Makefile.inc | 2 +- core/dump-region.c | 61 +++++++++++++++++++++++++++++++++++++++ core/init.c | 2 ++ include/dump.h | 26 +++++++++++++++++ platforms/astbmc/common.c | 4 +++ 5 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 core/dump-region.c create mode 100644 include/dump.h