[PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat Bhushan]

SPRG3 is guest accessible and SPRG3 can be clobbered by host
or another guest, So this need to be restored when loading
guest state.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/kvm/booke_interrupts.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index 2c6deb5ef..0d3403f 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -459,6 +459,8 @@ lightweight_exit:
 	 * written directly to the shared area, so we
 	 * need to reload them here with the guest's values.
 	 */
+	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
+	mtspr	SPRN_SPRG3, r3
 	PPC_LD(r3, VCPU_SHARED_SPRG4, r5)
 	mtspr	SPRN_SPRG4W, r3
 	PPC_LD(r3, VCPU_SHARED_SPRG5, r5)
-- 
1.9.3

[PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat Bhushan]

SPRG3 is guest accessible and SPRG3 can be clobbered by host
or another guest, So this need to be restored when loading
guest state.

Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
---
 arch/powerpc/kvm/booke_interrupts.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
index 2c6deb5ef..0d3403f 100644
--- a/arch/powerpc/kvm/booke_interrupts.S
+++ b/arch/powerpc/kvm/booke_interrupts.S
@@ -459,6 +459,8 @@ lightweight_exit:
 	 * written directly to the shared area, so we
 	 * need to reload them here with the guest's values.
 	 */
+	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
+	mtspr	SPRN_SPRG3, r3
 	PPC_LD(r3, VCPU_SHARED_SPRG4, r5)
 	mtspr	SPRN_SPRG4W, r3
 	PPC_LD(r3, VCPU_SHARED_SPRG5, r5)
-- 
1.9.3

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 16.07.14 08:02, Bharat Bhushan wrote:
> SPRG3 is guest accessible and SPRG3 can be clobbered by host
> or another guest, So this need to be restored when loading
> guest state.
>
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>   arch/powerpc/kvm/booke_interrupts.S | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
> index 2c6deb5ef..0d3403f 100644
> --- a/arch/powerpc/kvm/booke_interrupts.S
> +++ b/arch/powerpc/kvm/booke_interrupts.S
> @@ -459,6 +459,8 @@ lightweight_exit:
>   	 * written directly to the shared area, so we
>   	 * need to reload them here with the guest's values.
>   	 */
> +	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> +	mtspr	SPRN_SPRG3, r3

We also need to restore it when resuming the host, no?


Alex

>   	PPC_LD(r3, VCPU_SHARED_SPRG4, r5)
>   	mtspr	SPRN_SPRG4W, r3
>   	PPC_LD(r3, VCPU_SHARED_SPRG5, r5)

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 16.07.14 08:02, Bharat Bhushan wrote:
> SPRG3 is guest accessible and SPRG3 can be clobbered by host
> or another guest, So this need to be restored when loading
> guest state.
>
> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> ---
>   arch/powerpc/kvm/booke_interrupts.S | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kvm/booke_interrupts.S b/arch/powerpc/kvm/booke_interrupts.S
> index 2c6deb5ef..0d3403f 100644
> --- a/arch/powerpc/kvm/booke_interrupts.S
> +++ b/arch/powerpc/kvm/booke_interrupts.S
> @@ -459,6 +459,8 @@ lightweight_exit:
>   	 * written directly to the shared area, so we
>   	 * need to reload them here with the guest's values.
>   	 */
> +	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> +	mtspr	SPRN_SPRG3, r3

We also need to restore it when resuming the host, no?


Alex

>   	PPC_LD(r3, VCPU_SHARED_SPRG4, r5)
>   	mtspr	SPRN_SPRG4W, r3
>   	PPC_LD(r3, VCPU_SHARED_SPRG5, r5)

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 17, 2014 9:41 PM
> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> 
> 
> On 16.07.14 08:02, Bharat Bhushan wrote:
> > SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> > another guest, So this need to be restored when loading guest state.
> >
> > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > ---
> >   arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/powerpc/kvm/booke_interrupts.S
> > b/arch/powerpc/kvm/booke_interrupts.S
> > index 2c6deb5ef..0d3403f 100644
> > --- a/arch/powerpc/kvm/booke_interrupts.S
> > +++ b/arch/powerpc/kvm/booke_interrupts.S
> > @@ -459,6 +459,8 @@ lightweight_exit:
> >   	 * written directly to the shared area, so we
> >   	 * need to reload them here with the guest's values.
> >   	 */
> > +	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> > +	mtspr	SPRN_SPRG3, r3
> 
> We also need to restore it when resuming the host, no?

I do not think host expect some meaningful value when returning from guest, same true for SPRG4-7.
So there seems no reason to save host values and restore them.

Thanks
-Bharat
> 
> 
> Alex
> 
> >   	PPC_LD(r3, VCPU_SHARED_SPRG4, r5)
> >   	mtspr	SPRN_SPRG4W, r3
> >   	PPC_LD(r3, VCPU_SHARED_SPRG5, r5)

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, July 17, 2014 9:41 PM
>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>
>>
>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>> another guest, So this need to be restored when loading guest state.
>>>
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> ---
>>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>> b/arch/powerpc/kvm/booke_interrupts.S
>>> index 2c6deb5ef..0d3403f 100644
>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>    	 * written directly to the shared area, so we
>>>    	 * need to reload them here with the guest's values.
>>>    	 */
>>> +	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>> +	mtspr	SPRN_SPRG3, r3
>> We also need to restore it when resuming the host, no?
> I do not think host expect some meaningful value when returning from guest, same true for SPRG4-7.
> So there seems no reason to save host values and restore them.

Hmm - arch/powerpc/include/asm/reg.h says:

  * All 32-bit:
  *      - SPRG3 current thread_info pointer
  *        (virtual on BookE, physical on others)

but I can indeed find no trace of usage anywhere. This at least needs to 
go into the patch description.


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, July 17, 2014 9:41 PM
>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>
>>
>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>> another guest, So this need to be restored when loading guest state.
>>>
>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>> ---
>>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>> b/arch/powerpc/kvm/booke_interrupts.S
>>> index 2c6deb5ef..0d3403f 100644
>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>    	 * written directly to the shared area, so we
>>>    	 * need to reload them here with the guest's values.
>>>    	 */
>>> +	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>> +	mtspr	SPRN_SPRG3, r3
>> We also need to restore it when resuming the host, no?
> I do not think host expect some meaningful value when returning from guest, same true for SPRG4-7.
> So there seems no reason to save host values and restore them.

Hmm - arch/powerpc/include/asm/reg.h says:

  * All 32-bit:
  *      - SPRG3 current thread_info pointer
  *        (virtual on BookE, physical on others)

but I can indeed find no trace of usage anywhere. This at least needs to 
go into the patch description.


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 17.07.14 18:27, Alexander Graf wrote:
>
> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Thursday, July 17, 2014 9:41 PM
>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>
>>>
>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>> another guest, So this need to be restored when loading guest state.
>>>>
>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>> ---
>>>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>> index 2c6deb5ef..0d3403f 100644
>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>         * written directly to the shared area, so we
>>>>         * need to reload them here with the guest's values.
>>>>         */
>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>> +    mtspr    SPRN_SPRG3, r3
>>> We also need to restore it when resuming the host, no?
>> I do not think host expect some meaningful value when returning from 
>> guest, same true for SPRG4-7.
>> So there seems no reason to save host values and restore them.
>
> Hmm - arch/powerpc/include/asm/reg.h says:
>
>  * All 32-bit:
>  *      - SPRG3 current thread_info pointer
>  *        (virtual on BookE, physical on others)
>
> but I can indeed find no trace of usage anywhere. This at least needs 
> to go into the patch description.

Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so 
incredibly important that I have no idea how we could possibly run 
without switching the host value back in very early. And even then our 
interrupt handlers wouldn't work anymore.

This is more complicated :).


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 17.07.14 18:27, Alexander Graf wrote:
>
> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Thursday, July 17, 2014 9:41 PM
>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>
>>>
>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>> another guest, So this need to be restored when loading guest state.
>>>>
>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>> ---
>>>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>> index 2c6deb5ef..0d3403f 100644
>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>         * written directly to the shared area, so we
>>>>         * need to reload them here with the guest's values.
>>>>         */
>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>> +    mtspr    SPRN_SPRG3, r3
>>> We also need to restore it when resuming the host, no?
>> I do not think host expect some meaningful value when returning from 
>> guest, same true for SPRG4-7.
>> So there seems no reason to save host values and restore them.
>
> Hmm - arch/powerpc/include/asm/reg.h says:
>
>  * All 32-bit:
>  *      - SPRG3 current thread_info pointer
>  *        (virtual on BookE, physical on others)
>
> but I can indeed find no trace of usage anywhere. This at least needs 
> to go into the patch description.

Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so 
incredibly important that I have no idea how we could possibly run 
without switching the host value back in very early. And even then our 
interrupt handlers wouldn't work anymore.

This is more complicated :).


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> On 17.07.14 18:27, Alexander Graf wrote:
> >
> > On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>
> >>> -----Original Message-----
> >>> From: Alexander Graf [mailto:agraf@suse.de]
> >>> Sent: Thursday, July 17, 2014 9:41 PM
> >>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> >>>
> >>>
> >>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>>> another guest, So this need to be restored when loading guest state.

SPRG3 is not guest writeable.  We should be doing this so that guest
reads of SPRG3 through the alternative read-only SPR work, not because
"SPRG3 can be clobbered by host or another guest".

> >>>>
> >>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>> ---
> >>>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>    1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>> index 2c6deb5ef..0d3403f 100644
> >>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>         * written directly to the shared area, so we
> >>>>         * need to reload them here with the guest's values.
> >>>>         */
> >>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>> +    mtspr    SPRN_SPRG3, r3
> >>> We also need to restore it when resuming the host, no?
> >> I do not think host expect some meaningful value when returning from 
> >> guest, same true for SPRG4-7.
> >> So there seems no reason to save host values and restore them.

Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
Alex points out.

> > Hmm - arch/powerpc/include/asm/reg.h says:
> >
> >  * All 32-bit:
> >  *      - SPRG3 current thread_info pointer
> >  *        (virtual on BookE, physical on others)
> >
> > but I can indeed find no trace of usage anywhere. This at least needs 
> > to go into the patch description.
> 
> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so 
> incredibly important that I have no idea how we could possibly run 
> without switching the host value back in very early. And even then our 
> interrupt handlers wouldn't work anymore.
> 
> This is more complicated :).

To make this work we need to avoid SPRG3 as well, or at least avoid
using it for something needed prior to DO_KVM.

We also need to update the documentation in reg.h to reflect the fact
that we don't use SPRG4-7 anymore on e500.

-Scott

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> On 17.07.14 18:27, Alexander Graf wrote:
> >
> > On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>
> >>> -----Original Message-----
> >>> From: Alexander Graf [mailto:agraf@suse.de]
> >>> Sent: Thursday, July 17, 2014 9:41 PM
> >>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> >>>
> >>>
> >>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>>> another guest, So this need to be restored when loading guest state.

SPRG3 is not guest writeable.  We should be doing this so that guest
reads of SPRG3 through the alternative read-only SPR work, not because
"SPRG3 can be clobbered by host or another guest".

> >>>>
> >>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>> ---
> >>>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>    1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>> index 2c6deb5ef..0d3403f 100644
> >>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>         * written directly to the shared area, so we
> >>>>         * need to reload them here with the guest's values.
> >>>>         */
> >>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>> +    mtspr    SPRN_SPRG3, r3
> >>> We also need to restore it when resuming the host, no?
> >> I do not think host expect some meaningful value when returning from 
> >> guest, same true for SPRG4-7.
> >> So there seems no reason to save host values and restore them.

Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
Alex points out.

> > Hmm - arch/powerpc/include/asm/reg.h says:
> >
> >  * All 32-bit:
> >  *      - SPRG3 current thread_info pointer
> >  *        (virtual on BookE, physical on others)
> >
> > but I can indeed find no trace of usage anywhere. This at least needs 
> > to go into the patch description.
> 
> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so 
> incredibly important that I have no idea how we could possibly run 
> without switching the host value back in very early. And even then our 
> interrupt handlers wouldn't work anymore.
> 
> This is more complicated :).

To make this work we need to avoid SPRG3 as well, or at least avoid
using it for something needed prior to DO_KVM.

We also need to update the documentation in reg.h to reflect the fact
that we don't use SPRG4-7 anymore on e500.

-Scott

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 02:28, Scott Wood wrote:
> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>> On 17.07.14 18:27, Alexander Graf wrote:
>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>> -----Original Message-----
>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>>>
>>>>>
>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>>>> another guest, So this need to be restored when loading guest state.
> SPRG3 is not guest writeable.  We should be doing this so that guest
> reads of SPRG3 through the alternative read-only SPR work, not because
> "SPRG3 can be clobbered by host or another guest".
>
>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>> ---
>>>>>>     arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>          * written directly to the shared area, so we
>>>>>>          * need to reload them here with the guest's values.
>>>>>>          */
>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>> We also need to restore it when resuming the host, no?
>>>> I do not think host expect some meaningful value when returning from
>>>> guest, same true for SPRG4-7.
>>>> So there seems no reason to save host values and restore them.
> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
> Alex points out.
>
>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>
>>>   * All 32-bit:
>>>   *      - SPRG3 current thread_info pointer
>>>   *        (virtual on BookE, physical on others)
>>>
>>> but I can indeed find no trace of usage anywhere. This at least needs
>>> to go into the patch description.
>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>> incredibly important that I have no idea how we could possibly run
>> without switching the host value back in very early. And even then our
>> interrupt handlers wouldn't work anymore.
>>
>> This is more complicated :).
> To make this work we need to avoid SPRG3 as well, or at least avoid
> using it for something needed prior to DO_KVM.
>
> We also need to update the documentation in reg.h to reflect the fact
> that we don't use SPRG4-7 anymore on e500.

