[Skiboot] [PATCH v7 12/22] hdata: Add result table property to "dump" node

Vasant Hegde hegdevasant at linux.vnet.ibm.com
Tue May 14 21:50:09 AEST 2019


On 05/10/2019 08:59 AM, Oliver wrote:
> On Sat, Apr 13, 2019 at 7:19 PM Vasant Hegde
> <hegdevasant at linux.vnet.ibm.com> wrote:
>>
>> During MPIPL hostboot updates MDRT table with details of memory movement
>> (source, destination address, size, etc). Convert these details to fadump
>> structure format and pass it to kernel via 'result-table' property.
>>
>> Device tree property:
>>    /ibm,opal/dump/result-table - follows fadump structure format
>>
>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>> ---
>>   hdata/spira.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hdata/spira.h |  4 +++
>>   2 files changed, 97 insertions(+)
>>
>> diff --git a/hdata/spira.c b/hdata/spira.c
>> index 34e3034ce..fa6149322 100644
>> --- a/hdata/spira.c
>> +++ b/hdata/spira.c
>> @@ -26,6 +26,8 @@
>>   #include <fsp-attn.h>
>>   #include <fsp-leds.h>
>>   #include <skiboot.h>
>> +#include <mem_region-malloc.h>
>> +#include <opal-dump.h>
>>
>>   #include "hdata.h"
>>   #include "hostservices.h"
>> @@ -1055,6 +1057,94 @@ 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 opal_dump_add_result_table(const struct iplparams_iplparams *p)
>> +{
>> +       int i, j = 0;
>> +       u32 mdrt_cnt = spira.ntuples.mdump_res.act_cnt;
>> +       u32 mdrt_max_cnt = MDRT_TABLE_SIZE / sizeof(struct mdrt_table);
>> +       size_t prop_size;
>> +       struct mdrt_table *mdrt = (void *)(MDRT_TABLE_BASE);
>> +       struct fadump_section *fadump_section;
>> +       struct fadump *result_table;
>> +       struct dt_node *dump_node;
>> +
>> +       dump_node = dt_find_by_path(opal_node, "dump");
>> +       if (!dump_node)
>> +               return;
>> +
>> +       /* Check boot params to detect MPIPL boot or not */
>> +       if (p->cec_ipl_maj_type != IPLPARAMS_MAJ_TYPE_REIPL)
>> +               return;
>> +       /*
>> +        * On FSP system we get minor type as post dump IPL and on BMC system
>> +        * we get platform reboot. Hence lets check for both values
>> +        */
>> +       if (p->cec_ipl_min_type != IPLPARAMS_MIN_TYPE_POST_DUMP &&
>> +           p->cec_ipl_min_type != IPLPARAMS_MIN_TYPE_PLAT_REBOOT) {
>> +               prlog(PR_DEBUG, "DUMP: Non MPIPL reboot "
>> +                     "[minor type = 0x%x]\n", p->cec_ipl_min_type);
>> +               return;
>> +       }
>> +       if (p->cec_ipl_attrib != IPLPARAMS_ATTRIB_MEM_PRESERVE) {
>> +               prlog(PR_DEBUG, "DUMP: Memory not preserved\n");
>> +               return;
>> +       }
>> +
>> +       if (mdrt_cnt == 0 || mdrt_cnt >= mdrt_max_cnt) {
>> +               prlog(PR_DEBUG, "DUMP: Invalid MDRT count : %x\n", mdrt_cnt);
>> +               return;
>> +       }
>> +
>> +       prlog(PR_NOTICE, "DUMP: Dump found, MDRT count = 0x%x\n", mdrt_cnt);
>> +
>> +       /* Calculcate property size */
>> +       prop_size = sizeof(struct fadump) +
>> +               (mdrt_cnt * sizeof(struct fadump_section));
>> +       result_table = zalloc(prop_size);
>> +       if (!result_table) {
>> +               prlog(PR_ERR, "DUMP: Failed to allocate memory\n");
>> +               return;
>> +       }
>> +
>> +       /* Copy MDRT entries to result-table */
>> +       result_table->fadump_section_size = sizeof(struct fadump_section);
> 
>> +       for (i = 0; i < mdrt_cnt; i++) {
>> +               /* Skip garbage entry */
>> +               if ((mdrt->dest_addr == 0) || (mdrt->size == 0)) {
>> +                       mdrt++;
>> +                       continue;
>> +               }
>> +
>> +               if (mdrt->dump_type != DUMP_TYPE_FADUMP) {
>> +                       mdrt++;
>> +                       continue;
>> +               }
> You already have a loop counter, why are you doing pointer arithmetic?

So that I can access data below. Otherwise every time I have to calculate offset
and then access data. Something like below :
     mdrt =  (void *)(MDRT_TABLE_BASE + sizeof(struct mdrt_table));

> 
>> +
>> +               fadump_section = &(result_table->section[j++]);
>> +               fadump_section->source_type = mdrt->data_region;
>> +               /* Clear top bit */
>> +               fadump_section->source_addr =
>> +                       cpu_to_be64(mdrt->src_addr & ~(HRMOR_BIT));
>> +               fadump_section->dest_addr =
>> +                       cpu_to_be64(mdrt->dest_addr & ~(HRMOR_BIT));
>> +               fadump_section->source_size = cpu_to_be64(mdrt->size);
>> +               fadump_section->dest_size = cpu_to_be64(mdrt->size);
>> +               mdrt++;
>> +       }
>> +
>> +       if (j == 0) {
>> +               prlog(PR_DEBUG, "DUMP: MDRT table is empty\n");
>> +               free(result_table);
>> +               return;
>> +       }
>> +
>> +       result_table->section_count = cpu_to_be16(j);
>> +       /* Actual property size */
>> +       prop_size = sizeof(struct fadump) + (j * sizeof(struct fadump_section));
>> +       dt_add_property(dump_node, "result-table", result_table, prop_size);
>> +       free(result_table);
> 
> I'm not thrilled with this. The DT is used to communicate static
> configuration data from firmware to the OS and the results table is
> not static. Whatever kernel ends up consuming the results invalidates
> the table leaving you with stuff in the DT with no basis in reality.

Data content is static. Its not going to change. Once kernel invalidates dump
we release destination memory. But the assumption is by then we have captured
the data and we will not look into those memory again. Also current design is kernel
reboots after capturing data.

> Although we can add code to the kernel to prune the old entry, I'd
> rather not since:
> 
> a) it's a weird edge case,
> b) it goes against the convention of a static DT, and
> c) there's no reason to do it other than expedience.

agree. Kernel shouldn't modify DT entry.

> 
> For an alternative you could put a pointer to the result-table in the
> DT to a bit of skiboot owned memory. When the kernel is done with it
> could clear it to indicate to any subsequent kernels that it's been
> processed. That all said, it seems to me that a lot of the code to
> support this does nothing but marshalling and unmarshalling <address,
> size> pairs from the dump descriptor tables so maybe there's probably
> a better API to be found here.

May be passing <addr, size> is better option?

-Vasant



More information about the Skiboot mailing list