On 27.09.2013, at 15:03, Aneesh Kumar K.V wrote:
> Alexander Graf <agraf@xxxxxxx> writes:
>
>> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:
>>
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
>>>
>>> This help us to identify whether we are running with hypervisor mode KVM
>>> enabled. The change is needed so that we can have both HV and PR kvm
>>> enabled in the same kernel.
>>>
>>> If both HV and PR KVM are included, interrupts come in to the HV version
>>> of the kvmppc_interrupt code, which then jumps to the PR handler,
>>> renamed to kvmppc_interrupt_pr, if the guest is a PR guest.
>>>
>>> Allowing both PR and HV in the same kernel required some changes to
>>> kvm_dev_ioctl_check_extension(), since the values returned now can't
>>> be selected with #ifdefs as much as previously. We look at is_hv_enabled
>>> to return the right value when checking for capabilities.For capabilities that
>>> are only provided by HV KVM, we return the HV value only if
>>> is_hv_enabled is true. For capabilities provided by PR KVM but not HV,
>>> we return the PR value only if is_hv_enabled is false.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> arch/powerpc/include/asm/kvm_book3s.h | 53 --------------------------------
>>> arch/powerpc/include/asm/kvm_ppc.h | 5 +--
>>> arch/powerpc/kvm/Makefile | 2 +-
>>> arch/powerpc/kvm/book3s.c | 44 +++++++++++++++++++++++++++
>>> arch/powerpc/kvm/book3s_hv.c | 1 +
>>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 +++
>>> arch/powerpc/kvm/book3s_pr.c | 1 +
>>> arch/powerpc/kvm/book3s_segment.S | 7 ++++-
>>> arch/powerpc/kvm/book3s_xics.c | 2 +-
>>> arch/powerpc/kvm/powerpc.c | 54 ++++++++++++++++++---------------
>>> 10 files changed, 90 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
>>> index 3efba3c..ba33c49 100644
>>> --- a/arch/powerpc/include/asm/kvm_book3s.h
>>> +++ b/arch/powerpc/include/asm/kvm_book3s.h
>>> @@ -297,59 +297,6 @@ static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
>>> return vcpu->arch.fault_dar;
>>> }
>>>
>>> -#ifdef CONFIG_KVM_BOOK3S_PR
>>> -
>>> -static inline unsigned long kvmppc_interrupt_offset(struct kvm_vcpu *vcpu)
>>> -{
>>> - return to_book3s(vcpu)->hior;
>>> -}
>>> -
>>> -static inline void kvmppc_update_int_pending(struct kvm_vcpu *vcpu,
>>> - unsigned long pending_now, unsigned long old_pending)
>>> -{
>>> - if (pending_now)
>>> - vcpu->arch.shared->int_pending = 1;
>>> - else if (old_pending)
>>> - vcpu->arch.shared->int_pending = 0;
>>> -}
>>> -
>>> -static inline bool kvmppc_critical_section(struct kvm_vcpu *vcpu)
>>> -{
>>> - ulong crit_raw = vcpu->arch.shared->critical;
>>> - ulong crit_r1 = kvmppc_get_gpr(vcpu, 1);
>>> - bool crit;
>>> -
>>> - /* Truncate crit indicators in 32 bit mode */
>>> - if (!(vcpu->arch.shared->msr & MSR_SF)) {
>>> - crit_raw &= 0xffffffff;
>>> - crit_r1 &= 0xffffffff;
>>> - }
>>> -
>>> - /* Critical section when crit == r1 */
>>> - crit = (crit_raw == crit_r1);
>>> - /* ... and we're in supervisor mode */
>>> - crit = crit && !(vcpu->arch.shared->msr & MSR_PR);
>>> -
>>> - return crit;
>>> -}
>>> -#else /* CONFIG_KVM_BOOK3S_PR */
>>> -
>>> -static inline unsigned long kvmppc_interrupt_offset(struct kvm_vcpu *vcpu)
>>> -{
>>> - return 0;
>>> -}
>>> -
>>> -static inline void kvmppc_update_int_pending(struct kvm_vcpu *vcpu,
>>> - unsigned long pending_now, unsigned long old_pending)
>>> -{
>>> -}
>>> -
>>> -static inline bool kvmppc_critical_section(struct kvm_vcpu *vcpu)
>>> -{
>>> - return false;
>>> -}
>>> -#endif
>>> -
>>> /* Magic register values loaded into r3 and r4 before the 'sc' assembly
>>> * instruction for the OSI hypercalls */
>>> #define OSI_SC_MAGIC_R3 0x113724FA
>>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>>> index 4d9641c..58e732f 100644
>>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>>> @@ -169,6 +169,7 @@ extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
>>> extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
>>>
>>> struct kvmppc_ops {
>>> + bool is_hv_enabled;
>>
>> This doesn't really belong into an ops struct. Either you compare
>>
>> if (kvmppc_ops == kvmppc_ops_pr)
>
> will do that in the next update.
>
>>
>> against a global known good ops struct or you put the hint somewhere into the kvm struct.
>>
>>> int (*get_sregs)(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs);
>>> int (*set_sregs)(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs);
>>> int (*get_one_reg)(struct kvm_vcpu *vcpu, u64 id,
>>> @@ -309,10 +310,10 @@ static inline void kvmppc_set_xics_phys(int cpu, unsigned long addr)
>>>
>>> static inline u32 kvmppc_get_xics_latch(void)
>>> {
>>> - u32 xirr = get_paca()->kvm_hstate.saved_xirr;
>>> + u32 xirr;
>>>
>>> + xirr = get_paca()->kvm_hstate.saved_xirr;
>>> get_paca()->kvm_hstate.saved_xirr = 0;
>>> -
>>
>> I don't see any functionality change here?
>>
>>> return xirr;
>
> Mistake on my side, I had a if condition in there before, which i later
> removed. But forgot to move the assignment back. Will fix.
>
>>> }
>>>
>>> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
>>> index c343793..a514ecd 100644
>>> --- a/arch/powerpc/kvm/Makefile
>>> +++ b/arch/powerpc/kvm/Makefile
>>> @@ -77,7 +77,7 @@ kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) += \
>>> book3s_rmhandlers.o
>>> endif
>>>
>>> -kvm-book3s_64-objs-$(CONFIG_KVM_BOOK3S_64_HV) := \
>>> +kvm-book3s_64-objs-$(CONFIG_KVM_BOOK3S_64_HV) += \
>>
>> This change looks unrelated?
>>
>
> yes. Will move it to the correct patch
>
>
>>> book3s_hv.o \
>>> book3s_hv_interrupts.o \
>>> book3s_64_mmu_hv.o
>>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>>> index bdc3f95..12f94bf 100644
>>> --- a/arch/powerpc/kvm/book3s.c
>>> +++ b/arch/powerpc/kvm/book3s.c
>>> @@ -69,6 +69,50 @@ void kvmppc_core_load_guest_debugstate(struct kvm_vcpu *vcpu)
>>> {
>>> }
>>>
>>> +static inline unsigned long kvmppc_interrupt_offset(struct kvm_vcpu *vcpu)
>>
>> Please drop the "inline" keyword on functions in .c files :). That way
>> the compiler tells us about unused functions.
>
> ok .
>
>>
>>> +{
>>> + if (!kvmppc_ops->is_hv_enabled)
>>> + return to_book3s(vcpu)->hior;
>>> + return 0;
>>> +}
>>> +
>>> +static inline void kvmppc_update_int_pending(struct kvm_vcpu *vcpu,
>>> + unsigned long pending_now, unsigned long old_pending)
>>> +{
>>> + if (kvmppc_ops->is_hv_enabled)
>>> + return;
>>> + if (pending_now)
>>> + vcpu->arch.shared->int_pending = 1;
>>> + else if (old_pending)
>>> + vcpu->arch.shared->int_pending = 0;
>>> +}
>>> +
>
> ....
>
>>> diff --git a/arch/powerpc/kvm/book3s_segment.S b/arch/powerpc/kvm/book3s_segment.S
>>> index 1abe478..e0229dd 100644
>>> --- a/arch/powerpc/kvm/book3s_segment.S
>>> +++ b/arch/powerpc/kvm/book3s_segment.S
>>> @@ -161,9 +161,14 @@ kvmppc_handler_trampoline_enter_end:
>>> .global kvmppc_handler_trampoline_exit
>>> kvmppc_handler_trampoline_exit:
>>>
>>> +#if defined(CONFIG_KVM_BOOK3S_HV)
>>> +.global kvmppc_interrupt_pr
>>> +kvmppc_interrupt_pr:
>>> + ld r9, HSTATE_SCRATCH2(r13)
>>> +#else
>>> .global kvmppc_interrupt
>>> kvmppc_interrupt:
>>
>> Just always call it kvmppc_interrupt_pr and thus share at least that
>> part of the code :).
>
> But if i don't have HV enabled, we don't compile book3s_hv_rmhandlers.S
> Hence don't have the kvmppc_interrupt symbol defined.
Ah, because we're always jumping to kvmppc_interrupt. Can we make this slightly less magical? How about we always call kvmppc_interrupt_hv when CONFIG_KVM_BOOK3S_HV_POSSIBLE and always kvmppc_interrupt_pr when CONFIG_KVM_BOOK3S_PR_POSSIBLE and then branch to kvmppc_interrupt_pr from kvmppc_interrupt_hv?
IMHO that would make the code flow more obvious.
>
>>
>>> -
>>> +#endif
>>> /* Register usage at this point:
>>> *
>>> * SPRG_SCRATCH0 = guest R13
>>> diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
>>> index f0c732e..fa7625d 100644
>>> --- a/arch/powerpc/kvm/book3s_xics.c
>>> +++ b/arch/powerpc/kvm/book3s_xics.c
>>> @@ -818,7 +818,7 @@ int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
>>> }
>>>
>>> /* Check for real mode returning too hard */
>>> - if (xics->real_mode)
>>> + if (xics->real_mode && kvmppc_ops->is_hv_enabled)
>>> return kvmppc_xics_rm_complete(vcpu, req);
>>>
>>> switch (req) {
>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>> index 69b9305..00a96fc 100644
>>> --- a/arch/powerpc/kvm/powerpc.c
>>> +++ b/arch/powerpc/kvm/powerpc.c
>>> @@ -52,7 +52,6 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
>>> return 1;
>>> }
>>>
>>> -#ifndef CONFIG_KVM_BOOK3S_64_HV
>>> /*
>>> * Common checks before entering the guest world. Call with interrupts
>>> * disabled.
>>> @@ -127,7 +126,6 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>>
>>> return r;
>>> }
>>> -#endif /* CONFIG_KVM_BOOK3S_64_HV */
>>>
>>> int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
>>> {
>>> @@ -194,11 +192,9 @@ int kvmppc_sanity_check(struct kvm_vcpu *vcpu)
>>> if ((vcpu->arch.cpu_type != KVM_CPU_3S_64) && vcpu->arch.papr_enabled)
>>> goto out;
>>>
>>> -#ifdef CONFIG_KVM_BOOK3S_64_HV
>>> /* HV KVM can only do PAPR mode for now */
>>> - if (!vcpu->arch.papr_enabled)
>>> + if (!vcpu->arch.papr_enabled && kvmppc_ops->is_hv_enabled)
>>> goto out;
>>> -#endif
>>>
>>> #ifdef CONFIG_KVM_BOOKE_HV
>>> if (!cpu_has_feature(CPU_FTR_EMB_HV))
>>> @@ -322,22 +318,26 @@ int kvm_dev_ioctl_check_extension(long ext)
>>> case KVM_CAP_DEVICE_CTRL:
>>> r = 1;
>>> break;
>>> -#ifndef CONFIG_KVM_BOOK3S_64_HV
>>> case KVM_CAP_PPC_PAIRED_SINGLES:
>>> case KVM_CAP_PPC_OSI:
>>> case KVM_CAP_PPC_GET_PVINFO:
>>> #if defined(CONFIG_KVM_E500V2) || defined(CONFIG_KVM_E500MC)
>>> case KVM_CAP_SW_TLB:
>>> #endif
>>> -#ifdef CONFIG_KVM_MPIC
>>> - case KVM_CAP_IRQ_MPIC:
>>> -#endif
>>> - r = 1;
>>> + /* We support this only for PR */
>>> + r = !kvmppc_ops->is_hv_enabled;
>>
>> Reading this, have you test compiled your code against e500 configs?
>
>
> Not yet.
Please do so - for every patch in your series. If you like I can give you my example configs I usually use to test compile this.
>
>
>>
>>> break;
>>> +#ifdef CONFIG_KVM_MMIO
>>> case KVM_CAP_COALESCED_MMIO:
>>> r = KVM_COALESCED_MMIO_PAGE_OFFSET;
>>> break;
>>> #endif
>>> +#ifdef CONFIG_KVM_MPIC
>>> + case KVM_CAP_IRQ_MPIC:
>>> + r = 1;
>>> + break;
>>> +#endif
>>> +
>>> #ifdef CONFIG_PPC_BOOK3S_64
>>> case KVM_CAP_SPAPR_TCE:
>>> case KVM_CAP_PPC_ALLOC_HTAB:
>>> @@ -348,32 +348,37 @@ int kvm_dev_ioctl_check_extension(long ext)
>>> r = 1;
>>> break;
>>> #endif /* CONFIG_PPC_BOOK3S_64 */
>>> -#ifdef CONFIG_KVM_BOOK3S_64_HV
>>> +#ifdef CONFIG_KVM_BOOK3S_HV
>>> case KVM_CAP_PPC_SMT:
>>> - r = threads_per_core;
>>> + if (kvmppc_ops->is_hv_enabled)
>>> + r = threads_per_core;
>>> + else
>>> + r = 0;
>>
>> 0? Or 1?
>>
>
> That check extension was not supported before on PR. So not sure what
> the value should be. May be 1 is better indicating we have one thread.
> Will change.
Ah, the default case (cap unknown) returns 0, so this is fine.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