I would personally prefer if we claim SPRG3R as unsupported on e500v2 
until we find someone who actually uses it. There's a good chance we'd 
start jumping through a lot of hoops and reduce overall performance for 
no real-world gain today.


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 02:28, Scott Wood wrote:
> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>> On 17.07.14 18:27, Alexander Graf wrote:
>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>> -----Original Message-----
>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>>>
>>>>>
>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>>>> another guest, So this need to be restored when loading guest state.
> SPRG3 is not guest writeable.  We should be doing this so that guest
> reads of SPRG3 through the alternative read-only SPR work, not because
> "SPRG3 can be clobbered by host or another guest".
>
>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>> ---
>>>>>>     arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>          * written directly to the shared area, so we
>>>>>>          * need to reload them here with the guest's values.
>>>>>>          */
>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>> We also need to restore it when resuming the host, no?
>>>> I do not think host expect some meaningful value when returning from
>>>> guest, same true for SPRG4-7.
>>>> So there seems no reason to save host values and restore them.
> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
> Alex points out.
>
>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>
>>>   * All 32-bit:
>>>   *      - SPRG3 current thread_info pointer
>>>   *        (virtual on BookE, physical on others)
>>>
>>> but I can indeed find no trace of usage anywhere. This at least needs
>>> to go into the patch description.
>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>> incredibly important that I have no idea how we could possibly run
>> without switching the host value back in very early. And even then our
>> interrupt handlers wouldn't work anymore.
>>
>> This is more complicated :).
> To make this work we need to avoid SPRG3 as well, or at least avoid
> using it for something needed prior to DO_KVM.
>
> We also need to update the documentation in reg.h to reflect the fact
> that we don't use SPRG4-7 anymore on e500.

I would personally prefer if we claim SPRG3R as unsupported on e500v2 
until we find someone who actually uses it. There's a good chance we'd 
start jumping through a lot of hoops and reduce overall performance for 
no real-world gain today.


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> On 18.07.14 02:28, Scott Wood wrote:
> > On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> >> On 17.07.14 18:27, Alexander Graf wrote:
> >>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>>>> -----Original Message-----
> >>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>> Sent: Thursday, July 17, 2014 9:41 PM
> >>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> >>>>>
> >>>>>
> >>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>>>>> another guest, So this need to be restored when loading guest state.
> > SPRG3 is not guest writeable.  We should be doing this so that guest
> > reads of SPRG3 through the alternative read-only SPR work, not because
> > "SPRG3 can be clobbered by host or another guest".
> >
> >>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>> ---
> >>>>>>     arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>>>     1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> index 2c6deb5ef..0d3403f 100644
> >>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>>>          * written directly to the shared area, so we
> >>>>>>          * need to reload them here with the guest's values.
> >>>>>>          */
> >>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>>>> +    mtspr    SPRN_SPRG3, r3
> >>>>> We also need to restore it when resuming the host, no?
> >>>> I do not think host expect some meaningful value when returning from
> >>>> guest, same true for SPRG4-7.
> >>>> So there seems no reason to save host values and restore them.
> > Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
> > Alex points out.
> >
> >>> Hmm - arch/powerpc/include/asm/reg.h says:
> >>>
> >>>   * All 32-bit:
> >>>   *      - SPRG3 current thread_info pointer
> >>>   *        (virtual on BookE, physical on others)
> >>>
> >>> but I can indeed find no trace of usage anywhere. This at least needs
> >>> to go into the patch description.
> >> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> >> incredibly important that I have no idea how we could possibly run
> >> without switching the host value back in very early. And even then our
> >> interrupt handlers wouldn't work anymore.
> >>
> >> This is more complicated :).
> > To make this work we need to avoid SPRG3 as well, or at least avoid
> > using it for something needed prior to DO_KVM.
> >
> > We also need to update the documentation in reg.h to reflect the fact
> > that we don't use SPRG4-7 anymore on e500.
> 
> I would personally prefer if we claim SPRG3R as unsupported on e500v2 
> until we find someone who actually uses it. There's a good chance we'd 
> start jumping through a lot of hoops and reduce overall performance for 
> no real-world gain today.

The same problem applies to e500mc.

-Scott

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> On 18.07.14 02:28, Scott Wood wrote:
> > On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> >> On 17.07.14 18:27, Alexander Graf wrote:
> >>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>>>> -----Original Message-----
> >>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>> Sent: Thursday, July 17, 2014 9:41 PM
> >>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> >>>>>
> >>>>>
> >>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>>>>> another guest, So this need to be restored when loading guest state.
> > SPRG3 is not guest writeable.  We should be doing this so that guest
> > reads of SPRG3 through the alternative read-only SPR work, not because
> > "SPRG3 can be clobbered by host or another guest".
> >
> >>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>> ---
> >>>>>>     arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>>>     1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> index 2c6deb5ef..0d3403f 100644
> >>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>>>          * written directly to the shared area, so we
> >>>>>>          * need to reload them here with the guest's values.
> >>>>>>          */
> >>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>>>> +    mtspr    SPRN_SPRG3, r3
> >>>>> We also need to restore it when resuming the host, no?
> >>>> I do not think host expect some meaningful value when returning from
> >>>> guest, same true for SPRG4-7.
> >>>> So there seems no reason to save host values and restore them.
> > Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
> > Alex points out.
> >
> >>> Hmm - arch/powerpc/include/asm/reg.h says:
> >>>
> >>>   * All 32-bit:
> >>>   *      - SPRG3 current thread_info pointer
> >>>   *        (virtual on BookE, physical on others)
> >>>
> >>> but I can indeed find no trace of usage anywhere. This at least needs
> >>> to go into the patch description.
> >> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> >> incredibly important that I have no idea how we could possibly run
> >> without switching the host value back in very early. And even then our
> >> interrupt handlers wouldn't work anymore.
> >>
> >> This is more complicated :).
> > To make this work we need to avoid SPRG3 as well, or at least avoid
> > using it for something needed prior to DO_KVM.
> >
> > We also need to update the documentation in reg.h to reflect the fact
> > that we don't use SPRG4-7 anymore on e500.
> 
> I would personally prefer if we claim SPRG3R as unsupported on e500v2 
> until we find someone who actually uses it. There's a good chance we'd 
> start jumping through a lot of hoops and reduce overall performance for 
> no real-world gain today.

The same problem applies to e500mc.

-Scott

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 02:36, Scott Wood wrote:
> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
>> On 18.07.14 02:28, Scott Wood wrote:
>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>>>> On 17.07.14 18:27, Alexander Graf wrote:
>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>>>>>
>>>>>>>
>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>>>>>> another guest, So this need to be restored when loading guest state.
>>> SPRG3 is not guest writeable.  We should be doing this so that guest
>>> reads of SPRG3 through the alternative read-only SPR work, not because
>>> "SPRG3 can be clobbered by host or another guest".
>>>
>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>> ---
>>>>>>>>      arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>>>      1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>>>           * written directly to the shared area, so we
>>>>>>>>           * need to reload them here with the guest's values.
>>>>>>>>           */
>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>>>> We also need to restore it when resuming the host, no?
>>>>>> I do not think host expect some meaningful value when returning from
>>>>>> guest, same true for SPRG4-7.
>>>>>> So there seems no reason to save host values and restore them.
>>> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
>>> Alex points out.
>>>
>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>>>
>>>>>    * All 32-bit:
>>>>>    *      - SPRG3 current thread_info pointer
>>>>>    *        (virtual on BookE, physical on others)
>>>>>
>>>>> but I can indeed find no trace of usage anywhere. This at least needs
>>>>> to go into the patch description.
>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>>>> incredibly important that I have no idea how we could possibly run
>>>> without switching the host value back in very early. And even then our
>>>> interrupt handlers wouldn't work anymore.
>>>>
>>>> This is more complicated :).
>>> To make this work we need to avoid SPRG3 as well, or at least avoid
>>> using it for something needed prior to DO_KVM.
>>>
>>> We also need to update the documentation in reg.h to reflect the fact
>>> that we don't use SPRG4-7 anymore on e500.
>> I would personally prefer if we claim SPRG3R as unsupported on e500v2
>> until we find someone who actually uses it. There's a good chance we'd
>> start jumping through a lot of hoops and reduce overall performance for
>> no real-world gain today.
> The same problem applies to e500mc.

There we have SPRN_GSPRG3, no?


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 02:36, Scott Wood wrote:
> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
>> On 18.07.14 02:28, Scott Wood wrote:
>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>>>> On 17.07.14 18:27, Alexander Graf wrote:
>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>>>>>
>>>>>>>
>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>>>>>> another guest, So this need to be restored when loading guest state.
>>> SPRG3 is not guest writeable.  We should be doing this so that guest
>>> reads of SPRG3 through the alternative read-only SPR work, not because
>>> "SPRG3 can be clobbered by host or another guest".
>>>
>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>> ---
>>>>>>>>      arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>>>      1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>>>           * written directly to the shared area, so we
>>>>>>>>           * need to reload them here with the guest's values.
>>>>>>>>           */
>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>>>> We also need to restore it when resuming the host, no?
>>>>>> I do not think host expect some meaningful value when returning from
>>>>>> guest, same true for SPRG4-7.
>>>>>> So there seems no reason to save host values and restore them.
>>> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
>>> Alex points out.
>>>
>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>>>
>>>>>    * All 32-bit:
>>>>>    *      - SPRG3 current thread_info pointer
>>>>>    *        (virtual on BookE, physical on others)
>>>>>
>>>>> but I can indeed find no trace of usage anywhere. This at least needs
>>>>> to go into the patch description.
>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>>>> incredibly important that I have no idea how we could possibly run
>>>> without switching the host value back in very early. And even then our
>>>> interrupt handlers wouldn't work anymore.
>>>>
>>>> This is more complicated :).
>>> To make this work we need to avoid SPRG3 as well, or at least avoid
>>> using it for something needed prior to DO_KVM.
>>>
>>> We also need to update the documentation in reg.h to reflect the fact
>>> that we don't use SPRG4-7 anymore on e500.
>> I would personally prefer if we claim SPRG3R as unsupported on e500v2
>> until we find someone who actually uses it. There's a good chance we'd
>> start jumping through a lot of hoops and reduce overall performance for
>> no real-world gain today.
> The same problem applies to e500mc.

There we have SPRN_GSPRG3, no?


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
> On 18.07.14 02:36, Scott Wood wrote:
> > On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> >> On 18.07.14 02:28, Scott Wood wrote:
> >>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> >>>> On 17.07.14 18:27, Alexander Graf wrote:
> >>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
> >>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> >>>>>>>
> >>>>>>>
> >>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>>>>>>> another guest, So this need to be restored when loading guest state.
> >>> SPRG3 is not guest writeable.  We should be doing this so that guest
> >>> reads of SPRG3 through the alternative read-only SPR work, not because
> >>> "SPRG3 can be clobbered by host or another guest".
> >>>
> >>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>>>> ---
> >>>>>>>>      arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>>>>>      1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> index 2c6deb5ef..0d3403f 100644
> >>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>>>>>           * written directly to the shared area, so we
> >>>>>>>>           * need to reload them here with the guest's values.
> >>>>>>>>           */
> >>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>>>>>> +    mtspr    SPRN_SPRG3, r3
> >>>>>>> We also need to restore it when resuming the host, no?
> >>>>>> I do not think host expect some meaningful value when returning from
> >>>>>> guest, same true for SPRG4-7.
> >>>>>> So there seems no reason to save host values and restore them.
> >>> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
> >>> Alex points out.
> >>>
> >>>>> Hmm - arch/powerpc/include/asm/reg.h says:
> >>>>>
> >>>>>    * All 32-bit:
> >>>>>    *      - SPRG3 current thread_info pointer
> >>>>>    *        (virtual on BookE, physical on others)
> >>>>>
> >>>>> but I can indeed find no trace of usage anywhere. This at least needs
> >>>>> to go into the patch description.
> >>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> >>>> incredibly important that I have no idea how we could possibly run
> >>>> without switching the host value back in very early. And even then our
> >>>> interrupt handlers wouldn't work anymore.
> >>>>
> >>>> This is more complicated :).
> >>> To make this work we need to avoid SPRG3 as well, or at least avoid
> >>> using it for something needed prior to DO_KVM.
> >>>
> >>> We also need to update the documentation in reg.h to reflect the fact
> >>> that we don't use SPRG4-7 anymore on e500.
> >> I would personally prefer if we claim SPRG3R as unsupported on e500v2
> >> until we find someone who actually uses it. There's a good chance we'd
> >> start jumping through a lot of hoops and reduce overall performance for
> >> no real-world gain today.
> > The same problem applies to e500mc.
> 
> There we have SPRN_GSPRG3, no?

Oh, right.

Since it's only a problem for PR-mode, it can be fixed without needing
to avoid SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only
need to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
ffe129ecd79779221fdb03305049ec8b5a8beb0f).

And if we decide it's not worthwhile and don't revert that commit, we
should at least remove the comment that "Under KVM, the host SPRG1 is
used to point to the current VCPU data structure"...

