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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-04-25 at 11:57 +0100, Stefano Stabellini wrote:
> 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.

But in that case you ignore the newly supplied entry point? Is that
really the semantics of PSCI?

Needs a comment I think.

> > > +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 it isn't in device tree then the kernel doesn't even know which r0
value this function is.

> 
> 
> > > +        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.

Ack.

> 
> 
> > > +        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.

I think this ties into the lack of resetting the entry point I mentioned
above. Perhaps this should be in an } else then?
> 
> 
> > > @@ -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)

Not right now since you happen to have packed them tightly, but in the
future?

> 
> > 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.

My understanding is that some of these interfaces are rather ad-hoc
vendor supplied interfaces, in which case I think they are unlikely to
have any sort of consistent error return.

> Besides the mere fact that an ENOSYS-like error code is defined makes me
> inclined to return an error rather than crashing the domain.

It's defined for PSCI, but there is the potential for other firmware
interfaces using smc/hvc #0 and their own distinct r0 values.

> 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.

We do (or should!) crash the domain if it calls hvc with a non-Xen tag,
which is more analogous to the situation here.

When the guest uses the XEN tag then the return values are well defined,
so we can, and should, return a Xen error code in those cases.

> 
> 
> > > +    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