Re: [Xen-devel] [PATCH v1 01/13] Export hypervisor symbols

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.13 at 15:55, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 09/11/2013 03:51 AM, Jan Beulich wrote:
>>>>> On 10.09.13 at 17:20, Boris Ostrovsky<boris.ostrovsky@xxxxxxxxxx>  wrote:
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -102,11 +102,11 @@ $(BASEDIR)/common/symbols-dummy.o:
>>>   $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
>>>     $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>>>         $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
>>> -   $(NM) -n $(@D)/.$(@F).0 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0.S
>>> +   $(NM) -n $(@D)/.$(@F).0 | $(BASEDIR)/tools/symbols --all-symbols 
> >$(@D)/.$(@F).0.S
>> For one I can't see what use data symbols have for performance
>> analysis.
> 
> They are used by perf, similarly to kallsyms (usually when debug symbols 
> from binary are not available)

Right, but I specifically said _data_ symbols. I can see what code
ones are going to be used for.

>> And then I'm opposed to growing the symbol table size
>> unconditionally for no good reason.
> 
> I think I can remove --all-symbols, it is not strictly necessary for 
> what I plan now for
> perf. We may need to add it later, possibly with a config option.

Yes, please, as long as they're not really useful.

>>> --- a/xen/include/public/platform.h
>>> +++ b/xen/include/public/platform.h
>>> @@ -527,6 +527,26 @@ struct xenpf_core_parking {
>>>   typedef struct xenpf_core_parking xenpf_core_parking_t;
>>>   DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>>>   
>>> +#define XENPF_get_symbols  61
>>> +
>>> +#define XENSYMS_SZ         4096
>> This doesn't appear to belong into the public interface.
> 
> Linux driver needs to know size of the buffer that is passed from
> the hypervisir. I suppose I can just use PAGE_SIZE.

Buffer? Passed from the hypervisor?

And no, there's no PAGE_SIZE in the public interface as far as I'm
aware.

>>> +struct xenpf_symdata {
>>> +    /*
>>> +     * offset into Xen's symbol data and symbol number from
>>> +     * last call. Used only by Xen.
>>> +     */
>>> +    uint64_t xen_offset;
>>> +    uint64_t xen_symnum;
>> I wonder whether that's really a suitable mechanism.
> 
> Why do you think this is not suitable?
> 
> Linux needs to keep track of position in the symbol table while
> it is walking over the file, otherwise we will need to keep the state
> in hypervisor which is much less desirable.

This could be as simple as a "give me the n-th symbol" interface.
The handler in the hypervisor could cache the last symbol
together with the associated data (with the assumption that there's
only ever going to be one iteration in progress), invalidating the
cache if the coming in index isn't one greater than the last one
processed. All the caching of course is only necessary if otherwise
lookup times aren't acceptable.

>>> +
>>> +   /*
>>> +    * Symbols data, formatted similar to /proc/kallsyms:
>>> +    *   <address> <type> <name>
>>> +    */
>>> +    XEN_GUEST_HANDLE(char) buf;
>> This is too simplistic: Please use a proper structure here, to allow
>> switching the internal symbol table representation (which I have on
>> my todo list) without having to mimic old behavior.
> 
> I don't think I know what you are referring to here.

Rather than having a handle to a simply byte array, you ought
to have a handle to a structure containing address, type, and
(pointer to/handle of) name.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel