[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Hi Parth,

On 27/06/14 06:18, Parth Dixit wrote:
On 26 June 2014 21:31, Julien Grall <julien.grall@xxxxxxxxxx
<mailto:julien.grall@xxxxxxxxxx>> wrote:
     > +    return do_psci_cpu_off(power_state);
     > +}
     > +
     > +int do_psci_0_2_cpu_on(register_t target_cpu, register_t
     > +                       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

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?



     > diff --git a/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.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.