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

Scott Wood
2014-07-18T00:28:59Z

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