Re: [PATCH 2/2] KVM: PPC: hypervisor large decrementer support

[oliver <oohall@gmail.com>]

On Wed, Jun 1, 2016 at 4:23 PM, Michael Neuling <mikey@neuling.org> wrote:
> On Tue, 2016-05-31 at 17:16 +1000, Oliver O'Halloran wrote:
>> +#define IS_LD_ENABLED(reg)                 \
>> +     mfspr  reg,SPRN_LPCR;              \
>> +     andis. reg,reg,(LPCR_LD >> 16);
>
> FWIW you can use:
>         andis. reg,reg,(LPCR_LD)@ha
>
>> +#define GET_DEC(reg)                       \
>> +     IS_LD_ENABLED(reg);                \
>> +     mfspr reg, SPRN_DEC;               \
>> +     bne 99f;                           \
>> +     extsw reg, reg;                    \
>> +99:
>
> It's a little painful that GET_DEC() is now 2 SPR moves.  SPR moves can be
> a bit expensive.  Probably ok for now, but might be nice to store the guest
> dec LD state somewhere so we don't have to retrieve it from the LPCR SPR.

Seems reasonable. It looks like it stashes the LPCR value in the KVM vcpu
structure already


> Actually, it's probably best to do this now since checking the LD bit in
> the LPCR on P8 is a bit dodgy and unnecessary. There is a kvm->arch.lpcr
> you might be able use for this and you can add an asm-offsets for it too
> (like KVM_LPID).
>
> Is GET_DEC ever run in HV=0, where the guest couldn't read the LPCR?

It's only used in book3s_hv_rmhandlers.S, which contains the real mode h-call
handlers and none of that should be executed outside the host. Moving that
code into there from the generic exception header file is a good idea though.

> Also, this now trashes cr0... have you checked that's ok in the paths it's
> used?

It looks fine, but I'll document that.

>> +
>> +     LOAD_REG_ADDR(r6, decrementer_max);
>> +     ld      r6,0(r6);
>>       mtspr   SPRN_HDEC, r6
>>       /* and set per-LPAR registers, if doing dynamic micro-threading */
>>       ld      r6, HSTATE_SPLIT_MODE(r13)
>> @@ -897,7 +902,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
>>       mftb    r7
>>       subf    r3,r7,r8
>>       mtspr   SPRN_DEC,r3
>> -     stw     r3,VCPU_DEC(r4)
>> +     std     r3,VCPU_DEC(r4)
>>
>>       ld      r5, VCPU_SPRG0(r4)
>>       ld      r6, VCPU_SPRG1(r4)
>> @@ -953,8 +958,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
>>       isync
>>
>>       /* Check if HDEC expires soon */
>> -     mfspr   r3, SPRN_HDEC
>> -     cmpwi   r3, 512         /* 1 microsecond */
>> +     GET_HDEC(r3)
>> +     cmpdi   r3, 512         /* 1 microsecond */
>>       blt     hdec_soon
>>
>>       ld      r6, VCPU_CTR(r4)
>> @@ -990,8 +995,9 @@ deliver_guest_interrupt:
>>       beq     5f
>>       li      r0, BOOK3S_INTERRUPT_EXTERNAL
>>       bne     cr1, 12f
>> -     mfspr   r0, SPRN_DEC
>> -     cmpwi   r0, 0
>> +
>> +     GET_DEC(r0)
>> +     cmpdi   r0, 0
>
> We could just use mfspr DEC here since we are just comparing to 0.  It
> should work in any mode.

I'm not sure about that. The result of the comparison is used below:

>>       li      r0, BOOK3S_INTERRUPT_DECREMENTER
>>       bge     5f

It's checking for the DEC overflowing rather than checking if it's zero. If
LD=0 the mfspr result would not be sign extended causing the branch to be
taken even if the DEC overflowed.

Anyway I'm thinking I might drop this patch for now and let Balbir post it
as a part of his KVM series when that's ready.