[Skiboot] [PATCH v8 09/24] hdata: Create /ibm, opal/dump device tree node

[Nicholas Piggin]

All the previous firmware related patches I don't know enough about,
so hopefully someone else can review them.

Vasant Hegde's on June 17, 2019 3:10 am:
> We use MPIPL system parameter to detect whether MPIPL is supported or not.
> If its supported create new device tree node (/ibm,opal/dump) to pass all
> dump related information to kernel. This patch creates new node and populates
> below properties:
>   - compatible   - dump version (ibm,opal-dump)

I guess this seems like a reasonable way to advertise it to the OS.

>   - fw-load-area - Memory used by OPAL to load kernel/initrd from PNOR
>                    (KERNEL_LOAD_BASE & INITRAMFS_LOAD_BASE)
>                    This is the temporary memory used by OPAL during boot.
> 		   Later Linux kernel is free to use this memory. We will
> 		   pass this information to Linux. If Linux kernel is using
> 		   these memory, it will make sure to create destination
> 		   memory to preserve content during MPIPL.

This is difficult to understand what to do, let alone why.

Can you re-word it? If you have documented the dt changes anywhere,
you could re-use your documentation here (or point to the documentation
in the changelog). When you write the documentation, you should think of
some poor OS developer who has no idea about what hostboot or FSP or 
spira means -- could a FreeBSD developer read your docs and add fadump
support for their OS?

Am I understanding it correctly, the OS is not allowed to overwrite some
of this memory if it wants to use MPIPL? And if it does then it should
somehow preserve it?

Can you just mandate that the memory doesn't get used in that case? And
add it to OPAL's general firmware memory range code?

> 
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> ---
>  hdata/spira.c | 24 ++++++++++++++++++++++++
>  hdata/spira.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/hdata/spira.c b/hdata/spira.c
> index b5ec20db9..d2ad8e168 100644
> --- a/hdata/spira.c
> +++ b/hdata/spira.c
> @@ -934,6 +934,26 @@ static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams)
>  	dt_add_property_cells(node, "hw-key-hash-size", hw_key_hash_size);
>  }
>  
> +static void add_opal_dump_node(void)
> +{
> +	u64 fw_load_area[4];
> +	struct dt_node *node;
> +
> +	if (proc_gen < proc_gen_p9)
> +		return;

Already have a test for SYS_ATTR_MPIPL_SUPPORTED so this shouldn't be
necessary.

> +
> +	opal_node = dt_new_check(dt_root, "ibm,opal");
> +	node = dt_new(opal_node, "dump");
> +	assert(node);
> +	dt_add_property_string(node, "compatible", "ibm,opal-dump");
> +
> +	fw_load_area[0] = cpu_to_be64((u64)KERNEL_LOAD_BASE);
> +	fw_load_area[1] = cpu_to_be64(KERNEL_LOAD_SIZE);
> +	fw_load_area[2] = cpu_to_be64((u64)INITRAMFS_LOAD_BASE);
> +	fw_load_area[3] = cpu_to_be64(INITRAMFS_LOAD_SIZE);
> +	dt_add_property(node, "fw-load-area", fw_load_area, sizeof(fw_load_area));

This is a nasty opaque kind of data structure that's best to be avoided
if possible (or well documented, and documentation should be added with
the patch that adds the dt IMO).

reserved-memory device tree on the other hand is self descriptive and
well used and understood.

Thanks,
Nick