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

Bharat.Bhushan
2014-07-18T09:57:49Z

> -----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
>