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

Scott Wood
2014-07-18T00:36:20Z

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