-Scott

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
> On 18.07.14 02:36, Scott Wood wrote:
> > On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> >> On 18.07.14 02:28, Scott Wood wrote:
> >>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> >>>> On 17.07.14 18:27, Alexander Graf wrote:
> >>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
> >>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> >>>>>>>
> >>>>>>>
> >>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>>>>>>> another guest, So this need to be restored when loading guest state.
> >>> SPRG3 is not guest writeable.  We should be doing this so that guest
> >>> reads of SPRG3 through the alternative read-only SPR work, not because
> >>> "SPRG3 can be clobbered by host or another guest".
> >>>
> >>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>>>> ---
> >>>>>>>>      arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>>>>>      1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> index 2c6deb5ef..0d3403f 100644
> >>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>>>>>           * written directly to the shared area, so we
> >>>>>>>>           * need to reload them here with the guest's values.
> >>>>>>>>           */
> >>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>>>>>> +    mtspr    SPRN_SPRG3, r3
> >>>>>>> We also need to restore it when resuming the host, no?
> >>>>>> I do not think host expect some meaningful value when returning from
> >>>>>> guest, same true for SPRG4-7.
> >>>>>> So there seems no reason to save host values and restore them.
> >>> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
> >>> Alex points out.
> >>>
> >>>>> Hmm - arch/powerpc/include/asm/reg.h says:
> >>>>>
> >>>>>    * All 32-bit:
> >>>>>    *      - SPRG3 current thread_info pointer
> >>>>>    *        (virtual on BookE, physical on others)
> >>>>>
> >>>>> but I can indeed find no trace of usage anywhere. This at least needs
> >>>>> to go into the patch description.
> >>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> >>>> incredibly important that I have no idea how we could possibly run
> >>>> without switching the host value back in very early. And even then our
> >>>> interrupt handlers wouldn't work anymore.
> >>>>
> >>>> This is more complicated :).
> >>> To make this work we need to avoid SPRG3 as well, or at least avoid
> >>> using it for something needed prior to DO_KVM.
> >>>
> >>> We also need to update the documentation in reg.h to reflect the fact
> >>> that we don't use SPRG4-7 anymore on e500.
> >> I would personally prefer if we claim SPRG3R as unsupported on e500v2
> >> until we find someone who actually uses it. There's a good chance we'd
> >> start jumping through a lot of hoops and reduce overall performance for
> >> no real-world gain today.
> > The same problem applies to e500mc.
> 
> There we have SPRN_GSPRG3, no?

Oh, right.

Since it's only a problem for PR-mode, it can be fixed without needing
to avoid SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only
need to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
ffe129ecd79779221fdb03305049ec8b5a8beb0f).

And if we decide it's not worthwhile and don't revert that commit, we
should at least remove the comment that "Under KVM, the host SPRG1 is
used to point to the current VCPU data structure"...

-Scott

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogRnJpZGF5LCBKdWx5IDE4LCAyMDE0IDY6MTkgQU0NCj4gVG86IEFsZXhhbmRl
ciBHcmFmDQo+IENjOiBCaHVzaGFuIEJoYXJhdC1SNjU3Nzc7IGt2bS1wcGNAdmdlci5rZXJuZWwu
b3JnOyBrdm1Admdlci5rZXJuZWwub3JnOyBZb2Rlcg0KPiBTdHVhcnQtQjA4MjQ4DQo+IFN1Ympl
Y3Q6IFJlOiBbUEFUQ0hdIGt2bTogcHBjOiBib29rZTogUmVzdG9yZSBTUFJHMyB3aGVuIGVudGVy
aW5nIGd1ZXN0DQo+IA0KPiBPbiBGcmksIDIwMTQtMDctMTggYXQgMDI6MzcgKzAyMDAsIEFsZXhh
bmRlciBHcmFmIHdyb3RlOg0KPiA+IE9uIDE4LjA3LjE0IDAyOjM2LCBTY290dCBXb29kIHdyb3Rl
Og0KPiA+ID4gT24gRnJpLCAyMDE0LTA3LTE4IGF0IDAyOjMzICswMjAwLCBBbGV4YW5kZXIgR3Jh
ZiB3cm90ZToNCj4gPiA+PiBPbiAxOC4wNy4xNCAwMjoyOCwgU2NvdHQgV29vZCB3cm90ZToNCj4g
PiA+Pj4gT24gVGh1LCAyMDE0LTA3LTE3IGF0IDE4OjI5ICswMjAwLCBBbGV4YW5kZXIgR3JhZiB3
cm90ZToNCj4gPiA+Pj4+IE9uIDE3LjA3LjE0IDE4OjI3LCBBbGV4YW5kZXIgR3JhZiB3cm90ZToN
Cj4gPiA+Pj4+PiBPbiAxNy4wNy4xNCAxODoyNCwgQmhhcmF0LkJodXNoYW5AZnJlZXNjYWxlLmNv
bSB3cm90ZToNCj4gPiA+Pj4+Pj4+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPj4+
Pj4+PiBGcm9tOiBBbGV4YW5kZXIgR3JhZiBbbWFpbHRvOmFncmFmQHN1c2UuZGVdDQo+ID4gPj4+
Pj4+PiBTZW50OiBUaHVyc2RheSwgSnVseSAxNywgMjAxNCA5OjQxIFBNDQo+ID4gPj4+Pj4+PiBU
bzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3OyBrdm0tcHBjQHZnZXIua2VybmVsLm9yZw0KPiA+ID4+
Pj4+Pj4gQ2M6IGt2bUB2Z2VyLmtlcm5lbC5vcmc7IFdvb2QgU2NvdHQtQjA3NDIxOyBZb2Rlcg0K
PiA+ID4+Pj4+Pj4gU3R1YXJ0LUIwODI0OA0KPiA+ID4+Pj4+Pj4gU3ViamVjdDogUmU6IFtQQVRD
SF0ga3ZtOiBwcGM6IGJvb2tlOiBSZXN0b3JlIFNQUkczIHdoZW4NCj4gPiA+Pj4+Pj4+IGVudGVy
aW5nIGd1ZXN0DQo+ID4gPj4+Pj4+Pg0KPiA+ID4+Pj4+Pj4NCj4gPiA+Pj4+Pj4+IE9uIDE2LjA3
LjE0IDA4OjAyLCBCaGFyYXQgQmh1c2hhbiB3cm90ZToNCj4gPiA+Pj4+Pj4+PiBTUFJHMyBpcyBn
dWVzdCBhY2Nlc3NpYmxlIGFuZCBTUFJHMyBjYW4gYmUgY2xvYmJlcmVkIGJ5IGhvc3QNCj4gPiA+
Pj4+Pj4+PiBvciBhbm90aGVyIGd1ZXN0LCBTbyB0aGlzIG5lZWQgdG8gYmUgcmVzdG9yZWQgd2hl
biBsb2FkaW5nIGd1ZXN0DQo+IHN0YXRlLg0KPiA+ID4+PiBTUFJHMyBpcyBub3QgZ3Vlc3Qgd3Jp
dGVhYmxlLiAgV2Ugc2hvdWxkIGJlIGRvaW5nIHRoaXMgc28gdGhhdA0KPiA+ID4+PiBndWVzdCBy
ZWFkcyBvZiBTUFJHMyB0aHJvdWdoIHRoZSBhbHRlcm5hdGl2ZSByZWFkLW9ubHkgU1BSIHdvcmss
DQo+ID4gPj4+IG5vdCBiZWNhdXNlDQo+ID4gPj4+ICJTUFJHMyBjYW4gYmUgY2xvYmJlcmVkIGJ5
IGhvc3Qgb3IgYW5vdGhlciBndWVzdCIuDQo+ID4gPj4+DQo+ID4gPj4+Pj4+Pj4gU2lnbmVkLW9m
Zi1ieTogQmhhcmF0IEJodXNoYW4gPEJoYXJhdC5CaHVzaGFuQGZyZWVzY2FsZS5jb20+DQo+ID4g
Pj4+Pj4+Pj4gLS0tDQo+ID4gPj4+Pj4+Pj4gICAgICBhcmNoL3Bvd2VycGMva3ZtL2Jvb2tlX2lu
dGVycnVwdHMuUyB8IDIgKysNCj4gPiA+Pj4+Pj4+PiAgICAgIDEgZmlsZSBjaGFuZ2VkLCAyIGlu
c2VydGlvbnMoKykNCj4gPiA+Pj4+Pj4+Pg0KPiA+ID4+Pj4+Pj4+IGRpZmYgLS1naXQgYS9hcmNo
L3Bvd2VycGMva3ZtL2Jvb2tlX2ludGVycnVwdHMuUw0KPiA+ID4+Pj4+Pj4+IGIvYXJjaC9wb3dl
cnBjL2t2bS9ib29rZV9pbnRlcnJ1cHRzLlMNCj4gPiA+Pj4+Pj4+PiBpbmRleCAyYzZkZWI1ZWYu
LjBkMzQwM2YgMTAwNjQ0DQo+ID4gPj4+Pj4+Pj4gLS0tIGEvYXJjaC9wb3dlcnBjL2t2bS9ib29r
ZV9pbnRlcnJ1cHRzLlMNCj4gPiA+Pj4+Pj4+PiArKysgYi9hcmNoL3Bvd2VycGMva3ZtL2Jvb2tl
X2ludGVycnVwdHMuUw0KPiA+ID4+Pj4+Pj4+IEBAIC00NTksNiArNDU5LDggQEAgbGlnaHR3ZWln
aHRfZXhpdDoNCj4gPiA+Pj4+Pj4+PiAgICAgICAgICAgKiB3cml0dGVuIGRpcmVjdGx5IHRvIHRo
ZSBzaGFyZWQgYXJlYSwgc28gd2UNCj4gPiA+Pj4+Pj4+PiAgICAgICAgICAgKiBuZWVkIHRvIHJl
bG9hZCB0aGVtIGhlcmUgd2l0aCB0aGUgZ3Vlc3QncyB2YWx1ZXMuDQo+ID4gPj4+Pj4+Pj4gICAg
ICAgICAgICovDQo+ID4gPj4+Pj4+Pj4gKyAgICBQUENfTEQocjMsIFZDUFVfU0hBUkVEX1NQUkcz
LCByNSkNCj4gPiA+Pj4+Pj4+PiArICAgIG10c3ByICAgIFNQUk5fU1BSRzMsIHIzDQo+ID4gPj4+
Pj4+PiBXZSBhbHNvIG5lZWQgdG8gcmVzdG9yZSBpdCB3aGVuIHJlc3VtaW5nIHRoZSBob3N0LCBu
bz8NCj4gPiA+Pj4+Pj4gSSBkbyBub3QgdGhpbmsgaG9zdCBleHBlY3Qgc29tZSBtZWFuaW5nZnVs
IHZhbHVlIHdoZW4gcmV0dXJuaW5nDQo+ID4gPj4+Pj4+IGZyb20gZ3Vlc3QsIHNhbWUgdHJ1ZSBm
b3IgU1BSRzQtNy4NCj4gPiA+Pj4+Pj4gU28gdGhlcmUgc2VlbXMgbm8gcmVhc29uIHRvIHNhdmUg
aG9zdCB2YWx1ZXMgYW5kIHJlc3RvcmUgdGhlbS4NCj4gPiA+Pj4gTGludXggbm8gbG9uZ2VyIHVz
ZXMgU1BSRzQtNyBmb3IgaXRzZWxmLiAgVGhhdCBpcyBub3QgdHJ1ZSBvZg0KPiA+ID4+PiBTUFJH
MywgYXMgQWxleCBwb2ludHMgb3V0Lg0KPiA+ID4+Pg0KPiA+ID4+Pj4+IEhtbSAtIGFyY2gvcG93
ZXJwYy9pbmNsdWRlL2FzbS9yZWcuaCBzYXlzOg0KPiA+ID4+Pj4+DQo+ID4gPj4+Pj4gICAgKiBB
bGwgMzItYml0Og0KPiA+ID4+Pj4+ICAgICogICAgICAtIFNQUkczIGN1cnJlbnQgdGhyZWFkX2lu
Zm8gcG9pbnRlcg0KPiA+ID4+Pj4+ICAgICogICAgICAgICh2aXJ0dWFsIG9uIEJvb2tFLCBwaHlz
aWNhbCBvbiBvdGhlcnMpDQo+ID4gPj4+Pj4NCj4gPiA+Pj4+PiBidXQgSSBjYW4gaW5kZWVkIGZp
bmQgbm8gdHJhY2Ugb2YgdXNhZ2UgYW55d2hlcmUuIFRoaXMgYXQgbGVhc3QNCj4gPiA+Pj4+PiBu
ZWVkcyB0byBnbyBpbnRvIHRoZSBwYXRjaCBkZXNjcmlwdGlvbi4NCj4gPiA+Pj4+IEJhaCAtIGl0
IG9idmlvdXNseSBpcyB1c2VkLiBJdCdzIFNQUk5fU1BSR19USFJFQUQuIEFuZCBpdCdzIHNvDQo+
ID4gPj4+PiBpbmNyZWRpYmx5IGltcG9ydGFudCB0aGF0IEkgaGF2ZSBubyBpZGVhIGhvdyB3ZSBj
b3VsZCBwb3NzaWJseQ0KPiA+ID4+Pj4gcnVuIHdpdGhvdXQgc3dpdGNoaW5nIHRoZSBob3N0IHZh
bHVlIGJhY2sgaW4gdmVyeSBlYXJseS4gQW5kIGV2ZW4NCj4gPiA+Pj4+IHRoZW4gb3VyIGludGVy
cnVwdCBoYW5kbGVycyB3b3VsZG4ndCB3b3JrIGFueW1vcmUuDQo+ID4gPj4+Pg0KPiA+ID4+Pj4g
VGhpcyBpcyBtb3JlIGNvbXBsaWNhdGVkIDopLg0KPiA+ID4+PiBUbyBtYWtlIHRoaXMgd29yayB3
ZSBuZWVkIHRvIGF2b2lkIFNQUkczIGFzIHdlbGwsIG9yIGF0IGxlYXN0DQo+ID4gPj4+IGF2b2lk
IHVzaW5nIGl0IGZvciBzb21ldGhpbmcgbmVlZGVkIHByaW9yIHRvIERPX0tWTS4NCj4gPiA+Pj4N
Cj4gPiA+Pj4gV2UgYWxzbyBuZWVkIHRvIHVwZGF0ZSB0aGUgZG9jdW1lbnRhdGlvbiBpbiByZWcu
aCB0byByZWZsZWN0IHRoZQ0KPiA+ID4+PiBmYWN0IHRoYXQgd2UgZG9uJ3QgdXNlIFNQUkc0LTcg
YW55bW9yZSBvbiBlNTAwLg0KPiA+ID4+IEkgd291bGQgcGVyc29uYWxseSBwcmVmZXIgaWYgd2Ug
Y2xhaW0gU1BSRzNSIGFzIHVuc3VwcG9ydGVkIG9uDQo+ID4gPj4gZTUwMHYyIHVudGlsIHdlIGZp
bmQgc29tZW9uZSB3aG8gYWN0dWFsbHkgdXNlcyBpdC4gVGhlcmUncyBhIGdvb2QNCj4gPiA+PiBj
aGFuY2Ugd2UnZCBzdGFydCBqdW1waW5nIHRocm91Z2ggYSBsb3Qgb2YgaG9vcHMgYW5kIHJlZHVj
ZSBvdmVyYWxsDQo+ID4gPj4gcGVyZm9ybWFuY2UgZm9yIG5vIHJlYWwtd29ybGQgZ2FpbiB0b2Rh
eS4NCj4gPiA+IFRoZSBzYW1lIHByb2JsZW0gYXBwbGllcyB0byBlNTAwbWMuDQoNCldlIGhhdmUg
dHdvIFNQUkczIHJlZ2lzdGVycw0KIDEpIFNQUk5fU1BSRzMgICAgICAgICAgLS0gQWxsIHJlYWQv
d3JpdGUgYWNjZXNzIHRvIHRoaXMgcmVnaXN0ZXIgaW4gZ3Vlc3Qgd2lsbCB0cmFwIGFuZCBlbXVs
YXRlLCBTbyBubyBuZWVkIHRvIHNhdmUvcmVzdG9yZS4NCiAyKSBTUFJOX1NQUkczUiAgICAgICAg
IC0tIFRoaXMgaXMgZ3Vlc3QgcmVhZCBvbmx5IGFuZCBXZSBkbyBub3Qgc2VlIGFueSB1c2VyIG9m
IHRoaXMgcmVnaXN0ZXIsIHNvIGNhbiBsZWF2ZSB0aGlzIGZvciBub3cNCg0KPiA+DQo+ID4gVGhl
cmUgd2UgaGF2ZSBTUFJOX0dTUFJHMywgbm8/DQo+IA0KPiBPaCwgcmlnaHQuDQo+IA0KPiBTaW5j
ZSBpdCdzIG9ubHkgYSBwcm9ibGVtIGZvciBQUi1tb2RlLCBpdCBjYW4gYmUgZml4ZWQgd2l0aG91
dCBuZWVkaW5nIHRvIGF2b2lkDQo+IFNQUkczIGVudGlyZWx5LCBzaW5jZSBQUi1tb2RlIGRvZXNu
J3QgdXNlIERPX0tWTS4gIFdlJ2Qgb25seSBuZWVkIHRvIGF2b2lkIHVzaW5nDQo+IFNQUkdfVEhS
RUFEIGluIF9fS1ZNX0hBTkRMRVIgKGkuZS4gcmV2ZXJ0IGNvbW1pdA0KPiBmZmUxMjllY2Q3OTc3
OTIyMWZkYjAzMzA1MDQ5ZWM4YjVhOGJlYjBmKS4NCg0KRGlkIG5vdCBnZXQgd2h5IHVzaW5nIFNQ
UkdfVEhSRUFEIGhlcmUgaXMgYSBwcm9ibGVtPyBJcyBub3QgdGhpcyB3aWxsIGFsd2F5cyBhY2Nl
c3MgaG9zdCBTUFJHMyBhbmQgZ3Vlc3QgcmVhZCB3cml0ZSB3aWxsIGFsd2F5cyB0cmFwIChtYWlu
dGFpbmVkIGluIHZjcHUtPmFyY2gtPnNoYXJlZC0+cmVnLT5zcHJnMykNCg0KWWVzIHdlIGFyZSBu
b3QgY29uc2lzdGVudCB3aXRoICJLVk0sIHRoZSBob3N0IFNQUkcxIGlzIHVzZWQgdG8gcG9pbnQg
dG8gdGhlIGN1cnJlbnQgVkNQVSBkYXRhIHN0cnVjdHVyZSIgLCB3aGljaCBuZWVkIHRvIGJlIGNv
cnJlY3RlZA0KDQpUaGFua3MNCi1CaGFyYXQNCg0KPiANCj4gQW5kIGlmIHdlIGRlY2lkZSBpdCdz
IG5vdCB3b3J0aHdoaWxlIGFuZCBkb24ndCByZXZlcnQgdGhhdCBjb21taXQsIHdlIHNob3VsZCBh
dA0KPiBsZWFzdCByZW1vdmUgdGhlIGNvbW1lbnQgdGhhdCAiVW5kZXIgS1ZNLCB0aGUgaG9zdCBT
UFJHMSBpcyB1c2VkIHRvIHBvaW50IHRvIHRoZQ0KPiBjdXJyZW50IFZDUFUgZGF0YSBzdHJ1Y3R1
cmUiLi4uDQo+IA0KPiAtU2NvdHQNCj4gDQoNCg=

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, July 18, 2014 6:19 AM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder
> Stuart-B08248
> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> 
> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
> > On 18.07.14 02:36, Scott Wood wrote:
> > > On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> > >> On 18.07.14 02:28, Scott Wood wrote:
> > >>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> > >>>> On 17.07.14 18:27, Alexander Graf wrote:
> > >>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> > >>>>>>> -----Original Message-----
> > >>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> > >>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
> > >>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> > >>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder
> > >>>>>>> Stuart-B08248
> > >>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when
> > >>>>>>> entering guest
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> > >>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host
> > >>>>>>>> or another guest, So this need to be restored when loading guest
> state.
> > >>> SPRG3 is not guest writeable.  We should be doing this so that
> > >>> guest reads of SPRG3 through the alternative read-only SPR work,
> > >>> not because
> > >>> "SPRG3 can be clobbered by host or another guest".
> > >>>
> > >>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> > >>>>>>>> ---
> > >>>>>>>>      arch/powerpc/kvm/booke_interrupts.S | 2 ++
> > >>>>>>>>      1 file changed, 2 insertions(+)
> > >>>>>>>>
> > >>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>> index 2c6deb5ef..0d3403f 100644
> > >>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> > >>>>>>>>           * written directly to the shared area, so we
> > >>>>>>>>           * need to reload them here with the guest's values.
> > >>>>>>>>           */
> > >>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> > >>>>>>>> +    mtspr    SPRN_SPRG3, r3
> > >>>>>>> We also need to restore it when resuming the host, no?
> > >>>>>> I do not think host expect some meaningful value when returning
> > >>>>>> from guest, same true for SPRG4-7.
> > >>>>>> So there seems no reason to save host values and restore them.
> > >>> Linux no longer uses SPRG4-7 for itself.  That is not true of
> > >>> SPRG3, as Alex points out.
> > >>>
> > >>>>> Hmm - arch/powerpc/include/asm/reg.h says:
> > >>>>>
> > >>>>>    * All 32-bit:
> > >>>>>    *      - SPRG3 current thread_info pointer
> > >>>>>    *        (virtual on BookE, physical on others)
> > >>>>>
> > >>>>> but I can indeed find no trace of usage anywhere. This at least
> > >>>>> needs to go into the patch description.
> > >>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> > >>>> incredibly important that I have no idea how we could possibly
> > >>>> run without switching the host value back in very early. And even
> > >>>> then our interrupt handlers wouldn't work anymore.
> > >>>>
> > >>>> This is more complicated :).
> > >>> To make this work we need to avoid SPRG3 as well, or at least
> > >>> avoid using it for something needed prior to DO_KVM.
> > >>>
> > >>> We also need to update the documentation in reg.h to reflect the
> > >>> fact that we don't use SPRG4-7 anymore on e500.
> > >> I would personally prefer if we claim SPRG3R as unsupported on
> > >> e500v2 until we find someone who actually uses it. There's a good
> > >> chance we'd start jumping through a lot of hoops and reduce overall
> > >> performance for no real-world gain today.
> > > The same problem applies to e500mc.

