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

[Vasant Hegde]

On 06/28/2019 06:54 AM, Nicholas Piggin wrote:
> 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,

Sure. I will try to rephrase.

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

OS developer shouldn't be worried about internal behaviour of OPAL -OR- how it 
interacts
with FSP/BMC etc. All he should care about is device tree details and OPAL APIs. 
And we
have to make sure we have enough documentation around that.

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

Yes. OPAL tells Linux that these are the memory area we may stomp during boot 
process.
If you care about these memory to generate proper dump then  you have to preserve it
(basically kernel has to allocate destination memory to preserve it and add 
entry to MPIPL table).


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

So these are the memory OPAL uses to load petitboot kernel/initrd from BMC. Its 
one time
activity during boot. Hence I don't think we have to add/reserve this memory. We 
will publish
the range. Let kernel deal with that.

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

Probably. But then we are trusting HDAT too much ;-)

> 
>> +
>> +	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).

I do have documentation at the end. I will update it here as well.

-Vasant