[PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Timur Tabi]

The Freescale hypervisor does not allow guests to write to the timebase
registers (virtualizing the timebase register was deemed too complicated),
so don't try to synchronize the timebase registers when we're running
under the hypervisor.

This typically happens when kexec support is enabled.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/platforms/85xx/p3041_ds.c |   11 +++++++++++
 arch/powerpc/platforms/85xx/p4080_ds.c |   11 +++++++++++
 arch/powerpc/platforms/85xx/p5020_ds.c |   11 +++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/p3041_ds.c b/arch/powerpc/platforms/85xx/p3041_ds.c
index c0242bc..8b651dfe 100644
--- a/arch/powerpc/platforms/85xx/p3041_ds.c
+++ b/arch/powerpc/platforms/85xx/p3041_ds.c
@@ -40,6 +40,9 @@
 static int __init p3041_ds_probe(void)
 {
 	unsigned long root = of_get_flat_dt_root();
+#ifdef CONFIG_SMP
+	extern struct smp_ops_t smp_85xx_ops;
+#endif
 
 	if (of_flat_dt_is_compatible(root, "fsl,P3041DS"))
 		return 1;
@@ -51,6 +54,14 @@ static int __init p3041_ds_probe(void)
 		ppc_md.restart = fsl_hv_restart;
 		ppc_md.power_off = fsl_hv_halt;
 		ppc_md.halt = fsl_hv_halt;
+#ifdef CONFIG_SMP
+		/*
+		 * Disable the timebase sync operations because we can't write
+		 * to the timebase registers under the hypervisor.
+		  */
+		smp_85xx_ops.give_timebase = NULL;
+		smp_85xx_ops.take_timebase = NULL;
+#endif
 		return 1;
 	}
 
diff --git a/arch/powerpc/platforms/85xx/p4080_ds.c b/arch/powerpc/platforms/85xx/p4080_ds.c
index 32ea5eb..ae859ab 100644
--- a/arch/powerpc/platforms/85xx/p4080_ds.c
+++ b/arch/powerpc/platforms/85xx/p4080_ds.c
@@ -39,6 +39,9 @@
 static int __init p4080_ds_probe(void)
 {
 	unsigned long root = of_get_flat_dt_root();
+#ifdef CONFIG_SMP
+	extern struct smp_ops_t smp_85xx_ops;
+#endif
 
 	if (of_flat_dt_is_compatible(root, "fsl,P4080DS"))
 		return 1;
@@ -50,6 +53,14 @@ static int __init p4080_ds_probe(void)
 		ppc_md.restart = fsl_hv_restart;
 		ppc_md.power_off = fsl_hv_halt;
 		ppc_md.halt = fsl_hv_halt;
+#ifdef CONFIG_SMP
+		/*
+		 * Disable the timebase sync operations because we can't write
+		 * to the timebase registers under the hypervisor.
+		  */
+		smp_85xx_ops.give_timebase = NULL;
+		smp_85xx_ops.take_timebase = NULL;
+#endif
 		return 1;
 	}
 
diff --git a/arch/powerpc/platforms/85xx/p5020_ds.c b/arch/powerpc/platforms/85xx/p5020_ds.c
index 2ea9ccc..d951618 100644
--- a/arch/powerpc/platforms/85xx/p5020_ds.c
+++ b/arch/powerpc/platforms/85xx/p5020_ds.c
@@ -40,6 +40,9 @@
 static int __init p5020_ds_probe(void)
 {
 	unsigned long root = of_get_flat_dt_root();
+#ifdef CONFIG_SMP
+	extern struct smp_ops_t smp_85xx_ops;
+#endif
 
 	if (of_flat_dt_is_compatible(root, "fsl,P5020DS"))
 		return 1;
@@ -51,6 +54,14 @@ static int __init p5020_ds_probe(void)
 		ppc_md.restart = fsl_hv_restart;
 		ppc_md.power_off = fsl_hv_halt;
 		ppc_md.halt = fsl_hv_halt;
+#ifdef CONFIG_SMP
+		/*
+		 * Disable the timebase sync operations because we can't write
+		 * to the timebase registers under the hypervisor.
+		  */
+		smp_85xx_ops.give_timebase = NULL;
+		smp_85xx_ops.take_timebase = NULL;
+#endif
 		return 1;
 	}
 
