Re: [Xen-devel] [PATCH v7 01/19] common/symbols: Export hypervisor symbols to privileged guest

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/06/2014 01:57 PM, Andrew Cooper wrote:
> On 06/06/14 18:39, Boris Ostrovsky wrote:
> > Export Xen's symbols as {<address><type><name>} triplet via new XENPF_get_symbol
> > hypercall
> >
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > Reviewed-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> > Tested-by: Dietmar Hahn <dietmar.hahn@xxxxxxxxxxxxxx>
> > ---
> >   xen/arch/x86/platform_hypercall.c | 21 +++++++++++++++
> >   xen/common/symbols.c              | 54 +++++++++++++++++++++++++++++++++++++++
> >   xen/include/public/platform.h     | 19 ++++++++++++++
> >   xen/include/xen/symbols.h         |  3 +++
> >   xen/include/xlat.lst              |  1 +
> >   5 files changed, 98 insertions(+)
> >
> > diff --git a/xen/arch/x86/platform_hypercall.c 
> > b/xen/arch/x86/platform_hypercall.c
> > index 2162811..2c602e7 100644
> > --- a/xen/arch/x86/platform_hypercall.c
> > +++ b/xen/arch/x86/platform_hypercall.c
> > @@ -23,6 +23,7 @@
> >   #include <xen/cpu.h>
> >   #include <xen/pmstat.h>
> >   #include <xen/irq.h>
> > +#include <xen/symbols.h>
> >   #include <asm/current.h>
> >   #include <public/platform.h>
> >   #include <acpi/cpufreq/processor_perf.h>
> > @@ -601,6 +602,26 @@ ret_t 
> > do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> >       }
> >       break;
> >   
> > +    case XENPF_get_symbol:
> > +    {
> > +        static char name[KSYM_NAME_LEN + 1];
> There needs to be some concurrency protection surrounding the use of
> this static.  The symbol mutext in xensyms_read() is not sufficient.
>
> As it currently stands, two vcpus issuing this hypercall at the time
> will collide and both will get junk back.

We should be protected by xenpf_lock here.


> > diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> > index 053b9fa..afe301c 100644
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -527,6 +527,24 @@ struct xenpf_core_parking {
> >   typedef struct xenpf_core_parking xenpf_core_parking_t;
> >   DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
> >   
> > +#define XENPF_get_symbol   61
> > +struct xenpf_symdata {
> > +    /* IN variables */
> > +    uint32_t namelen; /* size of 'name' buffer */
> > +
> > +    /* IN/OUT variables */
> > +    uint32_t symnum;  /* IN:  Symbol to read                            */
> > +                      /* OUT: Next available symbol. If same as IN then */
> > +                      /*      we reached the end                        */
> > +
> > +    /* OUT variables */
> > +    char type;
> > +    uint64_t address;
> > +    XEN_GUEST_HANDLE(char) name;
> Please can we avoid introducing non-explicitly-width'd types into the
> public ABI.
>
> If you swap name and type, use XEN_GUEST_HANDLE_64() and promote type to
> a uint32, it should be 32/64 bit safe and not need translating.
I thought we are going via compat_platform_op for 32-bit guests and thus 
pass pack(4)'d structures so translating is not needed. Is that not true?
(I did test this on 32-bit dom0 and it worked fine, although that alone 
is probably not a good argument).
-boris

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