Re: [Xen-devel] [PATCH xen v2] xen: arm: fully implement multicall interface.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-04-07 at 14:27 +0100, Jan Beulich wrote:
> >>> On 07.04.14 at 14:48, <ian.campbell@xxxxxxxxxx> 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 <ian.campbell@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> on the non-ARM parts, with one comment on the ARM ones below.
> 
> > @@ -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();
> 
> On x86 we actually decided quite a long while ago to try to avoid
> domain_crash_synchronous() whenever possible. Would
> domain_crash(current->domain) not work for you here?

I wondered about that and concluded that I didn't really grok the
difference so I went with what I thought was the safer option. I'd be
happy to change if that's considered the right thing to do.

My concern was that domain_crash returns and I wasn't sure what I was
then supposed to do (obviously not actually run the call though) or when
the domain was actually guaranteed to die. In particular I might get
more calls to multicall_call for the rest of the batch. I'm not sure
that matters other than the possibility that skipping one call in the
middle might lead to confusing log spam from the remaining calls.

> Also I think you want the loop variable to be uint32_t, in line with
> your changes elsewhere in this patch.

True.

> 
> Jan
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel