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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

Hi Julien,

Comments are inline, i will try to send a new patch today/weekend after testing.

On 26 June 2014 21:31, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

Hi Parth,

Thank you for the patch.

On 06/26/2014 06:26 AM, Parth Dixit wrote:
> +void vcpu_block_event(struct vcpu *v)
> +{
> + Â Âvcpu_block();
> + Â Â/* The ARM spec declares that even if local irqs are masked in
> + Â Â* the CPSR register, an irq should wake up a cpu from WFI anyway.
> + Â Â* For this reason we need to check for irqs that need delivery,
> + Â Â* ignoring the CPSR register, *after* calling SCHEDOP_block to
> + Â Â* avoid races with vgic_vcpu_inject_irq.
> + Â Â*/

The comment is out of date here and you should move it on top of the
function.

Also, you've introduce this function but didn't replace the code in WFI
trap.

Furthermore, I would move this change in a separate patch. It will be
more clear.

ÂI will create new patch for this and replace WFI with this function Â-(AI ÂParth)

> + Â Âif ( local_events_need_delivery_nomask() )
> + Â Â Â Âvcpu_unblock(current);
> +}
> +

[..]

> +int do_psci_0_2_cpu_off(void)
> +{
> + Â Âuint32_t power_state = 0 ;

Coding style require a newline between the declarations.

Also, defining power_state looks pointless here.

Agreed will be taken care of in next patchset - (AI Parth)Â

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

[..]

> +
> +int do_psci_0_2_migrate(uint32_t target_cpu)
> +{
> + Â Âreturn PSCI_ENOSYS;
> +}
> +
> +

Only one newline.

> +int do_psci_0_2_migrate_info_type(void)
> +{
> + Â Âreturn PSCI_0_2_TOS_MP;
> +}
> +
> +

Same here.

> +register_t do_psci_0_2_migrate_info_up_cpu(void)
> +{
> + Â Âreturn PSCI_ENOSYS;
> +}
> +
> +

Same here.

> +void do_psci_0_2_system_off( void )
> +{
> + Â Âstruct domain *d = current->domain;
> + Â Âdomain_shutdown(d,SHUTDOWN_poweroff);
> +}
> +
> +

Same here.

Will take care in next patch set (AI Parth)Â

> +void do_psci_0_2_system_reset(void)
> +{
> + Â Âstruct domain *d = current->domain;
> + Â Âdomain_shutdown(d,SHUTDOWN_reboot);
> +}
> +

[..]

> Âstatic inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
> Â{
> @@ -55,6 +56,7 @@ static inline int arch_virq_is_global(int virq)
> Â Â Âreturn 1;
> Â}
>
> +

Spurious change.

Will take care in next patch set (AI Parth)Â

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

> +/* 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?

> +
> +/* PSCI v0.2 interface */
> +#define PSCI_0_2_FN_BASE Â Â Â Â Â Â Â Â Â Â 0x84000000
> +#define PSCI_0_2_FN(n) Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (PSCI_0_2_FN_BASE + (n))
> +#define PSCI_0_2_64BIT Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 0x40000000

In general we don't use hard-tab in Xen coding style. Can you remove them?

Will take care in next patchset (AI Parth)Â

[..]

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

Â

--
Julien Grall

Thanks

parth

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