From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH xen v2] xen: arm: fully implement multicall interface. Date: Mon, 7 Apr 2014 16:24:35 +0100 Message-ID: <5342C333.9020409@eu.citrix.com> References: <1396874925-751-1-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1396874925-751-1-git-send-email-ian.campbell@citrix.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: Ian Campbell , xen-devel@lists.xen.org Cc: keir@xen.org, julien.grall@linaro.org, tim@xen.org, jbeulich@suse.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 04/07/2014 01:48 PM, Ian Campbell wrote: > I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm: > implement do_multicall_call for both 32 and 64-bit" but it is obviously > insufficient since it doesn't actually wire up the hypercall. > > Before doing so we need to make the usual adjustments for ARM and turn the > unsigned longs into xen_ulong_t. There is no difference in the resulting > structure for x86. > > There are knock on changes to the trace interface, but again they are nops on > x86. > > For 32-bit ARM guests we require that the arguments which they pass to a > hypercall via a multicall do not use the upper bits of xen_ulong_t and kill > them if they violate this. This should ensure that no ABI surprises can be > silently lurking when running on a 32-bit hypervisor waiting to pounce when the > same kernel is run on a 64-bit hypervisor. Killing the guest is harsh but it > will be far easier to relax the restriction if it turns out to cause problems > than to tighten it up if we were lax to begin with. > > In the interests of clarity and always using explicitly sized types change the > unsigned int in the hypercall arguments to a uint32_t. There is no actual > change here on any platform. > > We should consider backporting this to 4.4.1 in case a guest decides they want > to use a multicall in common code e.g. I suggested such a thing while > reviewing a netback change recently. > > Signed-off-by: Ian Campbell > Cc: keir@xen.org > Cc: jbeulich@suse.com > Cc: George Dunlap Trace-related bits: Acked-by: George Dunlap > --- > This depends on "xen: make sure that likely and unlikely convert the expression > to a boolean" > > v2: - update compat version of __trace_multicall_call too > - update xen.h on requirements when sizeof(xen_ulong_t) > sizeof(a register) > - kill 32-bit guests which do not follow those requirements. After the > conversation on v1 I decided that starting out harsh and relaxing if it > becomes a problem was easier than discovering a mistake later. > --- > xen/arch/arm/traps.c | 23 +++++++++++++++++++++-- > xen/common/compat/multicall.c | 2 +- > xen/common/multicall.c | 4 ++-- > xen/common/trace.c | 2 +- > xen/include/public/xen.h | 10 ++++++---- > xen/include/xen/trace.h | 2 +- > 6 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index a7edc4e..062c4b5 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1012,6 +1012,7 @@ static arm_hypercall_t arm_hypercall_table[] = { > HYPERCALL(sysctl, 2), > HYPERCALL(hvm_op, 2), > HYPERCALL(grant_table_op, 3), > + HYPERCALL(multicall, 2), > HYPERCALL_ARM(vcpu_op, 3), > }; > > @@ -1159,6 +1160,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > #endif > } > > +static void check_multicall_32bit_clean(struct multicall_entry *multi) > +{ > + int i; > + > + for ( i = 0; i < arm_hypercall_table[multi->op].nr_args; i++ ) > + { > + if ( unlikely(multi->args[i] & 0xffffffff00000000ULL) ) > + { > + printk("%pv: multicall argument %d is not 32-bit clean %"PRIx64"\n", > + current, i, multi->args[i]); > + domain_crash_synchronous(); > + } > + } > +} > + > void do_multicall_call(struct multicall_entry *multi) > { > arm_hypercall_fn_t call = NULL; > @@ -1176,9 +1192,12 @@ void do_multicall_call(struct multicall_entry *multi) > return; > } > > + if ( is_32bit_domain(current->domain) ) > + check_multicall_32bit_clean(multi); > + > multi->result = call(multi->args[0], multi->args[1], > - multi->args[2], multi->args[3], > - multi->args[4]); > + multi->args[2], multi->args[3], > + multi->args[4]); > } > > /* > diff --git a/xen/common/compat/multicall.c b/xen/common/compat/multicall.c > index 95c047a..2af8aef 100644 > --- a/xen/common/compat/multicall.c > +++ b/xen/common/compat/multicall.c > @@ -29,7 +29,7 @@ DEFINE_XEN_GUEST_HANDLE(multicall_entry_compat_t); > > static void __trace_multicall_call(multicall_entry_t *call) > { > - unsigned long args[6]; > + xen_ulong_t args[6]; > int i; > > for ( i = 0; i < ARRAY_SIZE(args); i++ ) > diff --git a/xen/common/multicall.c b/xen/common/multicall.c > index e66c798..fa9d910 100644 > --- a/xen/common/multicall.c > +++ b/xen/common/multicall.c > @@ -35,10 +35,10 @@ static void trace_multicall_call(multicall_entry_t *call) > > ret_t > do_multicall( > - XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned int nr_calls) > + XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls) > { > struct mc_state *mcs = ¤t->mc_state; > - unsigned int i; > + uint32_t i; > int rc = 0; > > if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) ) > diff --git a/xen/common/trace.c b/xen/common/trace.c > index 1814165..f651cf3 100644 > --- a/xen/common/trace.c > +++ b/xen/common/trace.c > @@ -817,7 +817,7 @@ unlock: > } > > void __trace_hypercall(uint32_t event, unsigned long op, > - const unsigned long *args) > + const xen_ulong_t *args) > { > struct __packed { > uint32_t op; > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > index 8c5697e..a6a2092 100644 > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -541,13 +541,15 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t); > /* > * ` enum neg_errnoval > * ` HYPERVISOR_multicall(multicall_entry_t call_list[], > - * ` unsigned int nr_calls); > + * ` uint32_t nr_calls); > * > - * NB. The fields are natural register size for this architecture. > + * NB. The fields are logically the natural register size for this > + * architecture. In cases where xen_ulong_t is larger than this then > + * any unused bits in the upper portion must be zero. > */ > struct multicall_entry { > - unsigned long op, result; > - unsigned long args[6]; > + xen_ulong_t op, result; > + xen_ulong_t args[6]; > }; > typedef struct multicall_entry multicall_entry_t; > DEFINE_XEN_GUEST_HANDLE(multicall_entry_t); > diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h > index 3b8a7b3..12966ea 100644 > --- a/xen/include/xen/trace.h > +++ b/xen/include/xen/trace.h > @@ -45,7 +45,7 @@ static inline void trace_var(u32 event, int cycles, int extra, > } > > void __trace_hypercall(uint32_t event, unsigned long op, > - const unsigned long *args); > + const xen_ulong_t *args); > > /* Convenience macros for calling the trace function. */ > #define TRACE_0D(_e) \