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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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


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