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

Bharat.Bhushan
2014-07-18T13:42:40Z

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