Re: [Xen-devel] [PATCH v3 01/13] xen/arm: basic PSCI support, implement cpu_on and cpu_off

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 25 Apr 2013, Ian Campbell wrote:
> On Wed, 2013-04-24 at 20:07 +0100, Stefano Stabellini wrote:
> > Implement support for ARM Power State Coordination Interface, PSCI in
> > short.
> > Support HVC and SMC calls.
> > 
> > Changes in v3:
> > - move do_psci_* to psci.c;
> > - trap SMC;
> > - return PSCI error codes;
> > - remove Linux boot procotol from secondary cpus.
> > 
> > Changes in v2:
> > - set is_initialised in arch_set_info_guest;
> > - zero vcpu_guest_context before using it.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/arm/Makefile           |    1 +
> >  xen/arch/arm/domain.c           |    2 +
> >  xen/arch/arm/psci.c             |   87 
> > +++++++++++++++++++++++++++++++++++++++
> >  xen/arch/arm/traps.c            |   47 ++++++++++++++++++++-
> >  xen/include/asm-arm/processor.h |    1 +
> >  xen/include/asm-arm/psci.h      |   24 +++++++++++
> >  6 files changed, 161 insertions(+), 1 deletions(-)
> >  create mode 100644 xen/arch/arm/psci.c
> >  create mode 100644 xen/include/asm-arm/psci.h
> > 
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 2106a4f..8f75044 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -5,6 +5,7 @@ subdir-y += platforms
> >  obj-y += early_printk.o
> >  obj-y += cpu.o
> >  obj-y += domain.o
> > +obj-y += psci.o
> >  obj-y += domctl.o
> >  obj-y += sysctl.o
> >  obj-y += domain_build.o
> >  
> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > new file mode 100644
> > index 0000000..28153ad
> > --- /dev/null
> > +++ b/xen/arch/arm/psci.c
> > @@ -0,0 +1,87 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <xen/errno.h>
> > +#include <xen/sched.h>
> > +#include <xen/types.h>
> > +
> > +#include <asm/current.h>
> > +#include <asm/psci.h>
> > +
> > +int do_psci_cpu_on(uint32_t vcpuid, uint32_t entry_point)
> > +{
> > +    struct vcpu *v;
> > +    struct domain *d = current->domain;
> > +    struct vcpu_guest_context *ctxt;
> > +    int rc;
> > +
> > +    if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
> > +        return PSCI_EINVAL;
> > +
> > +    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> > +        return PSCI_EINVAL;
> > +
> > +    if ( !v->is_initialised ) {
> 
> If this is true then we eventually return success, should this be
> returning EINVAL or DENIED or something?

Nope: this is done on purpose so that you can call cpu_on on a cpu that
called cpu_off before.


> > +int do_psci_migrate(uint32_t vcpuid)
> > +{
> > +    return PSCI_ENOSYS;
> > +}
> 
> Is this required or should we just not expose it in device tree?

We should not expose it in device tree, but we might as well return a
proper error here.


> > +        if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> > +            return PSCI_DENIED;
> > +
> > +        memset(ctxt, 0, sizeof(*ctxt));
> > +        ctxt->user_regs.pc32 = entry_point;
> > +        ctxt->sctlr = SCTLR_BASE;
> > +        ctxt->ttbr0 = 0;
> > +        ctxt->ttbr1 = 0;
> > +        ctxt->ttbcr = 0; /* Defined Reset Value */
> > +        ctxt->user_regs.cpsr = 
> > PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC;
> 
> This value is repeated in a couple of places now, perhaps we should have
> a ctxt_init which sets up our vcpus defined "reset" values all in one
> place?

The only repeated value I could find is
"PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC", also used in
construct_dom0.
I would just introduce a constant called "CPSR_RESET_VALUE" and use it
everywhere.


> > +        ctxt->flags = VGCF_online;
> > +
> > +        domain_lock(d);
> > +        rc = arch_set_info_guest(v, ctxt);
> > +        free_vcpu_guest_context(ctxt);
> > +
> > +        if ( rc < 0 )
> > +        {
> > +            domain_unlock(d);
> > +            return PSCI_DENIED;
> > +        }
> > +        domain_unlock(d);
> > +    }
> > +
> > +    clear_bit(_VPF_down, &v->pause_flags);
> 
> ctxt->flags = VGCF_online would cause this to happen, I'd rather do that
> than repeat the logic here.

It is repeated because we might get here without getting inside the if
statement when the vcpu is already initialized.


> > @@ -647,6 +673,20 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> > unsigned int code)
> >      }
> >  }
> >  
> > +static void do_trap_psci(struct cpu_user_regs *regs)
> > +{
> > +    arm_psci_fn_t psci_call = NULL;
> > +
> > +    if ( regs->r0 >= ARRAY_SIZE(arm_psci_table) )
> > +    {
> > +        regs->r0 = -ENOSYS;
> > +        return;
> > +    }
> > +
> > +    psci_call = arm_psci_table[regs->r0].fn;
> 
> Might be NULL?

it can't be NULL if r0 < ARRAY_SIZE(arm_psci_table)


> Since Xen passes the valid PSCI numbers to the guest I think it should
> be illegal to call anything else, so rather than ENOSYS calling an
> unimplemented slots should result in domain_crash_synchronous. It seems
> like the general plan is for various firmware interfaces to use iss==0
> and to communicate the valid r0 numbers via device tree, but I don't
> think we can assume that all those different interfaces will have the
> same ENOSYS like value.

The (not final) document is not super clear about this but my
understanding is that they all have the same "not implemented" error
code.
Besides the mere fact that an ENOSYS-like error code is defined makes me
inclined to return an error rather than crashing the domain.
After all if the guest calls an vcpu_op hypercall that we don't
implement, we return error, we don't crash the domain for it.


> > +    regs->r0 = psci_call(regs->r1, regs->r2);
> > +}
> > +
> >  static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long 
> > iss)
> >  {
> >      arm_hypercall_fn_t call = NULL;
> > @@ -959,8 +999,13 @@ asmlinkage void do_trap_hypervisor(struct 
> > cpu_user_regs *regs)
> >      case HSR_EC_HVC:
> >          if ( (hsr.iss & 0xff00) == 0xff00 )
> >              return do_debug_trap(regs, hsr.iss & 0x00ff);
> > +        if ( hsr.iss == 0 )
> > +            return do_trap_psci(regs);
> >          do_trap_hypercall(regs, hsr.iss);
> 
> This is a pre-existing issue but this handles all no-zero iss as a Xen
> hypercall, I think we should
>       switch ( hsr.iss ) 
>       case XEN_PSCI_TAG: ...
>       case XEN_HYPERCALL_TAG: ...
>       default: domain_crash_synchrous
> and pull the domain crash out of do_trap_hypercall.
> 
> Maybe XEN_PSCI_TAG isn't the right name if other interfaces are going to
> use it, but it's only an internal name anyway.

OK

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