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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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)

> 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.

> > --- 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.

> > +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.

> > +
> > +   /*
> > +    * 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.

-boris


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