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

[Alexander Graf <agraf@suse.de>]

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