We have two SPRG3 registers
 1) SPRN_SPRG3          -- All read/write access to this register in guest will trap and emulate, So no need to save/restore.
 2) SPRN_SPRG3R         -- This is guest read only and We do not see any user of this register, so can leave this for now

> >
> > There we have SPRN_GSPRG3, no?
> 
> Oh, right.
> 
> Since it's only a problem for PR-mode, it can be fixed without needing to avoid
> SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only need to avoid using
> SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
> ffe129ecd79779221fdb03305049ec8b5a8beb0f).

Did not get why using SPRG_THREAD here is a problem? Is not this will always access host SPRG3 and guest read write will always trap (maintained in vcpu->arch->shared->reg->sprg3)

Yes we are not consistent with "KVM, the host SPRG1 is used to point to the current VCPU data structure" , which need to be corrected

Thanks
-Bharat

> 
> And if we decide it's not worthwhile and don't revert that commit, we should at
> least remove the comment that "Under KVM, the host SPRG1 is used to point to the
> current VCPU data structure"...
> 
> -Scott
> 

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 11:57, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Friday, July 18, 2014 6:19 AM
>> To: Alexander Graf
>> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder
>> Stuart-B08248
>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>
>> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
>>> On 18.07.14 02:36, Scott Wood wrote:
>>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
>>>>> On 18.07.14 02:28, Scott Wood wrote:
>>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>>>>>>> On 17.07.14 18:27, Alexander Graf wrote:
>>>>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder
>>>>>>>>>> Stuart-B08248
>>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when
>>>>>>>>>> entering guest
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host
>>>>>>>>>>> or another guest, So this need to be restored when loading guest
>> state.
>>>>>> SPRG3 is not guest writeable.  We should be doing this so that
>>>>>> guest reads of SPRG3 through the alternative read-only SPR work,
>>>>>> not because
>>>>>> "SPRG3 can be clobbered by host or another guest".
>>>>>>
>>>>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>>>>>>       1 file changed, 2 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>>>>>>            * written directly to the shared area, so we
>>>>>>>>>>>            * need to reload them here with the guest's values.
>>>>>>>>>>>            */
>>>>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>>>>>>> We also need to restore it when resuming the host, no?
>>>>>>>>> I do not think host expect some meaningful value when returning
>>>>>>>>> from guest, same true for SPRG4-7.
>>>>>>>>> So there seems no reason to save host values and restore them.
>>>>>> Linux no longer uses SPRG4-7 for itself.  That is not true of
>>>>>> SPRG3, as Alex points out.
>>>>>>
>>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>>>>>>
>>>>>>>>     * All 32-bit:
>>>>>>>>     *      - SPRG3 current thread_info pointer
>>>>>>>>     *        (virtual on BookE, physical on others)
>>>>>>>>
>>>>>>>> but I can indeed find no trace of usage anywhere. This at least
>>>>>>>> needs to go into the patch description.
>>>>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>>>>>>> incredibly important that I have no idea how we could possibly
>>>>>>> run without switching the host value back in very early. And even
>>>>>>> then our interrupt handlers wouldn't work anymore.
>>>>>>>
>>>>>>> This is more complicated :).
>>>>>> To make this work we need to avoid SPRG3 as well, or at least
>>>>>> avoid using it for something needed prior to DO_KVM.
>>>>>>
>>>>>> We also need to update the documentation in reg.h to reflect the
>>>>>> fact that we don't use SPRG4-7 anymore on e500.
>>>>> I would personally prefer if we claim SPRG3R as unsupported on
>>>>> e500v2 until we find someone who actually uses it. There's a good
>>>>> chance we'd start jumping through a lot of hoops and reduce overall
>>>>> performance for no real-world gain today.
>>>> The same problem applies to e500mc.
> We have two SPRG3 registers
>   1) SPRN_SPRG3          -- All read/write access to this register in guest will trap and emulate, So no need to save/restore.
>   2) SPRN_SPRG3R         -- This is guest read only and We do not see any user of this register, so can leave this for now
>
>>> There we have SPRN_GSPRG3, no?
>> Oh, right.
>>
>> Since it's only a problem for PR-mode, it can be fixed without needing to avoid
>> SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only need to avoid using
>> SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
>> ffe129ecd79779221fdb03305049ec8b5a8beb0f).
> Did not get why using SPRG_THREAD here is a problem? Is not this will always access host SPRG3 and guest read write will always trap (maintained in vcpu->arch->shared->reg->sprg3)

Guest reads via SPRG3R will access the real SPRG3 register. Guest reads 
via SPRG3 will trap :).


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 11:57, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Friday, July 18, 2014 6:19 AM
>> To: Alexander Graf
>> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder
>> Stuart-B08248
>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>
>> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
>>> On 18.07.14 02:36, Scott Wood wrote:
>>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
>>>>> On 18.07.14 02:28, Scott Wood wrote:
>>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>>>>>>> On 17.07.14 18:27, Alexander Graf wrote:
>>>>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder
>>>>>>>>>> Stuart-B08248
>>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when
>>>>>>>>>> entering guest
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host
>>>>>>>>>>> or another guest, So this need to be restored when loading guest
>> state.
>>>>>> SPRG3 is not guest writeable.  We should be doing this so that
>>>>>> guest reads of SPRG3 through the alternative read-only SPR work,
>>>>>> not because
>>>>>> "SPRG3 can be clobbered by host or another guest".
>>>>>>
>>>>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>>>>>>       1 file changed, 2 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>>>>>>            * written directly to the shared area, so we
>>>>>>>>>>>            * need to reload them here with the guest's values.
>>>>>>>>>>>            */
>>>>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>>>>>>> We also need to restore it when resuming the host, no?
>>>>>>>>> I do not think host expect some meaningful value when returning
>>>>>>>>> from guest, same true for SPRG4-7.
>>>>>>>>> So there seems no reason to save host values and restore them.
>>>>>> Linux no longer uses SPRG4-7 for itself.  That is not true of
>>>>>> SPRG3, as Alex points out.
>>>>>>
>>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>>>>>>
>>>>>>>>     * All 32-bit:
>>>>>>>>     *      - SPRG3 current thread_info pointer
>>>>>>>>     *        (virtual on BookE, physical on others)
>>>>>>>>
>>>>>>>> but I can indeed find no trace of usage anywhere. This at least
>>>>>>>> needs to go into the patch description.
>>>>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>>>>>>> incredibly important that I have no idea how we could possibly
>>>>>>> run without switching the host value back in very early. And even
>>>>>>> then our interrupt handlers wouldn't work anymore.
>>>>>>>
>>>>>>> This is more complicated :).
>>>>>> To make this work we need to avoid SPRG3 as well, or at least
>>>>>> avoid using it for something needed prior to DO_KVM.
>>>>>>
>>>>>> We also need to update the documentation in reg.h to reflect the
>>>>>> fact that we don't use SPRG4-7 anymore on e500.
>>>>> I would personally prefer if we claim SPRG3R as unsupported on
>>>>> e500v2 until we find someone who actually uses it. There's a good
>>>>> chance we'd start jumping through a lot of hoops and reduce overall
>>>>> performance for no real-world gain today.
>>>> The same problem applies to e500mc.
> We have two SPRG3 registers
>   1) SPRN_SPRG3          -- All read/write access to this register in guest will trap and emulate, So no need to save/restore.
>   2) SPRN_SPRG3R         -- This is guest read only and We do not see any user of this register, so can leave this for now
>
>>> There we have SPRN_GSPRG3, no?
>> Oh, right.
>>
>> Since it's only a problem for PR-mode, it can be fixed without needing to avoid
>> SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only need to avoid using
>> SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
>> ffe129ecd79779221fdb03305049ec8b5a8beb0f).
> Did not get why using SPRG_THREAD here is a problem? Is not this will always access host SPRG3 and guest read write will always trap (maintained in vcpu->arch->shared->reg->sprg3)

