Re: [Xen-devel] [PATCH] setup vwfi correctly on cpu0

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 03/30/2017 11:35 PM, Stefano Stabellini wrote:
> parse_vwfi runs after init_traps on cpu0, potentially resulting in the
> wrong HCR_EL2 for it. Secondary cpus boot after parse_vwfi, so in their
> case init_traps will write the correct set of flags to HCR_EL2.
>
> For cpu0, fix the issue by changing HCR_EL2 setting directly in
> parse_vwfi.
>
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> ---
> This patch should be apply to 4.8, 4.7, 4.6, not to unstable (it will be
> fixed differently there).
> ---
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 614501f..94d2e8a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -112,6 +112,16 @@ static void __init parse_vwfi(const char *s)
>                 vwfi = NATIVE;
>         else
>                 vwfi = TRAP;
> +    /*
> +     * HCR_EL2 has already been set on cpu0, change the setting here, if
> +     * needed. Other cpus haven't booted yet, init_traps will setup
> +     * HCR_EL2 correctly.
> +     */
> +    if (vwfi == NATIVE) {
Coding style:

if ( ... )
{

> +        register_t hcr;
> +        hcr = READ_SYSREG(HCR_EL2);
> +        WRITE_SYSREG(hcr & ~(HCR_TWI|HCR_TWE), HCR_EL2);
You are assuming the default value of vwfi and it makes very complicate 
for someone to follow the code and modify it.
IHMO, a parsing function should only parse the command line and setup 
variable. Hardware configuration or initialization should be done 
separately (such as in an init call).
Cheers,

--
Julien Grall

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