Re: [Qemu-ppc] [RFC PATCH 12/17] PowerPC: booke64: Add DO_KVM kernel hooks

[Scott Wood <scottwood@freescale.com>]

On 07/04/2012 01:15 PM, Caraman Mihai Claudiu-B02008 wrote:
>> ________________________________________
>> From: Alexander Graf [agraf@suse.de]
>> Sent: Wednesday, July 04, 2012 6:45 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: <kvm-ppc@vger.kernel.org>; KVM list; linuxppc-dev; qemu-ppc@nongnu.org List; Benjamin Herrenschmidt
>> Subject: Re: [Qemu-ppc] [RFC PATCH 12/17] PowerPC: booke64: Add DO_KVM kernel hooks
>>
>> On 04.07.2012, at 17:27, Caraman Mihai Claudiu-B02008 wrote:
>>
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Wednesday, July 04, 2012 5:30 PM
>>>> To: Caraman Mihai Claudiu-B02008
>>>> Cc: <kvm-ppc@vger.kernel.org>; KVM list; linuxppc-dev; qemu-
>>>> ppc@nongnu.org List; Benjamin Herrenschmidt
>>>> Subject: Re: [Qemu-ppc] [RFC PATCH 12/17] PowerPC: booke64: Add DO_KVM
>>>> kernel hooks
>>>>
>>>>
>>>> On 25.06.2012, at 14:26, Mihai Caraman wrote:
>>>>
>>>>> Hook DO_KVM macro to 64-bit booke in a optimal way similar to 32-bit
>>>> booke
>>>>> see head_fsl_booke.S file. Extend interrupt handlers' parameter list
>>>> with
>>>>> interrupt vector numbers to accomodate the macro. Rework Guest Doorbell
>>>>> handler to use the proper GSRRx save/restore registers.
>>>>> Only the bolted version of tlb miss handers is addressed now.
>>>>>
>>>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>>>> ---
>>>>> arch/powerpc/kernel/exceptions-64e.S |  114 ++++++++++++++++++++++++---
>>>> -------
>>>>> arch/powerpc/mm/tlb_low_64e.S        |   14 +++-
>>>>> 2 files changed, 92 insertions(+), 36 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/exceptions-64e.S
>>>> b/arch/powerpc/kernel/exceptions-64e.S
>>>>> index 06f7aec..a60f81f 100644
>>>>> --- a/arch/powerpc/kernel/exceptions-64e.S
>>>>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>>>>> @@ -25,6 +25,8 @@
>>>>> #include <asm/ppc-opcode.h>
>>>>> #include <asm/mmu.h>
>>>>> #include <asm/hw_irq.h>
>>>>> +#include <asm/kvm_asm.h>
>>>>> +#include <asm/kvm_booke_hv_asm.h>
>>>>>
>>>>> /* XXX This will ultimately add space for a special exception save
>>>>> *     structure used to save things like SRR0/SRR1, SPRGs, MAS, etc...
>>>>> @@ -34,13 +36,24 @@
>>>>> */
>>>>> #define     SPECIAL_EXC_FRAME_SIZE  INT_FRAME_SIZE
>>>>>
>>>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>>>> +#define KVM_BOOKE_HV_MFSPR(reg, spr)                               \
>>>>> +   BEGIN_FTR_SECTION                                       \
>>>>> +           mfspr   reg, spr;                               \
>>>>> +   END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
>>>>> +#else
>>>>> +#define KVM_BOOKE_HV_MFSPR(reg, spr)
>>>>> +#endif
>>>>
>>>> Bleks - this is ugly.
>>>
>>> I agree :) But I opted to keep the optimizations done for 32-bit.
>>>
>>>> Do we really need to open-code the #ifdef here?
>>>
>>> 32-bit implementation fortunately use asm macros, we can't nest defines.
>>>
>>>> Can't the feature section code determine that the feature is disabled and
>>>> just always not include the code?
>>>
>>> CPU_FTR_EMB_HV is set even if KVM is not configured.
>>
>> I don't get the point then. Why not have the whole DO_KVM masked under FTR_SECTION_IFSET(CPU_FTR_EMB_HV)? Are there book3s_64 implementations without HV? 
> 
> I guess you refer to book3e_64. I don't know all implementations but Embedded.HV category is optional.
> 
>> Can't we just mfspr unconditionally in DO_KVM?
> 
> I think Scott should better answer this question, I don't know why he opted for the other approach.

That was on 32-bit, where some of DO_KVM's users want SRR1 for their own
purposes.

>>>>> -.macro tlb_prolog_bolted addr
>>>>> +.macro tlb_prolog_bolted intnum addr
>>>>>     mtspr   SPRN_SPRG_TLB_SCRATCH,r13
>>>>>     mfspr   r13,SPRN_SPRG_PACA
>>>>>     std     r10,PACA_EXTLB+EX_TLB_R10(r13)
>>>>>     mfcr    r10
>>>>>     std     r11,PACA_EXTLB+EX_TLB_R11(r13)
>>>>> +#ifdef CONFIG_KVM_BOOKE_HV
>>>>> +BEGIN_FTR_SECTION
>>>>> +   mfspr   r11, SPRN_SRR1
>>>>> +END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
>>>>> +#endif
>>>>
>>>> This thing really should vanish behind DO_KVM :)
>>>
>>> Then let's do it first for 32-bit ;)
>>
>> You could #ifdef it in DO_KVM for 64-bit for now. IIRC it's not done on 32-bit because the register value is used even beyond DO_KVM there.
> 
> Nope, 32-bit code is also guarded by CONFIG_KVM_BOOKE_HV.

Only in the TLB miss handlers, not the normal exception prolog.

-Scott