From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v16 01/10] x86: add generic resource (e.g. MSR) access hypercall Date: Thu, 25 Sep 2014 20:57:38 +0100 Message-ID: <542473B2.5040504@citrix.com> References: <1411640350-26155-1-git-send-email-chao.p.peng@linux.intel.com> <1411640350-26155-2-git-send-email-chao.p.peng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1411640350-26155-2-git-send-email-chao.p.peng@linux.intel.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: Chao Peng , xen-devel@lists.xen.org Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, George.Dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, JBeulich@suse.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On 25/09/14 11:19, Chao Peng wrote: > Add a generic resource access hypercall for tool stack or other > components, e.g., accessing MSR, port I/O, etc. > > Signed-off-by: Dongxiao Xu > Signed-off-by: Chao Peng > --- > xen/arch/x86/platform_hypercall.c | 90 ++++++++++++++++++++++++++++++ > xen/arch/x86/x86_64/platform_hypercall.c | 4 ++ > xen/include/public/platform.h | 23 ++++++++ > xen/include/xlat.lst | 2 + > 4 files changed, 119 insertions(+) > > diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c > index 2162811..081d9f5 100644 > --- a/xen/arch/x86/platform_hypercall.c > +++ b/xen/arch/x86/platform_hypercall.c > @@ -61,6 +61,68 @@ long cpu_down_helper(void *data); > long core_parking_helper(void *data); > uint32_t get_cur_idle_nums(void); > > +struct xen_resource_access { > + int32_t ret; > + uint32_t nr; > + XEN_GUEST_HANDLE(xenpf_resource_data_t) data; > +}; > + > +static bool_t allow_access_msr(unsigned int msr) > +{ > + return 0; > +} > + > +static void resource_access(void *info) > +{ > + struct xen_resource_access *ra = info; > + xenpf_resource_data_t data; > + int ret = 0; > + unsigned int i; > + > + for ( i = 0; i < ra->nr; i++ ) > + { > + if ( copy_from_guest_offset(&data, ra->data, i, 1) ) You cannot use copy_{to,from}_guest() here. You are almost certainly on the wrong cpu, and running on the wrong set of pagetables. At best, you will copy bogus data from the wrong process/domain, and likely corrupt it when copying back. > + { > + ret = -EFAULT; > + break; > + } > + > + if ( data.rsvd ) { > + ret = -EINVAL; > + break; > + } > + > + switch ( data.cmd ) > + { > + case XEN_RESOURCE_OP_MSR_READ: > + case XEN_RESOURCE_OP_MSR_WRITE: > + if ( data.idx >> 32 ) > + ret = -EINVAL; > + else if ( !allow_access_msr(data.idx) ) > + ret = -EACCES; > + else if ( data.cmd == XEN_RESOURCE_OP_MSR_READ ) > + ret = rdmsr_safe(data.idx, data.val); > + else > + ret = wrmsr_safe(data.idx, data.val); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + if ( ret ) > + break; > + > + if ( copy_to_guest_offset(ra->data, i, &data, 1) ) > + { > + ret = -EFAULT; > + break; > + } > + } > + > + ra->ret = ret; > +} > + > ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op) > { > ret_t ret = 0; > @@ -601,6 +663,34 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op) > } > break; > > + case XENPF_resource_op: > + { > + struct xen_resource_access ra; > + struct xenpf_resource_op *rsc_op = &op->u.resource_op; > + unsigned int cpu = smp_processor_id(); > + > + ra.nr = rsc_op->nr; > + ra.data = rsc_op->data; You must do all copy_{from,to}_user() here, and strictly only pass Xen pointers to resource_access(). This means you will need to xmalloc() yourself some space for the xenpf_resource_data_t array. On a different note, you need to enforce a maximum resource_op.nr of something rather low to (16/32 perhaps?) to prevent a toolstack asking for 0xffffffff non-preemptible operations. ~Andrew > + > + if ( rsc_op->cpu == cpu ) > + resource_access(&ra); > + else if ( cpu_online(rsc_op->cpu) ) > + on_selected_cpus(cpumask_of(rsc_op->cpu), > + resource_access, &ra, 1); > + else > + { > + ret = -ENODEV; > + break; > + } > + > + if ( ra.ret ) > + { > + ret = ra.ret; > + break; > + } > + } > + break; > + > default: > ret = -ENOSYS; > break; > diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c > index b6f380e..4db6622 100644 > --- a/xen/arch/x86/x86_64/platform_hypercall.c > +++ b/xen/arch/x86/x86_64/platform_hypercall.c > @@ -32,6 +32,10 @@ CHECK_pf_pcpu_version; > CHECK_pf_enter_acpi_sleep; > #undef xen_pf_enter_acpi_sleep > > +#define xen_pf_resource_data xenpf_resource_data > +CHECK_pf_resource_data; > +#undef xen_pf_resource_data > + > #define COMPAT > #define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t) > #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t) > diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h > index 053b9fa..e4d9091 100644 > --- a/xen/include/public/platform.h > +++ b/xen/include/public/platform.h > @@ -527,6 +527,28 @@ struct xenpf_core_parking { > typedef struct xenpf_core_parking xenpf_core_parking_t; > DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t); > > +#define XENPF_resource_op 61 > + > +#define XEN_RESOURCE_OP_MSR_READ 0 > +#define XEN_RESOURCE_OP_MSR_WRITE 1 > + > +struct xenpf_resource_data { > + uint32_t cmd; /* XEN_RESOURCE_OP_* */ > + uint32_t rsvd; > + uint64_t idx; > + uint64_t val; > +}; > +typedef struct xenpf_resource_data xenpf_resource_data_t; > +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_data_t); > + > +struct xenpf_resource_op { > + uint32_t nr; /* number of data entry */ > + uint32_t cpu; /* which cpu to run */ > + XEN_GUEST_HANDLE(xenpf_resource_data_t) data; > +}; > +typedef struct xenpf_resource_op xenpf_resource_op_t; > +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_op_t); > + > /* > * ` enum neg_errnoval > * ` HYPERVISOR_platform_op(const struct xen_platform_op*); > @@ -553,6 +575,7 @@ struct xen_platform_op { > struct xenpf_cpu_hotadd cpu_add; > struct xenpf_mem_hotadd mem_add; > struct xenpf_core_parking core_parking; > + struct xenpf_resource_op resource_op; > uint8_t pad[128]; > } u; > }; > diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst > index 9a35dd7..100fcf5 100644 > --- a/xen/include/xlat.lst > +++ b/xen/include/xlat.lst > @@ -88,6 +88,8 @@ > ? xenpf_enter_acpi_sleep platform.h > ? xenpf_pcpuinfo platform.h > ? xenpf_pcpu_version platform.h > +? xenpf_resource_op platform.h > +? xenpf_resource_data platform.h > ! sched_poll sched.h > ? sched_remote_shutdown sched.h > ? sched_shutdown sched.h