Guest reads via SPRG3R will access the real SPRG3 register. Guest reads 
via SPRG3 will trap :).


Alex

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQWxleGFuZGVyIEdyYWYg
W21haWx0bzphZ3JhZkBzdXNlLmRlXQ0KPiBTZW50OiBGcmlkYXksIEp1bHkgMTgsIDIwMTQgNDoy
NSBQTQ0KPiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3OyBXb29kIFNjb3R0LUIwNzQyMQ0KPiBD
Yzoga3ZtLXBwY0B2Z2VyLmtlcm5lbC5vcmc7IGt2bUB2Z2VyLmtlcm5lbC5vcmc7IFlvZGVyIFN0
dWFydC1CMDgyNDgNCj4gU3ViamVjdDogUmU6IFtQQVRDSF0ga3ZtOiBwcGM6IGJvb2tlOiBSZXN0
b3JlIFNQUkczIHdoZW4gZW50ZXJpbmcgZ3Vlc3QNCj4gDQo+IA0KPiBPbiAxOC4wNy4xNCAxMTo1
NywgQmhhcmF0LkJodXNoYW5AZnJlZXNjYWxlLmNvbSB3cm90ZToNCj4gPg0KPiA+PiAtLS0tLU9y
aWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+PiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0KPiA+PiBT
ZW50OiBGcmlkYXksIEp1bHkgMTgsIDIwMTQgNjoxOSBBTQ0KPiA+PiBUbzogQWxleGFuZGVyIEdy
YWYNCj4gPj4gQ2M6IEJodXNoYW4gQmhhcmF0LVI2NTc3Nzsga3ZtLXBwY0B2Z2VyLmtlcm5lbC5v
cmc7DQo+ID4+IGt2bUB2Z2VyLmtlcm5lbC5vcmc7IFlvZGVyDQo+ID4+IFN0dWFydC1CMDgyNDgN
Cj4gPj4gU3ViamVjdDogUmU6IFtQQVRDSF0ga3ZtOiBwcGM6IGJvb2tlOiBSZXN0b3JlIFNQUkcz
IHdoZW4gZW50ZXJpbmcNCj4gPj4gZ3Vlc3QNCj4gPj4NCj4gPj4gT24gRnJpLCAyMDE0LTA3LTE4
IGF0IDAyOjM3ICswMjAwLCBBbGV4YW5kZXIgR3JhZiB3cm90ZToNCj4gPj4+IE9uIDE4LjA3LjE0
IDAyOjM2LCBTY290dCBXb29kIHdyb3RlOg0KPiA+Pj4+IE9uIEZyaSwgMjAxNC0wNy0xOCBhdCAw
MjozMyArMDIwMCwgQWxleGFuZGVyIEdyYWYgd3JvdGU6DQo+ID4+Pj4+IE9uIDE4LjA3LjE0IDAy
OjI4LCBTY290dCBXb29kIHdyb3RlOg0KPiA+Pj4+Pj4gT24gVGh1LCAyMDE0LTA3LTE3IGF0IDE4
OjI5ICswMjAwLCBBbGV4YW5kZXIgR3JhZiB3cm90ZToNCj4gPj4+Pj4+PiBPbiAxNy4wNy4xNCAx
ODoyNywgQWxleGFuZGVyIEdyYWYgd3JvdGU6DQo+ID4+Pj4+Pj4+IE9uIDE3LjA3LjE0IDE4OjI0
LCBCaGFyYXQuQmh1c2hhbkBmcmVlc2NhbGUuY29tIHdyb3RlOg0KPiA+Pj4+Pj4+Pj4+IC0tLS0t
T3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4+Pj4+Pj4+Pj4gRnJvbTogQWxleGFuZGVyIEdyYWYg
W21haWx0bzphZ3JhZkBzdXNlLmRlXQ0KPiA+Pj4+Pj4+Pj4+IFNlbnQ6IFRodXJzZGF5LCBKdWx5
IDE3LCAyMDE0IDk6NDEgUE0NCj4gPj4+Pj4+Pj4+PiBUbzogQmh1c2hhbiBCaGFyYXQtUjY1Nzc3
OyBrdm0tcHBjQHZnZXIua2VybmVsLm9yZw0KPiA+Pj4+Pj4+Pj4+IENjOiBrdm1Admdlci5rZXJu
ZWwub3JnOyBXb29kIFNjb3R0LUIwNzQyMTsgWW9kZXINCj4gPj4+Pj4+Pj4+PiBTdHVhcnQtQjA4
MjQ4DQo+ID4+Pj4+Pj4+Pj4gU3ViamVjdDogUmU6IFtQQVRDSF0ga3ZtOiBwcGM6IGJvb2tlOiBS
ZXN0b3JlIFNQUkczIHdoZW4NCj4gPj4+Pj4+Pj4+PiBlbnRlcmluZyBndWVzdA0KPiA+Pj4+Pj4+
Pj4+DQo+ID4+Pj4+Pj4+Pj4NCj4gPj4+Pj4+Pj4+PiBPbiAxNi4wNy4xNCAwODowMiwgQmhhcmF0
IEJodXNoYW4gd3JvdGU6DQo+ID4+Pj4+Pj4+Pj4+IFNQUkczIGlzIGd1ZXN0IGFjY2Vzc2libGUg
YW5kIFNQUkczIGNhbiBiZSBjbG9iYmVyZWQgYnkgaG9zdA0KPiA+Pj4+Pj4+Pj4+PiBvciBhbm90
aGVyIGd1ZXN0LCBTbyB0aGlzIG5lZWQgdG8gYmUgcmVzdG9yZWQgd2hlbiBsb2FkaW5nDQo+ID4+
Pj4+Pj4+Pj4+IGd1ZXN0DQo+ID4+IHN0YXRlLg0KPiA+Pj4+Pj4gU1BSRzMgaXMgbm90IGd1ZXN0
IHdyaXRlYWJsZS4gIFdlIHNob3VsZCBiZSBkb2luZyB0aGlzIHNvIHRoYXQNCj4gPj4+Pj4+IGd1
ZXN0IHJlYWRzIG9mIFNQUkczIHRocm91Z2ggdGhlIGFsdGVybmF0aXZlIHJlYWQtb25seSBTUFIg
d29yaywNCj4gPj4+Pj4+IG5vdCBiZWNhdXNlDQo+ID4+Pj4+PiAiU1BSRzMgY2FuIGJlIGNsb2Ji
ZXJlZCBieSBob3N0IG9yIGFub3RoZXIgZ3Vlc3QiLg0KPiA+Pj4+Pj4NCj4gPj4+Pj4+Pj4+Pj4g
U2lnbmVkLW9mZi1ieTogQmhhcmF0IEJodXNoYW4gPEJoYXJhdC5CaHVzaGFuQGZyZWVzY2FsZS5j
b20+DQo+ID4+Pj4+Pj4+Pj4+IC0tLQ0KPiA+Pj4+Pj4+Pj4+PiAgICAgICBhcmNoL3Bvd2VycGMv
a3ZtL2Jvb2tlX2ludGVycnVwdHMuUyB8IDIgKysNCj4gPj4+Pj4+Pj4+Pj4gICAgICAgMSBmaWxl
IGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKQ0KPiA+Pj4+Pj4+Pj4+Pg0KPiA+Pj4+Pj4+Pj4+PiBk
aWZmIC0tZ2l0IGEvYXJjaC9wb3dlcnBjL2t2bS9ib29rZV9pbnRlcnJ1cHRzLlMNCj4gPj4+Pj4+
Pj4+Pj4gYi9hcmNoL3Bvd2VycGMva3ZtL2Jvb2tlX2ludGVycnVwdHMuUw0KPiA+Pj4+Pj4+Pj4+
PiBpbmRleCAyYzZkZWI1ZWYuLjBkMzQwM2YgMTAwNjQ0DQo+ID4+Pj4+Pj4+Pj4+IC0tLSBhL2Fy
Y2gvcG93ZXJwYy9rdm0vYm9va2VfaW50ZXJydXB0cy5TDQo+ID4+Pj4+Pj4+Pj4+ICsrKyBiL2Fy
Y2gvcG93ZXJwYy9rdm0vYm9va2VfaW50ZXJydXB0cy5TDQo+ID4+Pj4+Pj4+Pj4+IEBAIC00NTks
NiArNDU5LDggQEAgbGlnaHR3ZWlnaHRfZXhpdDoNCj4gPj4+Pj4+Pj4+Pj4gICAgICAgICAgICAq
IHdyaXR0ZW4gZGlyZWN0bHkgdG8gdGhlIHNoYXJlZCBhcmVhLCBzbyB3ZQ0KPiA+Pj4+Pj4+Pj4+
PiAgICAgICAgICAgICogbmVlZCB0byByZWxvYWQgdGhlbSBoZXJlIHdpdGggdGhlIGd1ZXN0J3Mg
dmFsdWVzLg0KPiA+Pj4+Pj4+Pj4+PiAgICAgICAgICAgICovDQo+ID4+Pj4+Pj4+Pj4+ICsgICAg
UFBDX0xEKHIzLCBWQ1BVX1NIQVJFRF9TUFJHMywgcjUpDQo+ID4+Pj4+Pj4+Pj4+ICsgICAgbXRz
cHIgICAgU1BSTl9TUFJHMywgcjMNCj4gPj4+Pj4+Pj4+PiBXZSBhbHNvIG5lZWQgdG8gcmVzdG9y
ZSBpdCB3aGVuIHJlc3VtaW5nIHRoZSBob3N0LCBubz8NCj4gPj4+Pj4+Pj4+IEkgZG8gbm90IHRo
aW5rIGhvc3QgZXhwZWN0IHNvbWUgbWVhbmluZ2Z1bCB2YWx1ZSB3aGVuDQo+ID4+Pj4+Pj4+PiBy
ZXR1cm5pbmcgZnJvbSBndWVzdCwgc2FtZSB0cnVlIGZvciBTUFJHNC03Lg0KPiA+Pj4+Pj4+Pj4g
U28gdGhlcmUgc2VlbXMgbm8gcmVhc29uIHRvIHNhdmUgaG9zdCB2YWx1ZXMgYW5kIHJlc3RvcmUg
dGhlbS4NCj4gPj4+Pj4+IExpbnV4IG5vIGxvbmdlciB1c2VzIFNQUkc0LTcgZm9yIGl0c2VsZi4g
IFRoYXQgaXMgbm90IHRydWUgb2YNCj4gPj4+Pj4+IFNQUkczLCBhcyBBbGV4IHBvaW50cyBvdXQu
DQo+ID4+Pj4+Pg0KPiA+Pj4+Pj4+PiBIbW0gLSBhcmNoL3Bvd2VycGMvaW5jbHVkZS9hc20vcmVn
Lmggc2F5czoNCj4gPj4+Pj4+Pj4NCj4gPj4+Pj4+Pj4gICAgICogQWxsIDMyLWJpdDoNCj4gPj4+
Pj4+Pj4gICAgICogICAgICAtIFNQUkczIGN1cnJlbnQgdGhyZWFkX2luZm8gcG9pbnRlcg0KPiA+
Pj4+Pj4+PiAgICAgKiAgICAgICAgKHZpcnR1YWwgb24gQm9va0UsIHBoeXNpY2FsIG9uIG90aGVy
cykNCj4gPj4+Pj4+Pj4NCj4gPj4+Pj4+Pj4gYnV0IEkgY2FuIGluZGVlZCBmaW5kIG5vIHRyYWNl
IG9mIHVzYWdlIGFueXdoZXJlLiBUaGlzIGF0IGxlYXN0DQo+ID4+Pj4+Pj4+IG5lZWRzIHRvIGdv
IGludG8gdGhlIHBhdGNoIGRlc2NyaXB0aW9uLg0KPiA+Pj4+Pj4+IEJhaCAtIGl0IG9idmlvdXNs
eSBpcyB1c2VkLiBJdCdzIFNQUk5fU1BSR19USFJFQUQuIEFuZCBpdCdzIHNvDQo+ID4+Pj4+Pj4g
aW5jcmVkaWJseSBpbXBvcnRhbnQgdGhhdCBJIGhhdmUgbm8gaWRlYSBob3cgd2UgY291bGQgcG9z
c2libHkNCj4gPj4+Pj4+PiBydW4gd2l0aG91dCBzd2l0Y2hpbmcgdGhlIGhvc3QgdmFsdWUgYmFj
ayBpbiB2ZXJ5IGVhcmx5LiBBbmQNCj4gPj4+Pj4+PiBldmVuIHRoZW4gb3VyIGludGVycnVwdCBo
YW5kbGVycyB3b3VsZG4ndCB3b3JrIGFueW1vcmUuDQo+ID4+Pj4+Pj4NCj4gPj4+Pj4+PiBUaGlz
IGlzIG1vcmUgY29tcGxpY2F0ZWQgOikuDQo+ID4+Pj4+PiBUbyBtYWtlIHRoaXMgd29yayB3ZSBu
ZWVkIHRvIGF2b2lkIFNQUkczIGFzIHdlbGwsIG9yIGF0IGxlYXN0DQo+ID4+Pj4+PiBhdm9pZCB1
c2luZyBpdCBmb3Igc29tZXRoaW5nIG5lZWRlZCBwcmlvciB0byBET19LVk0uDQo+ID4+Pj4+Pg0K
PiA+Pj4+Pj4gV2UgYWxzbyBuZWVkIHRvIHVwZGF0ZSB0aGUgZG9jdW1lbnRhdGlvbiBpbiByZWcu
aCB0byByZWZsZWN0IHRoZQ0KPiA+Pj4+Pj4gZmFjdCB0aGF0IHdlIGRvbid0IHVzZSBTUFJHNC03
IGFueW1vcmUgb24gZTUwMC4NCj4gPj4+Pj4gSSB3b3VsZCBwZXJzb25hbGx5IHByZWZlciBpZiB3
ZSBjbGFpbSBTUFJHM1IgYXMgdW5zdXBwb3J0ZWQgb24NCj4gPj4+Pj4gZTUwMHYyIHVudGlsIHdl
IGZpbmQgc29tZW9uZSB3aG8gYWN0dWFsbHkgdXNlcyBpdC4gVGhlcmUncyBhIGdvb2QNCj4gPj4+
Pj4gY2hhbmNlIHdlJ2Qgc3RhcnQganVtcGluZyB0aHJvdWdoIGEgbG90IG9mIGhvb3BzIGFuZCBy
ZWR1Y2UNCj4gPj4+Pj4gb3ZlcmFsbCBwZXJmb3JtYW5jZSBmb3Igbm8gcmVhbC13b3JsZCBnYWlu
IHRvZGF5Lg0KPiA+Pj4+IFRoZSBzYW1lIHByb2JsZW0gYXBwbGllcyB0byBlNTAwbWMuDQo+ID4g
V2UgaGF2ZSB0d28gU1BSRzMgcmVnaXN0ZXJzDQo+ID4gICAxKSBTUFJOX1NQUkczICAgICAgICAg
IC0tIEFsbCByZWFkL3dyaXRlIGFjY2VzcyB0byB0aGlzIHJlZ2lzdGVyIGluIGd1ZXN0DQo+IHdp
bGwgdHJhcCBhbmQgZW11bGF0ZSwgU28gbm8gbmVlZCB0byBzYXZlL3Jlc3RvcmUuDQo+ID4gICAy
KSBTUFJOX1NQUkczUiAgICAgICAgIC0tIFRoaXMgaXMgZ3Vlc3QgcmVhZCBvbmx5IGFuZCBXZSBk
byBub3Qgc2VlIGFueSB1c2VyDQo+IG9mIHRoaXMgcmVnaXN0ZXIsIHNvIGNhbiBsZWF2ZSB0aGlz
IGZvciBub3cNCj4gPg0KPiA+Pj4gVGhlcmUgd2UgaGF2ZSBTUFJOX0dTUFJHMywgbm8/DQo+ID4+
IE9oLCByaWdodC4NCj4gPj4NCj4gPj4gU2luY2UgaXQncyBvbmx5IGEgcHJvYmxlbSBmb3IgUFIt
bW9kZSwgaXQgY2FuIGJlIGZpeGVkIHdpdGhvdXQNCj4gPj4gbmVlZGluZyB0byBhdm9pZA0KPiA+
PiBTUFJHMyBlbnRpcmVseSwgc2luY2UgUFItbW9kZSBkb2Vzbid0IHVzZSBET19LVk0uICBXZSdk
IG9ubHkgbmVlZCB0bw0KPiA+PiBhdm9pZCB1c2luZyBTUFJHX1RIUkVBRCBpbiBfX0tWTV9IQU5E
TEVSIChpLmUuIHJldmVydCBjb21taXQNCj4gPj4gZmZlMTI5ZWNkNzk3NzkyMjFmZGIwMzMwNTA0
OWVjOGI1YThiZWIwZikuDQo+ID4gRGlkIG5vdCBnZXQgd2h5IHVzaW5nIFNQUkdfVEhSRUFEIGhl
cmUgaXMgYSBwcm9ibGVtPyBJcyBub3QgdGhpcyB3aWxsDQo+ID4gYWx3YXlzIGFjY2VzcyBob3N0
IFNQUkczIGFuZCBndWVzdCByZWFkIHdyaXRlIHdpbGwgYWx3YXlzIHRyYXANCj4gPiAobWFpbnRh
aW5lZCBpbiB2Y3B1LT5hcmNoLT5zaGFyZWQtPnJlZy0+c3ByZzMpDQo+IA0KPiBHdWVzdCByZWFk
cyB2aWEgU1BSRzNSIHdpbGwgYWNjZXNzIHRoZSByZWFsIFNQUkczIHJlZ2lzdGVyLiBHdWVzdCBy
ZWFkcyB2aWENCj4gU1BSRzMgd2lsbCB0cmFwIDopLg0KDQpBZ3JlZSwgQWxzbyB3ZSBkbyBub3Qg
c2VlIExpbnV4IGFzIGd1ZXN0IGlzIGFjY2Vzc2luZyBTUFJHM1IuIEJ1dCB3aGF0IEkgZGlkIG5v
dCBnZXQgd2hhdCB0aGUgbWVudGlvbmVkIHBhdGNoIGhhdmUgdG8gZG8gd2l0aCB0aGF0Pw0KDQoN
Cj4gDQo+IA0KPiBBbGV4DQoNCg=

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Friday, July 18, 2014 4:25 PM
> To: Bhushan Bharat-R65777; Wood Scott-B07421
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-B08248
> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> 
> 
> On 18.07.14 11:57, Bharat.Bhushan@freescale.com wrote:
> >
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Friday, July 18, 2014 6:19 AM
> >> To: Alexander Graf
> >> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> >> kvm@vger.kernel.org; Yoder
> >> Stuart-B08248
> >> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering
> >> guest
> >>
> >> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
> >>> On 18.07.14 02:36, Scott Wood wrote:
> >>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> >>>>> On 18.07.14 02:28, Scott Wood wrote:
> >>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> >>>>>>> On 17.07.14 18:27, Alexander Graf wrote:
> >>>>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
> >>>>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder
> >>>>>>>>>> Stuart-B08248
> >>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when
> >>>>>>>>>> entering guest
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host
> >>>>>>>>>>> or another guest, So this need to be restored when loading
> >>>>>>>>>>> guest
> >> state.
> >>>>>> SPRG3 is not guest writeable.  We should be doing this so that
> >>>>>> guest reads of SPRG3 through the alternative read-only SPR work,
> >>>>>> not because
> >>>>>> "SPRG3 can be clobbered by host or another guest".
> >>>>>>
> >>>>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>       arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>>>>>>>>>       1 file changed, 2 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>>>> index 2c6deb5ef..0d3403f 100644
> >>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>>>>>>>>>            * written directly to the shared area, so we
> >>>>>>>>>>>            * need to reload them here with the guest's values.
> >>>>>>>>>>>            */
> >>>>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>>>>>>>>>> +    mtspr    SPRN_SPRG3, r3
> >>>>>>>>>> We also need to restore it when resuming the host, no?
> >>>>>>>>> I do not think host expect some meaningful value when
> >>>>>>>>> returning from guest, same true for SPRG4-7.
> >>>>>>>>> So there seems no reason to save host values and restore them.
> >>>>>> Linux no longer uses SPRG4-7 for itself.  That is not true of
> >>>>>> SPRG3, as Alex points out.
> >>>>>>
> >>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
> >>>>>>>>
> >>>>>>>>     * All 32-bit:
> >>>>>>>>     *      - SPRG3 current thread_info pointer
> >>>>>>>>     *        (virtual on BookE, physical on others)
> >>>>>>>>
> >>>>>>>> but I can indeed find no trace of usage anywhere. This at least
> >>>>>>>> needs to go into the patch description.
> >>>>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> >>>>>>> incredibly important that I have no idea how we could possibly
> >>>>>>> run without switching the host value back in very early. And
> >>>>>>> even then our interrupt handlers wouldn't work anymore.
> >>>>>>>
> >>>>>>> This is more complicated :).
> >>>>>> To make this work we need to avoid SPRG3 as well, or at least
> >>>>>> avoid using it for something needed prior to DO_KVM.
> >>>>>>
> >>>>>> We also need to update the documentation in reg.h to reflect the
> >>>>>> fact that we don't use SPRG4-7 anymore on e500.
> >>>>> I would personally prefer if we claim SPRG3R as unsupported on
> >>>>> e500v2 until we find someone who actually uses it. There's a good
> >>>>> chance we'd start jumping through a lot of hoops and reduce
> >>>>> overall performance for no real-world gain today.
> >>>> The same problem applies to e500mc.
> > We have two SPRG3 registers
> >   1) SPRN_SPRG3          -- All read/write access to this register in guest
> will trap and emulate, So no need to save/restore.
> >   2) SPRN_SPRG3R         -- This is guest read only and We do not see any user
> of this register, so can leave this for now
> >
> >>> There we have SPRN_GSPRG3, no?
> >> Oh, right.
> >>
> >> Since it's only a problem for PR-mode, it can be fixed without
> >> needing to avoid
> >> SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only need to
> >> avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
> >> ffe129ecd79779221fdb03305049ec8b5a8beb0f).
> > Did not get why using SPRG_THREAD here is a problem? Is not this will
> > always access host SPRG3 and guest read write will always trap
> > (maintained in vcpu->arch->shared->reg->sprg3)
> 
> Guest reads via SPRG3R will access the real SPRG3 register. Guest reads via
> SPRG3 will trap :).