-- 
1.7.3.4

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Scott Wood]

On Tue, 14 Jun 2011 18:04:33 -0500
Timur Tabi <timur@freescale.com> wrote:

> The Freescale hypervisor does not allow guests to write to the timebase
> registers (virtualizing the timebase register was deemed too complicated),
> so don't try to synchronize the timebase registers when we're running
> under the hypervisor.
> 
> This typically happens when kexec support is enabled.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

FWIW, it's not supported under KVM either -- though we don't support an SMP
guest under KVM yet, and KVM silently ignores it rather than logs errors as
the FSL HV does.

-Scott

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Timur Tabi]

Scott Wood wrote:
> FWIW, it's not supported under KVM either -- though we don't support an SMP
> guest under KVM yet, and KVM silently ignores it rather than logs errors as
> the FSL HV does.

Does KVM set the root compatible to "fsl,P4080DS-hv"?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Scott Wood]

On Tue, 14 Jun 2011 18:15:26 -0500
Timur Tabi <timur@freescale.com> wrote:

> Scott Wood wrote:
> > FWIW, it's not supported under KVM either -- though we don't support an SMP
> > guest under KVM yet, and KVM silently ignores it rather than logs errors as
> > the FSL HV does.
> 
> Does KVM set the root compatible to "fsl,P4080DS-hv"?

No, Qemu/KVM like to pretend they're fully emulating concrete hardware,
even though there's stuff missing.

The only upstream e500 KVM platform is currently mpc8544ds.

For now, there's no SMP KVM guest support.  Maybe kexec can be fixed to not
hard-reset the core by the time that changes. :-)

-Scott

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Benjamin Herrenschmidt]

On Tue, 2011-06-14 at 18:25 -0500, Scott Wood wrote:
> On Tue, 14 Jun 2011 18:15:26 -0500
> Timur Tabi <timur@freescale.com> wrote:
> 
> > Scott Wood wrote:
> > > FWIW, it's not supported under KVM either -- though we don't support an SMP
> > > guest under KVM yet, and KVM silently ignores it rather than logs errors as
> > > the FSL HV does.
> > 
> > Does KVM set the root compatible to "fsl,P4080DS-hv"?
> 
> No, Qemu/KVM like to pretend they're fully emulating concrete hardware,
> even though there's stuff missing.
> 
> The only upstream e500 KVM platform is currently mpc8544ds.
> 
> For now, there's no SMP KVM guest support.  Maybe kexec can be fixed to not
> hard-reset the core by the time that changes. :-)

We might want to generically have a CPU feature bit indicating we are
running in guest vs. HV mode. I know Paulus is planning to introduce one
so you may want to sync with him.

Cheers,
Ben.

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Tabi Timur-B04825]

Benjamin Herrenschmidt wrote:
> We might want to generically have a CPU feature bit indicating we are
> running in guest vs. HV mode. I know Paulus is planning to introduce one
> so you may want to sync with him.

Are you talking about CPU_FTR_HVMODE_206?

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Benjamin Herrenschmidt]

On Wed, 2011-06-15 at 02:10 +0000, Tabi Timur-B04825 wrote:
> Benjamin Herrenschmidt wrote:
> > We might want to generically have a CPU feature bit indicating we are
> > running in guest vs. HV mode. I know Paulus is planning to introduce one
> > so you may want to sync with him.
> 
> Are you talking about CPU_FTR_HVMODE_206?

Well, not exactly. Paul wants to break that up since we're adding some
primitive support for 201 HV mode too (for 970's). Last we discussed,
the plan was to go for a generic HV mode bit and a separate bit for the
version.

Cheers,
Ben.

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Kumar Gala]

