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

[Alexander Graf <agraf@suse.de>]

On 18.07.14 02:49, Scott Wood wrote:
> 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.
>> 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).
>
> 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"...

Agreed, please send a patch :).


Alex