From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v7 01/19] common/symbols: Export hypervisor symbols to privileged guest Date: Fri, 6 Jun 2014 20:13:39 +0100 Message-ID: <539212E3.7060800@citrix.com> References: <1402076415-26475-1-git-send-email-boris.ostrovsky@oracle.com> <1402076415-26475-2-git-send-email-boris.ostrovsky@oracle.com> <53920101.8090304@citrix.com> <539210EC.9050000@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <539210EC.9050000@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky Cc: kevin.tian@intel.com, keir@xen.org, JBeulich@suse.com, tim@xen.org, dietmar.hahn@ts.fujitsu.com, xen-devel@lists.xen.org, jun.nakajima@intel.com, suravee.suthikulpanit@amd.com List-Id: xen-devel@lists.xenproject.org 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 {
} triplet via new >>> XENPF_get_symbol >>> hypercall >>> >>> Signed-off-by: Boris Ostrovsky >>> Reviewed-by: Dietmar Hahn >>> Tested-by: Dietmar Hahn >>> --- >>> 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 >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -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