On Jun 14, 2011, at 9:33 PM, Benjamin Herrenschmidt wrote:

> On Wed, 2011-06-15 at 02:10 +0000, Tabi Timur-B04825 wrote:
>> Benjamin Herrenschmidt wrote:
>>> We might want to generically have a CPU feature bit indicating we =
are
>>> running in guest vs. HV mode. I know Paulus is planning to introduce =
one
>>> so you may want to sync with him.
>>=20
>> Are you talking about CPU_FTR_HVMODE_206?
>=20
> Well, not exactly. Paul wants to break that up since we're adding some
> primitive support for 201 HV mode too (for 970's). Last we discussed,
> the plan was to go for a generic HV mode bit and a separate bit for =
the
> version.
>=20
> Cheers,
> Ben.

Any ETA on Paul's intro of the FTR bit?  If not I'll pull this into my =
'next' tree and we can clean up later.

- k

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Timur Tabi]

Kumar Gala wrote:
>> > 
>> > Well, not exactly. Paul wants to break that up since we're adding some
>> > primitive support for 201 HV mode too (for 970's). Last we discussed,
>> > the plan was to go for a generic HV mode bit and a separate bit for the
>> > version.
>> > 
>> > Cheers,
>> > Ben.

> Any ETA on Paul's intro of the FTR bit?  If not I'll pull this into my 'next' tree and we can clean up later.

Just FYI, this particular patch is because of a limitation in the Freescale
hypervisor.  It's not because we're running in guest mode.  If the hypervisor
provided full emulation of the timebase register, then we wouldn't need this
patch.  The same can be said of KVM or any other hypervisor.

So a generic HV mode bit is not going to help me, unless there's also a bit
that's specific to our hypervisor.  And even then, we would need some way to
differentiate among different versions of our hypervisor, in case some future
version adds timebase support.  We currently use the device tree for all this,
so I'm not sure what a FTR bit will gain us.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Scott Wood]

On Wed, 22 Jun 2011 09:55:36 -0500
Timur Tabi <timur@freescale.com> wrote:

> Kumar Gala wrote:
> >> >=20
> >> > Well, not exactly. Paul wants to break that up since we're adding so=
me
> >> > primitive support for 201 HV mode too (for 970's). Last we discussed,
> >> > the plan was to go for a generic HV mode bit and a separate bit for =
the
> >> > version.
> >> >=20
> >> > Cheers,
> >> > Ben.
>=20
> > Any ETA on Paul's intro of the FTR bit?  If not I'll pull this into my =
'next' tree and we can clean up later.
>=20
> Just FYI, this particular patch is because of a limitation in the Freesca=
le
> hypervisor.  It's not because we're running in guest mode.  If the hyperv=
isor
> provided full emulation of the timebase register, then we wouldn't need t=
his
> patch.  The same can be said of KVM or any other hypervisor.

=46rom Power ISA 2.06B, book III-E, section 9.2.1:

Virtualized Implementation Note:

In virtualized implementations, TBU and TBL are
read-only.

> So a generic HV mode bit is not going to help me, unless there's also a b=
it
> that's specific to our hypervisor.  And even then, we would need some way=
 to
> differentiate among different versions of our hypervisor, in case some fu=
ture
> version adds timebase support.=20

That's very unlikely to happen.

Ideally we would avoid doing this sync even when not running under a
hypervisor, as long as firmware has done the sync, and kexec hasn't messed
it up.  Besides being a waste of boot time, the firmware's sync is
probably tighter since it can use a platform-specific mechanism to start all
the timebases at once.

-Scott

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Timur Tabi]

Scott Wood wrote:
> From Power ISA 2.06B, book III-E, section 9.2.1:
> 
> Virtualized Implementation Note:
> 
> In virtualized implementations, TBU and TBL are
> read-only.

But does that mean that a guest should never be allowed to modify a virtualized
timebase register, even if the hypervisor can support it?

>> > So a generic HV mode bit is not going to help me, unless there's also a bit
>> > that's specific to our hypervisor.  And even then, we would need some way to
>> > differentiate among different versions of our hypervisor, in case some future
>> > version adds timebase support. 

> That's very unlikely to happen.

I know. I was just being architecturally pedantic.

> Ideally we would avoid doing this sync even when not running under a
> hypervisor, as long as firmware has done the sync, and kexec hasn't messed
> it up.  Besides being a waste of boot time, the firmware's sync is
> probably tighter since it can use a platform-specific mechanism to start all
> the timebases at once.

I agree with that, but for now, I need to work around that kexec "limitation".

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Scott Wood]

