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

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

On 06/06/14 20:05, Boris Ostrovsky wrote:
> 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.

So we are.  Perhaps worth leaving a small comment indicating as such?

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

Erm, pass.  Each time I try to get my head around the compat interface,
a new supprise pops up.  platform_op appears to be different to other
hypercall handlers I am more used to.

If it works, then probably best not to play with it.

~Andrew