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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

Hi Julien

On 27 June 2014 16:24, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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

Implementation of kvm is slightly different, they are setting PSCI version from QEMU through ioctl Âand then using it to differentiate between the versions at runtime. Are you proposing the same? i.e. setting PSCI version externally using xen tools and then using it throughout ? .(right now i am using function id's to differentiate between the two, i can extend that by introducing some variable in vcpu struct for runtime detection)

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

I can :-), I will change it. sorry for the confusion, i thought you were asking why they have been moved...

ÂÂ

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

sure will take care in next patchsetÂ

  Â> +/* 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.

Ok, i will create two tables.Â

Regards,

--
Julien Grall

Thanks

parth

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