On Thu, 23 Jun 2011 12:33:40 -0500
Timur Tabi <timur@freescale.com> wrote:

> Scott Wood wrote:
> > From Power ISA 2.06B, book III-E, section 9.2.1:
> > 
> > Virtualized Implementation Note:
> > 
> > In virtualized implementations, TBU and TBL are
> > read-only.
> 
> But does that mean that a guest should never be allowed to modify a virtualized
> timebase register, even if the hypervisor can support it?

The book3e mtspr writeup doesn't appear to specify the behavior when
writing to a read-only SPR, so perhaps you could argue that something other
than a no-op is implementation-specific behavior.

For a guest, the safe thing is to not write to those registers unless you
specifically know it's going to do what you want under a particular
implementation.  It's not specifically a Topaz limitation.

> >> > So a generic HV mode bit is not going to help me, unless there's also a bit
> >> > that's specific to our hypervisor.  And even then, we would need some way to
> >> > differentiate among different versions of our hypervisor, in case some future
> >> > version adds timebase support. 
> 
> > That's very unlikely to happen.
> 
> I know. I was just being architecturally pedantic.

It's not as if it would hurt anything to ignore such a capability.

> > Ideally we would avoid doing this sync even when not running under a
> > hypervisor, as long as firmware has done the sync, and kexec hasn't messed
> > it up.  Besides being a waste of boot time, the firmware's sync is
> > probably tighter since it can use a platform-specific mechanism to start all
> > the timebases at once.
> 
> I agree with that, but for now, I need to work around that kexec "limitation".

Is there any way we can detect whether we booted via kexec (as opposed to
just having kexec support enabled), and only do the sync in that case?

-Scott

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Segher Boessenkool]

>> But does that mean that a guest should never be allowed to modify a 
>> virtualized
>> timebase register, even if the hypervisor can support it?
>
> The book3e mtspr writeup doesn't appear to specify the behavior when
> writing to a read-only SPR, so perhaps you could argue that something 
> other
> than a no-op is implementation-specific behavior.

v2.06 III-E 9.2.1:
"Writing the Time Base is hypervisor privileged."

v2.06 III-E 2.1:
"If a hypervisor-privileged register is accessed in the guest supervisor
state (MSR[GS PR] = 0b10), an Embedded Hypervisor Privilege exception 
occurs."

(v2.06 III-E 5.4.1, the big SPR table, also shows the TB regs (for 
writing,
i.e. 284 and 285) to be hypervisor privileged.  Consistency, hurray :-) 
)


Segher

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Tabi Timur-B04825]

Segher Boessenkool wrote:
>
> v2.06 III-E 9.2.1:
> "Writing the Time Base is hypervisor privileged."
>
> v2.06 III-E 2.1:
> "If a hypervisor-privileged register is accessed in the guest supervisor
> state (MSR[GS PR] =3D 0b10), an Embedded Hypervisor Privilege exception
> occurs."
>
> (v2.06 III-E 5.4.1, the big SPR table, also shows the TB regs (for writin=
g,
> i.e. 284 and 285) to be hypervisor privileged.  Consistency, hurray :-) )

To me, all this means that a guest cannot write to the actual timebase=20
register.  I'm not interpreting this to mean that a hypervisor can't=20
virtualize the timebase and allow a guest to read/write a virtual timebase=
=20
register, so that it thinks it's writing to the real hardware timebase regi=
ster.

