Re: [PATCH v2] xen/arm : emulation of arm's PSCI v0.2 standard

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

On 27/06/14 06:18, Parth Dixit wrote:
> On 26 June 2014 21:31, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
>      > +    return do_psci_cpu_off(power_state);
>      > +}
>      > +
>      > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t
>     entry_point,
>      > +                       register_t context_id)
>
>
>     This function is 95% the same code. I would merge as much as possible
>     with do_psci_cpu_on to avoid code duplication.
>
> Ok, i will try to merge the functions together (AI Parth)

FWIW, KVM is using the same PSCI on function on V0.1 and V0.2. I'm 
wondering if we can assume the same thing here. Of course, you will have 
to add the few improvement in the function (checking if the VCPU is 
already online...).

>      > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
>      > index 189964b..3487380 100644
>      > --- a/xen/include/asm-arm/psci.h
>      > +++ b/xen/include/asm-arm/psci.h
>      > @@ -1,11 +1,6 @@
>      >  #ifndef __ASM_PSCI_H__
>      >  #define __ASM_PSCI_H__
>      >
>      > -#define PSCI_SUCCESS  0
>      > -#define PSCI_ENOSYS  -1
>      > -#define PSCI_EINVAL  -2
>      > -#define PSCI_DENIED  -3
>      > -
>
>     Why did you just moved them at the end of the file and change the
>     indentation?
>
> This was done to have common return functions between PSCI v0.1 and v
> 0.2 and to define them at one place

Ok... I still don't understand why you can't add the new return value here.

BTW, IIRC there was a copyright int the PSCI header from Linux. I think 
you have to retain it here.

>      > +/* PSCI version */
>      > +#define XEN_PSCI_V_0_1 1
>
>     This doesn't seem to be used, right?
>
> Yes it is defined for consistency and also for future use in case we
> return v0.1, i'd prefer to keep it this way what do you think?

Ok

>     [..]
>
>      > diff --git a/xen/include/public/arch-arm.h
>     b/xen/include/public/arch-arm.h
>      > index 7496556..93803e4 100644
>      > --- a/xen/include/public/arch-arm.h
>      > +++ b/xen/include/public/arch-arm.h
>      > @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t;
>      >  #define PSCI_cpu_off     1
>      >  #define PSCI_cpu_on      2
>      >  #define PSCI_migrate     3
>      > +#define PSCI_0_1_MAX     4
>
>     Why do you expose PSCI_0_1_MAX
>
> You are right i think i can get away by treating PSCI_migrate as max
> value (AI Parth)

Can't you define 2 table in traps.c: one for PSCI v0.1 and one for PSCI 
v0.2? This would avoid stupid issue later when we will implement a new 
function for 0.1.

Regards,

-- 
Julien Grall