Agree, Also we do not see Linux as guest is accessing SPRG3R. But what I did not get what the mentioned patch have to do with that?


> 
> 
> Alex

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

PiA+DQo+ID4gT24gMTguMDcuMTQgMTE6NTcsIEJoYXJhdC5CaHVzaGFuQGZyZWVzY2FsZS5jb20g
d3JvdGU6DQo+ID4gPg0KPiA+ID4+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPj4g
RnJvbTogV29vZCBTY290dC1CMDc0MjENCj4gPiA+PiBTZW50OiBGcmlkYXksIEp1bHkgMTgsIDIw
MTQgNjoxOSBBTQ0KPiA+ID4+IFRvOiBBbGV4YW5kZXIgR3JhZg0KPiA+ID4+IENjOiBCaHVzaGFu
IEJoYXJhdC1SNjU3Nzc7IGt2bS1wcGNAdmdlci5rZXJuZWwub3JnOw0KPiA+ID4+IGt2bUB2Z2Vy
Lmtlcm5lbC5vcmc7IFlvZGVyDQo+ID4gPj4gU3R1YXJ0LUIwODI0OA0KPiA+ID4+IFN1YmplY3Q6
IFJlOiBbUEFUQ0hdIGt2bTogcHBjOiBib29rZTogUmVzdG9yZSBTUFJHMyB3aGVuIGVudGVyaW5n
DQo+ID4gPj4gZ3Vlc3QNCj4gPiA+Pg0KPiA+ID4+IE9uIEZyaSwgMjAxNC0wNy0xOCBhdCAwMjoz
NyArMDIwMCwgQWxleGFuZGVyIEdyYWYgd3JvdGU6DQo+ID4gPj4+IE9uIDE4LjA3LjE0IDAyOjM2
LCBTY290dCBXb29kIHdyb3RlOg0KPiA+ID4+Pj4gT24gRnJpLCAyMDE0LTA3LTE4IGF0IDAyOjMz
ICswMjAwLCBBbGV4YW5kZXIgR3JhZiB3cm90ZToNCj4gPiA+Pj4+PiBPbiAxOC4wNy4xNCAwMjoy
OCwgU2NvdHQgV29vZCB3cm90ZToNCj4gPiA+Pj4+Pj4gT24gVGh1LCAyMDE0LTA3LTE3IGF0IDE4
OjI5ICswMjAwLCBBbGV4YW5kZXIgR3JhZiB3cm90ZToNCj4gPiA+Pj4+Pj4+IE9uIDE3LjA3LjE0
IDE4OjI3LCBBbGV4YW5kZXIgR3JhZiB3cm90ZToNCj4gPiA+Pj4+Pj4+PiBPbiAxNy4wNy4xNCAx
ODoyNCwgQmhhcmF0LkJodXNoYW5AZnJlZXNjYWxlLmNvbSB3cm90ZToNCj4gPiA+Pj4+Pj4+Pj4+
IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ID4gPj4+Pj4+Pj4+PiBGcm9tOiBBbGV4YW5k
ZXIgR3JhZiBbbWFpbHRvOmFncmFmQHN1c2UuZGVdDQo+ID4gPj4+Pj4+Pj4+PiBTZW50OiBUaHVy
c2RheSwgSnVseSAxNywgMjAxNCA5OjQxIFBNDQo+ID4gPj4+Pj4+Pj4+PiBUbzogQmh1c2hhbiBC
aGFyYXQtUjY1Nzc3OyBrdm0tcHBjQHZnZXIua2VybmVsLm9yZw0KPiA+ID4+Pj4+Pj4+Pj4gQ2M6
IGt2bUB2Z2VyLmtlcm5lbC5vcmc7IFdvb2QgU2NvdHQtQjA3NDIxOyBZb2Rlcg0KPiA+ID4+Pj4+
Pj4+Pj4gU3R1YXJ0LUIwODI0OA0KPiA+ID4+Pj4+Pj4+Pj4gU3ViamVjdDogUmU6IFtQQVRDSF0g
a3ZtOiBwcGM6IGJvb2tlOiBSZXN0b3JlIFNQUkczIHdoZW4NCj4gPiA+Pj4+Pj4+Pj4+IGVudGVy
aW5nIGd1ZXN0DQo+ID4gPj4+Pj4+Pj4+Pg0KPiA+ID4+Pj4+Pj4+Pj4NCj4gPiA+Pj4+Pj4+Pj4+
IE9uIDE2LjA3LjE0IDA4OjAyLCBCaGFyYXQgQmh1c2hhbiB3cm90ZToNCj4gPiA+Pj4+Pj4+Pj4+
PiBTUFJHMyBpcyBndWVzdCBhY2Nlc3NpYmxlIGFuZCBTUFJHMyBjYW4gYmUgY2xvYmJlcmVkIGJ5
DQo+ID4gPj4+Pj4+Pj4+Pj4gaG9zdCBvciBhbm90aGVyIGd1ZXN0LCBTbyB0aGlzIG5lZWQgdG8g
YmUgcmVzdG9yZWQgd2hlbg0KPiA+ID4+Pj4+Pj4+Pj4+IGxvYWRpbmcgZ3Vlc3QNCj4gPiA+PiBz
dGF0ZS4NCj4gPiA+Pj4+Pj4gU1BSRzMgaXMgbm90IGd1ZXN0IHdyaXRlYWJsZS4gIFdlIHNob3Vs
ZCBiZSBkb2luZyB0aGlzIHNvIHRoYXQNCj4gPiA+Pj4+Pj4gZ3Vlc3QgcmVhZHMgb2YgU1BSRzMg
dGhyb3VnaCB0aGUgYWx0ZXJuYXRpdmUgcmVhZC1vbmx5IFNQUg0KPiA+ID4+Pj4+PiB3b3JrLCBu
b3QgYmVjYXVzZQ0KPiA+ID4+Pj4+PiAiU1BSRzMgY2FuIGJlIGNsb2JiZXJlZCBieSBob3N0IG9y
IGFub3RoZXIgZ3Vlc3QiLg0KPiA+ID4+Pj4+Pg0KPiA+ID4+Pj4+Pj4+Pj4+IFNpZ25lZC1vZmYt
Ynk6IEJoYXJhdCBCaHVzaGFuDQo+ID4gPj4+Pj4+Pj4+Pj4gPEJoYXJhdC5CaHVzaGFuQGZyZWVz
Y2FsZS5jb20+DQo+ID4gPj4+Pj4+Pj4+Pj4gLS0tDQo+ID4gPj4+Pj4+Pj4+Pj4gICAgICAgYXJj
aC9wb3dlcnBjL2t2bS9ib29rZV9pbnRlcnJ1cHRzLlMgfCAyICsrDQo+ID4gPj4+Pj4+Pj4+Pj4g
ICAgICAgMSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKQ0KPiA+ID4+Pj4+Pj4+Pj4+DQo+
ID4gPj4+Pj4+Pj4+Pj4gZGlmZiAtLWdpdCBhL2FyY2gvcG93ZXJwYy9rdm0vYm9va2VfaW50ZXJy
dXB0cy5TDQo+ID4gPj4+Pj4+Pj4+Pj4gYi9hcmNoL3Bvd2VycGMva3ZtL2Jvb2tlX2ludGVycnVw
dHMuUw0KPiA+ID4+Pj4+Pj4+Pj4+IGluZGV4IDJjNmRlYjVlZi4uMGQzNDAzZiAxMDA2NDQNCj4g
PiA+Pj4+Pj4+Pj4+PiAtLS0gYS9hcmNoL3Bvd2VycGMva3ZtL2Jvb2tlX2ludGVycnVwdHMuUw0K
PiA+ID4+Pj4+Pj4+Pj4+ICsrKyBiL2FyY2gvcG93ZXJwYy9rdm0vYm9va2VfaW50ZXJydXB0cy5T
DQo+ID4gPj4+Pj4+Pj4+Pj4gQEAgLTQ1OSw2ICs0NTksOCBAQCBsaWdodHdlaWdodF9leGl0Og0K
PiA+ID4+Pj4+Pj4+Pj4+ICAgICAgICAgICAgKiB3cml0dGVuIGRpcmVjdGx5IHRvIHRoZSBzaGFy
ZWQgYXJlYSwgc28gd2UNCj4gPiA+Pj4+Pj4+Pj4+PiAgICAgICAgICAgICogbmVlZCB0byByZWxv
YWQgdGhlbSBoZXJlIHdpdGggdGhlIGd1ZXN0J3MgdmFsdWVzLg0KPiA+ID4+Pj4+Pj4+Pj4+ICAg
ICAgICAgICAgKi8NCj4gPiA+Pj4+Pj4+Pj4+PiArICAgIFBQQ19MRChyMywgVkNQVV9TSEFSRURf
U1BSRzMsIHI1KQ0KPiA+ID4+Pj4+Pj4+Pj4+ICsgICAgbXRzcHIgICAgU1BSTl9TUFJHMywgcjMN
Cj4gPiA+Pj4+Pj4+Pj4+IFdlIGFsc28gbmVlZCB0byByZXN0b3JlIGl0IHdoZW4gcmVzdW1pbmcg
dGhlIGhvc3QsIG5vPw0KPiA+ID4+Pj4+Pj4+PiBJIGRvIG5vdCB0aGluayBob3N0IGV4cGVjdCBz
b21lIG1lYW5pbmdmdWwgdmFsdWUgd2hlbg0KPiA+ID4+Pj4+Pj4+PiByZXR1cm5pbmcgZnJvbSBn
dWVzdCwgc2FtZSB0cnVlIGZvciBTUFJHNC03Lg0KPiA+ID4+Pj4+Pj4+PiBTbyB0aGVyZSBzZWVt
cyBubyByZWFzb24gdG8gc2F2ZSBob3N0IHZhbHVlcyBhbmQgcmVzdG9yZSB0aGVtLg0KPiA+ID4+
Pj4+PiBMaW51eCBubyBsb25nZXIgdXNlcyBTUFJHNC03IGZvciBpdHNlbGYuICBUaGF0IGlzIG5v
dCB0cnVlIG9mDQo+ID4gPj4+Pj4+IFNQUkczLCBhcyBBbGV4IHBvaW50cyBvdXQuDQo+ID4gPj4+
Pj4+DQo+ID4gPj4+Pj4+Pj4gSG1tIC0gYXJjaC9wb3dlcnBjL2luY2x1ZGUvYXNtL3JlZy5oIHNh
eXM6DQo+ID4gPj4+Pj4+Pj4NCj4gPiA+Pj4+Pj4+PiAgICAgKiBBbGwgMzItYml0Og0KPiA+ID4+
Pj4+Pj4+ICAgICAqICAgICAgLSBTUFJHMyBjdXJyZW50IHRocmVhZF9pbmZvIHBvaW50ZXINCj4g
PiA+Pj4+Pj4+PiAgICAgKiAgICAgICAgKHZpcnR1YWwgb24gQm9va0UsIHBoeXNpY2FsIG9uIG90
aGVycykNCj4gPiA+Pj4+Pj4+Pg0KPiA+ID4+Pj4+Pj4+IGJ1dCBJIGNhbiBpbmRlZWQgZmluZCBu
byB0cmFjZSBvZiB1c2FnZSBhbnl3aGVyZS4gVGhpcyBhdA0KPiA+ID4+Pj4+Pj4+IGxlYXN0IG5l
ZWRzIHRvIGdvIGludG8gdGhlIHBhdGNoIGRlc2NyaXB0aW9uLg0KPiA+ID4+Pj4+Pj4gQmFoIC0g
aXQgb2J2aW91c2x5IGlzIHVzZWQuIEl0J3MgU1BSTl9TUFJHX1RIUkVBRC4gQW5kIGl0J3Mgc28N
Cj4gPiA+Pj4+Pj4+IGluY3JlZGlibHkgaW1wb3J0YW50IHRoYXQgSSBoYXZlIG5vIGlkZWEgaG93
IHdlIGNvdWxkIHBvc3NpYmx5DQo+ID4gPj4+Pj4+PiBydW4gd2l0aG91dCBzd2l0Y2hpbmcgdGhl
IGhvc3QgdmFsdWUgYmFjayBpbiB2ZXJ5IGVhcmx5LiBBbmQNCj4gPiA+Pj4+Pj4+IGV2ZW4gdGhl
biBvdXIgaW50ZXJydXB0IGhhbmRsZXJzIHdvdWxkbid0IHdvcmsgYW55bW9yZS4NCj4gPiA+Pj4+
Pj4+DQo+ID4gPj4+Pj4+PiBUaGlzIGlzIG1vcmUgY29tcGxpY2F0ZWQgOikuDQo+ID4gPj4+Pj4+
IFRvIG1ha2UgdGhpcyB3b3JrIHdlIG5lZWQgdG8gYXZvaWQgU1BSRzMgYXMgd2VsbCwgb3IgYXQg
bGVhc3QNCj4gPiA+Pj4+Pj4gYXZvaWQgdXNpbmcgaXQgZm9yIHNvbWV0aGluZyBuZWVkZWQgcHJp
b3IgdG8gRE9fS1ZNLg0KPiA+ID4+Pj4+Pg0KPiA+ID4+Pj4+PiBXZSBhbHNvIG5lZWQgdG8gdXBk
YXRlIHRoZSBkb2N1bWVudGF0aW9uIGluIHJlZy5oIHRvIHJlZmxlY3QNCj4gPiA+Pj4+Pj4gdGhl
IGZhY3QgdGhhdCB3ZSBkb24ndCB1c2UgU1BSRzQtNyBhbnltb3JlIG9uIGU1MDAuDQo+ID4gPj4+
Pj4gSSB3b3VsZCBwZXJzb25hbGx5IHByZWZlciBpZiB3ZSBjbGFpbSBTUFJHM1IgYXMgdW5zdXBw
b3J0ZWQgb24NCj4gPiA+Pj4+PiBlNTAwdjIgdW50aWwgd2UgZmluZCBzb21lb25lIHdobyBhY3R1
YWxseSB1c2VzIGl0LiBUaGVyZSdzIGENCj4gPiA+Pj4+PiBnb29kIGNoYW5jZSB3ZSdkIHN0YXJ0
IGp1bXBpbmcgdGhyb3VnaCBhIGxvdCBvZiBob29wcyBhbmQgcmVkdWNlDQo+ID4gPj4+Pj4gb3Zl
cmFsbCBwZXJmb3JtYW5jZSBmb3Igbm8gcmVhbC13b3JsZCBnYWluIHRvZGF5Lg0KPiA+ID4+Pj4g
VGhlIHNhbWUgcHJvYmxlbSBhcHBsaWVzIHRvIGU1MDBtYy4NCj4gPiA+IFdlIGhhdmUgdHdvIFNQ
UkczIHJlZ2lzdGVycw0KPiA+ID4gICAxKSBTUFJOX1NQUkczICAgICAgICAgIC0tIEFsbCByZWFk
L3dyaXRlIGFjY2VzcyB0byB0aGlzIHJlZ2lzdGVyIGluIGd1ZXN0DQo+ID4gd2lsbCB0cmFwIGFu
ZCBlbXVsYXRlLCBTbyBubyBuZWVkIHRvIHNhdmUvcmVzdG9yZS4NCj4gPiA+ICAgMikgU1BSTl9T
UFJHM1IgICAgICAgICAtLSBUaGlzIGlzIGd1ZXN0IHJlYWQgb25seSBhbmQgV2UgZG8gbm90IHNl
ZSBhbnkNCj4gdXNlcg0KPiA+IG9mIHRoaXMgcmVnaXN0ZXIsIHNvIGNhbiBsZWF2ZSB0aGlzIGZv
ciBub3cNCj4gPiA+DQo+ID4gPj4+IFRoZXJlIHdlIGhhdmUgU1BSTl9HU1BSRzMsIG5vPw0KPiA+
ID4+IE9oLCByaWdodC4NCj4gPiA+Pg0KPiA+ID4+IFNpbmNlIGl0J3Mgb25seSBhIHByb2JsZW0g
Zm9yIFBSLW1vZGUsIGl0IGNhbiBiZSBmaXhlZCB3aXRob3V0DQo+ID4gPj4gbmVlZGluZyB0byBh
dm9pZA0KPiA+ID4+IFNQUkczIGVudGlyZWx5LCBzaW5jZSBQUi1tb2RlIGRvZXNuJ3QgdXNlIERP
X0tWTS4gIFdlJ2Qgb25seSBuZWVkDQo+ID4gPj4gdG8gYXZvaWQgdXNpbmcgU1BSR19USFJFQUQg
aW4gX19LVk1fSEFORExFUiAoaS5lLiByZXZlcnQgY29tbWl0DQo+ID4gPj4gZmZlMTI5ZWNkNzk3
NzkyMjFmZGIwMzMwNTA0OWVjOGI1YThiZWIwZikuDQo+ID4gPiBEaWQgbm90IGdldCB3aHkgdXNp
bmcgU1BSR19USFJFQUQgaGVyZSBpcyBhIHByb2JsZW0/IElzIG5vdCB0aGlzDQo+ID4gPiB3aWxs
IGFsd2F5cyBhY2Nlc3MgaG9zdCBTUFJHMyBhbmQgZ3Vlc3QgcmVhZCB3cml0ZSB3aWxsIGFsd2F5
cyB0cmFwDQo+ID4gPiAobWFpbnRhaW5lZCBpbiB2Y3B1LT5hcmNoLT5zaGFyZWQtPnJlZy0+c3By
ZzMpDQo+ID4NCj4gPiBHdWVzdCByZWFkcyB2aWEgU1BSRzNSIHdpbGwgYWNjZXNzIHRoZSByZWFs
IFNQUkczIHJlZ2lzdGVyLiBHdWVzdA0KPiA+IHJlYWRzIHZpYQ0KPiA+IFNQUkczIHdpbGwgdHJh
cCA6KS4NCj4gDQo+IEFncmVlLCBBbHNvIHdlIGRvIG5vdCBzZWUgTGludXggYXMgZ3Vlc3QgaXMg
YWNjZXNzaW5nIFNQUkczUi4gQnV0IHdoYXQgSSBkaWQgbm90DQo+IGdldCB3aGF0IHRoZSBtZW50
aW9uZWQgcGF0Y2ggaGF2ZSB0byBkbyB3aXRoIHRoYXQ/DQoNCkdvdCBpdCBhZnRlciBkaXNjdXNz
aW9uIG9uIElSQywgSWYgd2UgbmVlZCB0byBjYXJlIGFib3V0IFNQUk5HM1IgZW11bGF0aW9uIHRo
ZW4gd2Ugc2hvdWxkIG5vdCB1c2UgU1BSR19USFJFQUQgLg0KDQpUaGFua3MNCi1CaGFyYXQNCj4g
DQo+IA0KPiA+DQo+ID4NCj4gPiBBbGV4DQoNCg=

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

