[Skiboot] [PATCH] hdata: Make sure reserved node name starts with "ibm, "

[Vasant Hegde]

On 06/25/2018 08:03 AM, Oliver wrote:
> On Sat, Jun 23, 2018 at 4:23 PM, Vasant Hegde
> <hegdevasant at linux.vnet.ibm.com> wrote:
>> HDAT does not use consistent label format for reserved memory label.
>> Few starts with "ibm," while few other starts with component name.
>>
>> Lets make sure all the reserved node name starts with "ibm,".
>>
>> Reported-by: Pridhiviraj Paidipeddi <ppaidipe at linux.vnet.ibm.com>
>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>> ---
>> @Stewart,
>>    This fixes https://github.com/open-power/boston-openpower/issues/675
>>
>> -Vasant
>>
>>   hdata/memory.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/hdata/memory.c b/hdata/memory.c
>> index ecce881d1..c0c30066e 100644
>> --- a/hdata/memory.c
>> +++ b/hdata/memory.c
>> @@ -27,6 +27,8 @@
>>   #include "spira.h"
>>   #include "hdata.h"
> 
>> +#define FW_RESERVE_NODE_PREFIX "ibm,"
>> +
> 
> Is this really better than just using the string literal?

Yeah. I can make it string literal.

> 
>>   struct HDIF_ram_area_id {
>>          __be16 id;
>>   #define RAM_AREA_INSTALLED     0x8000
>> @@ -595,7 +597,7 @@ static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
>>          const struct msvpd_hb_reserved_mem *hb_resv_mem;
>>          u64 start_addr, end_addr, label_size;
>>          struct dt_node *node;
>> -       int count, i;
>> +       int count, i, label_offset;
>>          char *label;
>>
>>          /*
>> @@ -643,11 +645,19 @@ static void get_hb_reserved_mem(struct HDIF_common_hdr *ms_vpd)
>>                  if (label_size > 64)
>>                          label_size = 64;
>>
>> -               label = malloc(label_size+1);
>> +               /* Accommodate "ibm," */
>> +               label_offset = 0;
>> +               label = malloc(label_size + strlen(FW_RESERVE_NODE_PREFIX) + 1);
> 
> Come to think of it we could probably eliminate the malloc()ed label
> in favour of a stack allocated array. The HDAT labels are only 64
> bytes and that's not much more than a bare stack frame.

Yeah. We can avoid malloc here. will fix it in v2.

> 
>>                  assert(label);
>>
>> -               memcpy(label, hb_resv_mem->label, label_size);
>> -               label[label_size] = '\0';
>> +               /* Add "ibm," to reserved node name */
>> +               if (strncasecmp(hb_resv_mem->label, "ibm", 3)) {
>> +                       memcpy(label, FW_RESERVE_NODE_PREFIX,
>> +                              strlen(FW_RESERVE_NODE_PREFIX));
>> +                       label_offset = strlen(FW_RESERVE_NODE_PREFIX);
> 
> Using strncat() or even snprintf() would probably be clearer

Ok.

> 
>> +               }
>> +               memcpy(label + label_offset, hb_resv_mem->label, label_size);
>> +               label[label_size + label_offset] = '\0';
> 
> I'd put the name canonicalization into add_hb_reserve_node() rather
> than here. Opal-prd uses the ibm,prd-label property to find reserved
> regions that are exported by hostboot so the label needs to be left
> unchanged here.

Oh yeah. I missed prd-label issue. I do not wanted to change `trace-area` stuff. 
Hence I fixed it here.

Are we fine with converting "trace-area" label to "ibm,trace-area" ? if so I can 
move this
fix to add_hb_reserve_node().

-Vasant