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

[Caraman Mihai Claudiu-B02008]

>________________________________________
>From: Alexander Graf [agraf at suse.de]
>Sent: Wednesday, July 04, 2012 6:45 PM
>To: Caraman Mihai Claudiu-B02008
>Cc: <kvm-ppc at vger.kernel.org>; KVM list; linuxppc-dev; qemu-ppc at 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 at suse.de]
>>> Sent: Wednesday, July 04, 2012 5:30 PM
>>> To: Caraman Mihai Claudiu-B02008
>>> Cc: <kvm-ppc at vger.kernel.org>; KVM list; linuxppc-dev; qemu-
>>> ppc at 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 at 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.

>>>> -/* Guest Doorbell */
>>>> -   MASKABLE_EXCEPTION(0x2c0, guest_doorbell, .unknown_exception,
>>> ACK_NONE)
>>>> +/*
>>>> + * Guest doorbell interrupt
>>>> + * This general exception use GSRRx save/restore registers
>>>> + */
>>>> +   START_EXCEPTION(guest_doorbell);
>>>> +   EXCEPTION_PROLOG(0x2c0, BOOKE_INTERRUPT_GUEST_DBELL, GEN,
>>>> +                    SPRN_GSRR0, SPRN_GSRR1, PROLOG_ADDITION_NONE)
>>>> +   EXCEPTION_COMMON(0x2c0, PACA_EXGEN, INTS_KEEP)
>>>> +   addi    r3,r1,STACK_FRAME_OVERHEAD
>>>> +   bl      .save_nvgprs
>>>> +   INTS_RESTORE_HARD
>>>> +   bl      .unknown_exception
>>>> +   b       .ret_from_except
>>>
>>> This is independent of DO_KVM, right?
>>
>> Yes, just kvm_handler definitions in bookehv_interrupts.S depends on this.
>
>Then please split it out into a separate patch.

Can you be more precise, are you referring to guest_doorbell exception handler?

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

-Mike