> >
> > On 18.07.14 11:57, Bharat.Bhushan@freescale.com wrote:
> > >
> > >> -----Original Message-----
> > >> From: Wood Scott-B07421
> > >> Sent: Friday, July 18, 2014 6:19 AM
> > >> To: Alexander Graf
> > >> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org;
> > >> kvm@vger.kernel.org; Yoder
> > >> Stuart-B08248
> > >> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering
> > >> guest
> > >>
> > >> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
> > >>> On 18.07.14 02:36, Scott Wood wrote:
> > >>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
> > >>>>> On 18.07.14 02:28, Scott Wood wrote:
> > >>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
> > >>>>>>> On 17.07.14 18:27, Alexander Graf wrote:
> > >>>>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> > >>>>>>>>>> -----Original Message-----
> > >>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
> > >>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
> > >>>>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> > >>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder
> > >>>>>>>>>> Stuart-B08248
> > >>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when
> > >>>>>>>>>> entering guest
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
> > >>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by
> > >>>>>>>>>>> host or another guest, So this need to be restored when
> > >>>>>>>>>>> loading guest
> > >> state.
> > >>>>>> SPRG3 is not guest writeable.  We should be doing this so that
> > >>>>>> guest reads of SPRG3 through the alternative read-only SPR
> > >>>>>> work, not because
> > >>>>>> "SPRG3 can be clobbered by host or another guest".
> > >>>>>>
> > >>>>>>>>>>> Signed-off-by: Bharat Bhushan
> > >>>>>>>>>>> <Bharat.Bhushan@freescale.com>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>       arch/powerpc/kvm/booke_interrupts.S | 2 ++
> > >>>>>>>>>>>       1 file changed, 2 insertions(+)
> > >>>>>>>>>>>
> > >>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>>>>> index 2c6deb5ef..0d3403f 100644
> > >>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> > >>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
> > >>>>>>>>>>>            * written directly to the shared area, so we
> > >>>>>>>>>>>            * need to reload them here with the guest's values.
> > >>>>>>>>>>>            */
> > >>>>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> > >>>>>>>>>>> +    mtspr    SPRN_SPRG3, r3
> > >>>>>>>>>> We also need to restore it when resuming the host, no?
> > >>>>>>>>> I do not think host expect some meaningful value when
> > >>>>>>>>> returning from guest, same true for SPRG4-7.
> > >>>>>>>>> So there seems no reason to save host values and restore them.
> > >>>>>> Linux no longer uses SPRG4-7 for itself.  That is not true of
> > >>>>>> SPRG3, as Alex points out.
> > >>>>>>
> > >>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
> > >>>>>>>>
> > >>>>>>>>     * All 32-bit:
> > >>>>>>>>     *      - SPRG3 current thread_info pointer
> > >>>>>>>>     *        (virtual on BookE, physical on others)
> > >>>>>>>>
> > >>>>>>>> but I can indeed find no trace of usage anywhere. This at
> > >>>>>>>> least needs to go into the patch description.
> > >>>>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
> > >>>>>>> incredibly important that I have no idea how we could possibly
> > >>>>>>> run without switching the host value back in very early. And
> > >>>>>>> even then our interrupt handlers wouldn't work anymore.
> > >>>>>>>
> > >>>>>>> This is more complicated :).
> > >>>>>> To make this work we need to avoid SPRG3 as well, or at least
> > >>>>>> avoid using it for something needed prior to DO_KVM.
> > >>>>>>
> > >>>>>> We also need to update the documentation in reg.h to reflect
> > >>>>>> the fact that we don't use SPRG4-7 anymore on e500.
> > >>>>> I would personally prefer if we claim SPRG3R as unsupported on
> > >>>>> e500v2 until we find someone who actually uses it. There's a
> > >>>>> good chance we'd start jumping through a lot of hoops and reduce
> > >>>>> overall performance for no real-world gain today.
> > >>>> The same problem applies to e500mc.
> > > We have two SPRG3 registers
> > >   1) SPRN_SPRG3          -- All read/write access to this register in guest
> > will trap and emulate, So no need to save/restore.
> > >   2) SPRN_SPRG3R         -- This is guest read only and We do not see any
> user
> > of this register, so can leave this for now
> > >
> > >>> There we have SPRN_GSPRG3, no?
> > >> Oh, right.
> > >>
> > >> Since it's only a problem for PR-mode, it can be fixed without
> > >> needing to avoid
> > >> SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only need
> > >> to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
> > >> ffe129ecd79779221fdb03305049ec8b5a8beb0f).
> > > Did not get why using SPRG_THREAD here is a problem? Is not this
> > > will always access host SPRG3 and guest read write will always trap
> > > (maintained in vcpu->arch->shared->reg->sprg3)
> >
> > Guest reads via SPRG3R will access the real SPRG3 register. Guest
> > reads via
> > SPRG3 will trap :).
> 
> Agree, Also we do not see Linux as guest is accessing SPRG3R. But what I did not
> get what the mentioned patch have to do with that?

