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

[Vasant Hegde]

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 at 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