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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@oracle.com>
> Reviewed-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> Tested-by: Dietmar Hahn <dietmar.hahn@ts.fujitsu.com>
> ---
>  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.

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

~Andrew