Re: [Xen-devel] [PATCH 1/4] xen/arm: basic PSCI support, implement cpu_on

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-03-21 at 18:42 +0000, Stefano Stabellini wrote:
> Implement support for ARM Power State Coordination Interface, PSCI in
> short. The current implementation is based on HVC and only supports the
> cpu_on call.

Doesn't the PSCI interface require the use of SMC not HVC?
I thought I heard Charles say at connect that there was now a PSCI v2,
and I suspect I'm looking at the v1 document (which mentions HVC only in
passing). Which interface did you implement?

Anyhow, we can trap SMCs to the hypervisor by setting the right control
register bits. We should do this anyway -- no good can come of a guest
making a call direct to the monitor!

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: will.deacon@xxxxxxx
> CC: marc.zyngier@xxxxxxx
> ---
>  xen/arch/arm/domain.c        |   66 
> ++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c         |   41 ++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h |    4 ++
>  3 files changed, 111 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 2506369..6542233 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c

Perhaps this stuff and the obvious stuff from traps.c should be in a new
psci.c instead?

> @@ -584,6 +584,72 @@ void arch_dump_vcpu_info(struct vcpu *v)
>  {
>  }
>  
> +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 -EINVAL;
> +
> +    if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> +        return -ENOENT;
> +
> +    if ( v->is_initialised )
> +        return -EEXIST;
> +    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> +        return -ENOMEM;

Are the PSCI error codes really the same as the Xen errno values?

> +
> +    ctxt->user_regs.pc = entry_point;
> +    /* Linux boot protocol. See linux.Documentation/arm/Booting. */
> +    ctxt->user_regs.r0 = 0; /* SBZ */
> +    /* Machine ID: We use DTB therefore no machine id */
> +    ctxt->user_regs.r1 = 0xffffffff;
> +    /* ATAGS/DTB: We currently require that the guest kernel to be
> +     * using CONFIG_ARM_APPENDED_DTB. Ensure that r2 does not look
> +     * like a valid pointer to a set of ATAGS or a DTB.
> +     */
> +    ctxt->user_regs.r2 = 0xffffffff;

Is this protocol relevant to secondary CPUs? I'd have expected not and
that r[0-15] are all undefined on startup?

> +        if ( !hsr.iss )
> +            return do_trap_psci(regs);

Ugh, using ISS==0 for this interface is a bit unfortunate, who
arbitrates this namespace?

(nit: you are logically testing for 0 not False, so I think == 0 would
be nicer than !, or even a switch statement might be good)

>          do_trap_hypercall(regs, hsr.iss);
>          break;
>      case HSR_EC_DATA_ABORT_GUEST:



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