Re: [Xen-devel] [PATCH] setup vwfi correctly on cpu0
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
> > > >
> > > > ---
> > > > 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
Re: [Xen-devel] [PATCH] setup vwfi correctly on cpu0
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
---
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: [Xen-devel] [PATCH] setup vwfi correctly on cpu0
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
> >
> > ---
> > 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: [Xen-devel] [PATCH] setup vwfi correctly on cpu0
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
---
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
[Xen-devel] [PATCH] setup vwfi correctly on cpu0
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
---
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
