Re: [RFC PATCH 06/11] kvm: powerpc: book3s: Add is_hv_enabled to kvmppc_ops

On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:

From: "Aneesh Kumar K.V" <redacted>
=20
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.
=20
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.
=20
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.
=20
Signed-off-by: Aneesh Kumar K.V <redacted>
---
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 =

++++++++++++++++++---------------

b/arch/powerpc/include/asm/kvm_book3s.h

kvm_vcpu *vcpu)

	return vcpu->arch.fault_dar;
}
=20
-#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 =3D 1;
-	else if (old_pending)
-		vcpu->arch.shared->int_pending =3D 0;
-}
-
-static inline bool kvmppc_critical_section(struct kvm_vcpu *vcpu)
-{
-	ulong crit_raw =3D vcpu->arch.shared->critical;
-	ulong crit_r1 =3D kvmppc_get_gpr(vcpu, 1);
-	bool crit;
-
-	/* Truncate crit indicators in 32 bit mode */
-	if (!(vcpu->arch.shared->msr & MSR_SF)) {
-		crit_raw &=3D 0xffffffff;
-		crit_r1 &=3D 0xffffffff;
-	}
-
-	/* Critical section when crit =3D=3D r1 */
-	crit =3D (crit_raw =3D=3D crit_r1);
-	/* ... and we're in supervisor mode */
-	crit =3D 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

b/arch/powerpc/include/asm/kvm_ppc.h

irq);

extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
=20
struct kvmppc_ops {
+	bool is_hv_enabled;

This doesn't really belong into an ops struct. Either you compare

  if (kvmppc_ops =3D=3D kvmppc_ops_pr)

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);

unsigned long addr)

=20
static inline u32 kvmppc_get_xics_latch(void)
{
-	u32 xirr =3D get_paca()->kvm_hstate.saved_xirr;
+	u32 xirr;
=20
+	xirr =3D get_paca()->kvm_hstate.saved_xirr;
	get_paca()->kvm_hstate.saved_xirr =3D 0;
-

I don't see any functionality change here?

kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HANDLER) +=3D \

	book3s_rmhandlers.o
endif
=20
-kvm-book3s_64-objs-$(CONFIG_KVM_BOOK3S_64_HV) :=3D \
+kvm-book3s_64-objs-$(CONFIG_KVM_BOOK3S_64_HV)  +=3D \

This change looks unrelated?

kvm_vcpu *vcpu)

{
}
=20
+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.

+{
+	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 =3D 1;
+	else if (old_pending)
+		vcpu->arch.shared->int_pending =3D 0;
+}
+
+static inline bool kvmppc_critical_section(struct kvm_vcpu *vcpu)
+{
+	ulong crit_raw;
+	ulong crit_r1;
+	bool crit;
+
+	if (kvmppc_ops->is_hv_enabled)
+		return false;
+
+	crit_raw =3D vcpu->arch.shared->critical;
+	crit_r1 =3D kvmppc_get_gpr(vcpu, 1);
+
+	/* Truncate crit indicators in 32 bit mode */
+	if (!(vcpu->arch.shared->msr & MSR_SF)) {
+		crit_raw &=3D 0xffffffff;
+		crit_r1 &=3D 0xffffffff;
+	}
+
+	/* Critical section when crit =3D=3D r1 */
+	crit =3D (crit_raw =3D=3D crit_r1);
+	/* ... and we're in supervisor mode */
+	crit =3D crit && !(vcpu->arch.shared->msr & MSR_PR);
+
+	return crit;
+}
+
void kvmppc_inject_interrupt(struct kvm_vcpu *vcpu, int vec, u64 =

flags)

b/arch/powerpc/kvm/book3s_hv.c

unsigned long hva);

extern void kvm_set_spte_hva_hv(struct kvm *kvm, unsigned long hva, =

pte_t pte);

b/arch/powerpc/kvm/book3s_hv_rmhandlers.S

b/arch/powerpc/kvm/book3s_pr.c

kvm_vcpu *vcpu,

b/arch/powerpc/kvm/book3s_segment.S

Just always call it kvmppc_interrupt_pr and thus share at least that =
part of the code :).

b/arch/powerpc/kvm/book3s_xics.c

req)

interrupts

vcpu->arch.papr_enabled)

Reading this, have you test compiled your code against e500 configs?

0? Or 1?


Alex