--=20
Timur Tabi
Linux kernel developer at Freescale=

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Segher Boessenkool]

(context put back:)

>> But does that mean that a guest should never be allowed to modify a 
>> virtualized
>> timebase register, even if the hypervisor can support it?
>
> The book3e mtspr writeup doesn't appear to specify the behavior when
> writing to a read-only SPR, so perhaps you could argue that something 
> other
> than a no-op is implementation-specific behavior.

>> v2.06 III-E 9.2.1:
>> "Writing the Time Base is hypervisor privileged."
>>
>> v2.06 III-E 2.1:
>> "If a hypervisor-privileged register is accessed in the guest 
>> supervisor
>> state (MSR[GS PR] = 0b10), an Embedded Hypervisor Privilege exception
>> occurs."
>>
>> (v2.06 III-E 5.4.1, the big SPR table, also shows the TB regs (for 
>> writing,
>> i.e. 284 and 285) to be hypervisor privileged.  Consistency, hurray 
>> :-) )
>
> To me, all this means that a guest cannot write to the actual timebase
> register.

It also means that the hypervisor gets a trap when a guest tries to do 
this.

>   I'm not interpreting this to mean that a hypervisor can't
> virtualize the timebase and allow a guest to read/write a virtual 
> timebase
> register, so that it thinks it's writing to the real hardware timebase 
> register.

Yes, a hypervisor can do this.  The behaviour of the hardware is not
implementation-specific (modulo bugs ;-) ); when a guest tries to write
to the timebase, the hypervisor gets a trap.  The hypervisor can then
do whatever it wants with it.


Segher

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Scott Wood]

On Thu, 23 Jun 2011 21:38:58 -0500
Tabi Timur-B04825 <B04825@freescale.com> wrote:

> Segher Boessenkool wrote:
> >
> > v2.06 III-E 9.2.1:
> > "Writing the Time Base is hypervisor privileged."
> >
> > v2.06 III-E 2.1:
> > "If a hypervisor-privileged register is accessed in the guest supervisor
> > state (MSR[GS PR] = 0b10), an Embedded Hypervisor Privilege exception
> > occurs."
> >
> > (v2.06 III-E 5.4.1, the big SPR table, also shows the TB regs (for writing,
> > i.e. 284 and 285) to be hypervisor privileged.  Consistency, hurray :-) )
> 
> To me, all this means that a guest cannot write to the actual timebase 
> register.  I'm not interpreting this to mean that a hypervisor can't 
> virtualize the timebase and allow a guest to read/write a virtual timebase 
> register, so that it thinks it's writing to the real hardware timebase register.
> 

Right, I was referring to the virtualized implementation note added in
2.06B.  The virtualized implementation notes apply to what happens in the
guest as seen by the guest (considered as a separate implementation of the
Power ISA), not to what happens at a hardware level in guest mode.

-Scott

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Benjamin Herrenschmidt]

On Wed, 2011-06-22 at 06:44 -0500, Kumar Gala wrote:
> 
> Any ETA on Paul's intro of the FTR bit?  If not I'll pull this into my
> 'next' tree and we can clean up later.

His latest KVM patch set has that.

Cheers,
Ben.

Re: [PATCH] powerpc/85xx: disable timebase synchronization under the hypervisor

[Kumar Gala]

On Jun 14, 2011, at 6:04 PM, Timur Tabi wrote:

> The Freescale hypervisor does not allow guests to write to the timebase
> registers (virtualizing the timebase register was deemed too complicated),
> so don't try to synchronize the timebase registers when we're running
> under the hypervisor.
> 
> This typically happens when kexec support is enabled.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> arch/powerpc/platforms/85xx/p3041_ds.c |   11 +++++++++++
> arch/powerpc/platforms/85xx/p4080_ds.c |   11 +++++++++++
> arch/powerpc/platforms/85xx/p5020_ds.c |   11 +++++++++++
> 3 files changed, 33 insertions(+), 0 deletions(-)

applied to next

- K