[PATCH] setup vwfi correctly on cpu0

[PATCH] setup vwfi correctly on cpu0

[Stefano Stabellini]

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@kernel.org>

---
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) {
+        register_t hcr;
+        hcr = READ_SYSREG(HCR_EL2);
+        WRITE_SYSREG(hcr & ~(HCR_TWI|HCR_TWE), HCR_EL2);
+    }
 }
 custom_param("vwfi", parse_vwfi);
 

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

Re: [PATCH] setup vwfi correctly on cpu0

[Julien Grall]

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@kernel.org>
>
> ---
> 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@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] setup vwfi correctly on cpu0

[Stefano Stabellini]

On Fri, 31 Mar 2017, Julien Grall wrote:
> 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@kernel.org>
> > 
> > ---
> > 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 ( ... )
> {

OK


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

Do you mean the default vwfi setting? If so, no, I am not: hcr &
~(HCR_TWI|HCR_TWE) works regardless of the default.


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

That is true. Given that this is meant for backports I was rewarding
shortness over cleanliness. I'll introduce a presmp_initcall, should
still be short enough.

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

Re: [PATCH] setup vwfi correctly on cpu0

[Julien Grall]

Hi Stefano,

On 31/03/17 23:33, Stefano Stabellini wrote:
> On Fri, 31 Mar 2017, Julien Grall wrote:
>> 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@kernel.org>
>>>
>>> ---
>>> 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 ( ... )
>> {
>
> OK
>
>
>>> +        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.
>
> Do you mean the default vwfi setting? If so, no, I am not: hcr &
> ~(HCR_TWI|HCR_TWE) works regardless of the default.

Sorry, I was not clear. You assume that vwfi == TRAP by default, 
although I guess it is fine because it is a backport.

Cheers,

-- 
Julien Grall

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

Re: [PATCH] setup vwfi correctly on cpu0

[Stefano Stabellini]

On Mon, 3 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 31/03/17 23:33, Stefano Stabellini wrote:
> > On Fri, 31 Mar 2017, Julien Grall wrote:
> > > 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@kernel.org>
> > > > 
> > > > ---
> > > > 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 ( ... )
> > > {
> > 
> > OK
> > 
> > 
> > > > +        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.
> > 
> > Do you mean the default vwfi setting? If so, no, I am not: hcr &
> > ~(HCR_TWI|HCR_TWE) works regardless of the default.
> 
> Sorry, I was not clear. You assume that vwfi == TRAP by default, although I
> guess it is fine because it is a backport.

Yes, that was my thinking. See new patch:
alpine.DEB.2.10.1703311535180.3287@sstabellini-ThinkPad-X260

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