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

[Oliver]

On Tue, Jun 26, 2018 at 9:56 PM, Vasant Hegde
<hegdevasant at linux.vnet.ibm.com> wrote:
> 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().

Currently nothing on the host uses the trace-area stuff so renaming it
is fine. A trace reservation only exists in HDAT when someone
configures cronus or the FSP (via attribute overrides) to reserve some
system memory for use as a nHTM trace buffer. Nothing on the host
looks at that reservation so we should be fine.

>
> -Vasant
>