Got it after discussion on IRC, If we need to care about SPRNG3R emulation then we should not use SPRG_THREAD .

Thanks
-Bharat
> 
> 
> >
> >
> > Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 02:49, Scott Wood wrote:
> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
>> On 18.07.14 02:36, Scott Wood wrote:
>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
>>>> On 18.07.14 02:28, Scott Wood wrote:
>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>>>>>> On 17.07.14 18:27, Alexander Graf wrote:
>>>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>>>>>>>> another guest, So this need to be restored when loading guest state.
>>>>> SPRG3 is not guest writeable.  We should be doing this so that guest
>>>>> reads of SPRG3 through the alternative read-only SPR work, not because
>>>>> "SPRG3 can be clobbered by host or another guest".
>>>>>
>>>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>>>> ---
>>>>>>>>>>       arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>>>>>       1 file changed, 2 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>>>>>            * written directly to the shared area, so we
>>>>>>>>>>            * need to reload them here with the guest's values.
>>>>>>>>>>            */
>>>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>>>>>> We also need to restore it when resuming the host, no?
>>>>>>>> I do not think host expect some meaningful value when returning from
>>>>>>>> guest, same true for SPRG4-7.
>>>>>>>> So there seems no reason to save host values and restore them.
>>>>> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
>>>>> Alex points out.
>>>>>
>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>>>>>
>>>>>>>     * All 32-bit:
>>>>>>>     *      - SPRG3 current thread_info pointer
>>>>>>>     *        (virtual on BookE, physical on others)
>>>>>>>
>>>>>>> but I can indeed find no trace of usage anywhere. This at least needs
>>>>>>> to go into the patch description.
>>>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>>>>>> incredibly important that I have no idea how we could possibly run
>>>>>> without switching the host value back in very early. And even then our
>>>>>> interrupt handlers wouldn't work anymore.
>>>>>>
>>>>>> This is more complicated :).
>>>>> To make this work we need to avoid SPRG3 as well, or at least avoid
>>>>> using it for something needed prior to DO_KVM.
>>>>>
>>>>> We also need to update the documentation in reg.h to reflect the fact
>>>>> that we don't use SPRG4-7 anymore on e500.
>>>> I would personally prefer if we claim SPRG3R as unsupported on e500v2
>>>> until we find someone who actually uses it. There's a good chance we'd
>>>> start jumping through a lot of hoops and reduce overall performance for
>>>> no real-world gain today.
>>> The same problem applies to e500mc.
>> There we have SPRN_GSPRG3, no?
> Oh, right.
>
> Since it's only a problem for PR-mode, it can be fixed without needing
> to avoid SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only
> need to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
> ffe129ecd79779221fdb03305049ec8b5a8beb0f).
>
> And if we decide it's not worthwhile and don't revert that commit, we
> should at least remove the comment that "Under KVM, the host SPRG1 is
> used to point to the current VCPU data structure"...

Agreed, please send a patch :).


Alex

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Alexander Graf]

On 18.07.14 02:49, Scott Wood wrote:
> On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
>> On 18.07.14 02:36, Scott Wood wrote:
>>> On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
>>>> On 18.07.14 02:28, Scott Wood wrote:
>>>>> On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
>>>>>> On 17.07.14 18:27, Alexander Graf wrote:
>>>>>>> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>>>>> Sent: Thursday, July 17, 2014 9:41 PM
>>>>>>>>> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
>>>>>>>>> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
>>>>>>>>> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 16.07.14 08:02, Bharat Bhushan wrote:
>>>>>>>>>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
>>>>>>>>>> another guest, So this need to be restored when loading guest state.
>>>>> SPRG3 is not guest writeable.  We should be doing this so that guest
>>>>> reads of SPRG3 through the alternative read-only SPR work, not because
>>>>> "SPRG3 can be clobbered by host or another guest".
>>>>>
>>>>>>>>>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
>>>>>>>>>> ---
>>>>>>>>>>       arch/powerpc/kvm/booke_interrupts.S | 2 ++
>>>>>>>>>>       1 file changed, 2 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> index 2c6deb5ef..0d3403f 100644
>>>>>>>>>> --- a/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> +++ b/arch/powerpc/kvm/booke_interrupts.S
>>>>>>>>>> @@ -459,6 +459,8 @@ lightweight_exit:
>>>>>>>>>>            * written directly to the shared area, so we
>>>>>>>>>>            * need to reload them here with the guest's values.
>>>>>>>>>>            */
>>>>>>>>>> +    PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
>>>>>>>>>> +    mtspr    SPRN_SPRG3, r3
>>>>>>>>> We also need to restore it when resuming the host, no?
>>>>>>>> I do not think host expect some meaningful value when returning from
>>>>>>>> guest, same true for SPRG4-7.
>>>>>>>> So there seems no reason to save host values and restore them.
>>>>> Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
>>>>> Alex points out.
>>>>>
>>>>>>> Hmm - arch/powerpc/include/asm/reg.h says:
>>>>>>>
>>>>>>>     * All 32-bit:
>>>>>>>     *      - SPRG3 current thread_info pointer
>>>>>>>     *        (virtual on BookE, physical on others)
>>>>>>>
>>>>>>> but I can indeed find no trace of usage anywhere. This at least needs
>>>>>>> to go into the patch description.
>>>>>> Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
>>>>>> incredibly important that I have no idea how we could possibly run
>>>>>> without switching the host value back in very early. And even then our
>>>>>> interrupt handlers wouldn't work anymore.
>>>>>>
>>>>>> This is more complicated :).
>>>>> To make this work we need to avoid SPRG3 as well, or at least avoid
>>>>> using it for something needed prior to DO_KVM.
>>>>>
>>>>> We also need to update the documentation in reg.h to reflect the fact
>>>>> that we don't use SPRG4-7 anymore on e500.
>>>> I would personally prefer if we claim SPRG3R as unsupported on e500v2
>>>> until we find someone who actually uses it. There's a good chance we'd
>>>> start jumping through a lot of hoops and reduce overall performance for
>>>> no real-world gain today.
>>> The same problem applies to e500mc.
>> There we have SPRN_GSPRG3, no?
> Oh, right.
>
> Since it's only a problem for PR-mode, it can be fixed without needing
> to avoid SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only
> need to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
> ffe129ecd79779221fdb03305049ec8b5a8beb0f).
>
> And if we decide it's not worthwhile and don't revert that commit, we
> should at least remove the comment that "Under KVM, the host SPRG1 is
> used to point to the current VCPU data structure"...

Agreed, please send a patch :).


Alex

RE: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Bharat.Bhushan]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 17, 2014 9:58 PM
> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
> 
> 
> On 17.07.14 18:24, Bharat.Bhushan@freescale.com wrote:
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, July 17, 2014 9:41 PM
> >> To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
> >> Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
> >> Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering
> >> guest
> >>
> >>
> >> On 16.07.14 08:02, Bharat Bhushan wrote:
> >>> SPRG3 is guest accessible and SPRG3 can be clobbered by host or
> >>> another guest, So this need to be restored when loading guest state.
> >>>
> >>> Signed-off-by: Bharat Bhushan <Bharat.Bhushan@freescale.com>
> >>> ---
> >>>    arch/powerpc/kvm/booke_interrupts.S | 2 ++
> >>>    1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/kvm/booke_interrupts.S
> >>> b/arch/powerpc/kvm/booke_interrupts.S
> >>> index 2c6deb5ef..0d3403f 100644
> >>> --- a/arch/powerpc/kvm/booke_interrupts.S
> >>> +++ b/arch/powerpc/kvm/booke_interrupts.S
> >>> @@ -459,6 +459,8 @@ lightweight_exit:
> >>>    	 * written directly to the shared area, so we
> >>>    	 * need to reload them here with the guest's values.
> >>>    	 */
> >>> +	PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
> >>> +	mtspr	SPRN_SPRG3, r3
> >> We also need to restore it when resuming the host, no?
> > I do not think host expect some meaningful value when returning from guest,
> same true for SPRG4-7.
> > So there seems no reason to save host values and restore them.
> 
> Hmm - arch/powerpc/include/asm/reg.h says:
> 
>   * All 32-bit:
>   *      - SPRG3 current thread_info pointer
>   *        (virtual on BookE, physical on others)
> 
> but I can indeed find no trace of usage anywhere. This at least needs to go into
> the patch description.

I will add a comment in code as well.

Thanks
-Bharat

> 
> 
> Alex