[Skiboot] [RFC PATCH] Introduce OPAL_GET_SYMBOL / OPAL_LOOKUP_SYMBOL

[Nicholas Piggin]

Oliver O'Halloran's on December 10, 2019 10:30 am:
> On Mon, Dec 9, 2019 at 5:43 PM Nicholas Piggin <npiggin at gmail.com> wrote:
>>
>> These calls can be used by Linux to annotate BUG addresses with symbols,
>> look up symbol addresses in xmon, etc.
>>
>> This is preferable over having Linux parse the OPAL symbol map itself,
>> because it could possibly be used to support other code regions, e.g.,
>> the wake-up code in the HOMER (where CPUs have been seen to get stuck).
>>
>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>> ---
>>  core/opal.c                             |  2 +
>>  core/utils.c                            | 92 +++++++++++++++++++++++--
>>  doc/opal-api/opal-get-symbol-181.rst    | 42 +++++++++++
>>  doc/opal-api/opal-lookup-symbol-182.rst | 35 ++++++++++
>>  include/opal-api.h                      |  4 +-
>>  5 files changed, 168 insertions(+), 7 deletions(-)
>>  create mode 100644 doc/opal-api/opal-get-symbol-181.rst
>>  create mode 100644 doc/opal-api/opal-lookup-symbol-182.rst
>>
>> diff --git a/core/opal.c b/core/opal.c
>> index d1e5bac86..dfeb497d6 100644
>> --- a/core/opal.c
>> +++ b/core/opal.c
>> @@ -142,6 +142,8 @@ int64_t opal_entry_check(struct stack_frame *eframe)
>>                 case OPAL_CEC_REBOOT:
>>                 case OPAL_CEC_REBOOT2:
>>                 case OPAL_SIGNAL_SYSTEM_RESET:
> 
>> +               case OPAL_GET_SYMBOL:
>> +               case OPAL_LOOKUP_SYMBOL:
> 
> I hate the names since I'd expect getting a symbol and looking up a
> symbol to be different names for the same thing. How about something
> like: OPAL_SYMBOL_TO_ADDR and OPAL_ADDR_TO_SYMBOL?

Sure.

>> +static unsigned long lookup_symbol(const char *name, unsigned long *size)
>> +{
>> +       size_t len = strlen(name);
>> +       unsigned long addr = 0;
>> +       char *sym;
>> +       char *p = __sym_map_start;
>> +
>> +       while (p < __sym_map_end) {
>> +               addr = strtoul(p, &p, 16) | SKIBOOT_BASE;
>> +               p += 3;
>> +               if (p >= __sym_map_end)
>> +                       return 0;
>> +
>> +               if (*(p + len) == '\n' && !strncmp(name, p, len)) {
>> +                       char *sym_end;
>> +
>> +                       if (get_symbol(addr, &sym, &sym_end, size) == 0) {
>> +                               assert(!strcmp(name, "_end"));
> 
> Looking up _end is a little weird, but I don't see why it needs to be an assert.

Yeah probably shouldn't be an assert. The point is more that if we find
it in the symbol map then get_symbol should be able to find it (unless
it's _end).

> 
>> +                               *size = 0;
>> +                       }
>> +
>> +                       /*
>> +                        * May be more than one symbol at this address but
>> +                        * symbol length calculation should still work in
>> +                        * that case.
>> +                        */
>> +
>> +                       return addr;
>> +               }
>> +
>> +               while(p < __sym_map_end && *p != '\n')
>> +                       p++;
>> +               p++;
>> +       }
>> +       return 0;
>> +}
> 
> Calling get_symbol() which re-scans the whole map is a little gross.
> Granted the alternative is more string parsing in C so it's fine I
> guess.

Yeah. Possibly one function could be made that does everything, but
I wanted to change as little as possible and reuse what's there. It's
obviously not a performance concern either way, being a sequential
scan.

> 
>> +static int64_t opal_get_symbol(uint64_t addr, __be64 *symaddr, __be64 *symsize, char *namebuf, uint64_t buflen)
>> +{
>> +       unsigned long saddr;
>> +       unsigned long ssize;
>> +       char *sym, *sym_end;
>> +       size_t l;
>> +
>> +       saddr = get_symbol(addr, &sym, &sym_end, &ssize);
>> +       if (!saddr)
>> +               return OPAL_RESOURCE;
> 
> Validating the pointers are non-null is probably a good idea.

Sure.

>> +  ``buflen``
>> +    Contains the length of the bufer that may be used.
> s/bufer/buffer/

Thakns.

> 
>> +
>> +
>> +Returns
>> +-------
>> +
>> +:ref:`OPAL_SUCCESS`
>> +  Found a symbol.
>> +:ref:`OPAL_RESOURCE`
>> +  Did not find a symbol.
> 
> OPAL_EMPTY might be more appropriate.
> 

Agree.

Thanks,
Nick