[PATCH 1/3] kvm tools: Fix powerpc build errors caused by recent changes

[PATCH 1/3] kvm tools: Fix powerpc build errors caused by recent changes

[Michael Ellerman]

Several caused by commit 8074303 "remove global kvm object",
ioport__setup_arch(), term_getc_iov() & term_getc() in the
spapr_hvcons.c code, and kvm_cpu__reboot() in rtas_power_off().

Commit 221b584 "move active_console into struct kvm_config" added
checks in h_put_term_char() & h_get_term_char() of
kvm->cfg.active_console but needs to be vcpu->kvm->cfg.active_console.

That commit also missed updates to term_putc() & term_getc() in
spapr_rtas.c, and I'm guessing that we need similar checks of
active_console in rtas_put_term_char() & rtas_get_term_char().

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 tools/kvm/powerpc/ioport.c       |    2 +-
 tools/kvm/powerpc/spapr_hvcons.c |    6 +++---
 tools/kvm/powerpc/spapr_rtas.c   |   14 +++++++++-----
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/kvm/powerpc/ioport.c b/tools/kvm/powerpc/ioport.c
index a8e4dc3..264fb7e 100644
--- a/tools/kvm/powerpc/ioport.c
+++ b/tools/kvm/powerpc/ioport.c
@@ -12,7 +12,7 @@
 
 #include <stdlib.h>
 
-void ioport__setup_arch(void)
+void ioport__setup_arch(struct kvm *kvm)
 {
 	/* PPC has no legacy ioports to set up */
 }
diff --git a/tools/kvm/powerpc/spapr_hvcons.c b/tools/kvm/powerpc/spapr_hvcons.c
index 1fe4bdb..0bdf75b 100644
--- a/tools/kvm/powerpc/spapr_hvcons.c
+++ b/tools/kvm/powerpc/spapr_hvcons.c
@@ -50,7 +50,7 @@ static unsigned long h_put_term_char(struct kvm_cpu *vcpu, unsigned long opcode,
 	do {
 		int ret;
 
-		if (kvm->cfg.active_console == CONSOLE_HV)
+		if (vcpu->kvm->cfg.active_console == CONSOLE_HV)
 			ret = term_putc_iov(&iov, 1, 0);
 		else
 			ret = 0;
@@ -74,14 +74,14 @@ static unsigned long h_get_term_char(struct kvm_cpu *vcpu, unsigned long opcode,
 	union hv_chario data;
 	struct iovec iov;
 
-	if (kvm->cfg.active_console != CONSOLE_HV)
+	if (vcpu->kvm->cfg.active_console != CONSOLE_HV)
 		return H_SUCCESS;
 
 	if (term_readable(0)) {
 		iov.iov_base = data.buf;
 		iov.iov_len = 16;
 
-		*len = term_getc_iov(&iov, 1, 0);
+		*len = term_getc_iov(vcpu->kvm, &iov, 1, 0);
 		*char0_7 = be64_to_cpu(data.a.char0_7);
 		*char8_15 = be64_to_cpu(data.a.char8_15);
 	} else {
diff --git a/tools/kvm/powerpc/spapr_rtas.c b/tools/kvm/powerpc/spapr_rtas.c
index 14a3462..c81d82b 100644
--- a/tools/kvm/powerpc/spapr_rtas.c
+++ b/tools/kvm/powerpc/spapr_rtas.c
@@ -41,7 +41,7 @@ static void rtas_display_character(struct kvm_cpu *vcpu,
                                    uint32_t nret, target_ulong rets)
 {
 	char c = rtas_ld(vcpu->kvm, args, 0);
-	term_putc(CONSOLE_HV, &c, 1, 0);
+	term_putc(&c, 1, 0);
 	rtas_st(vcpu->kvm, rets, 0, 0);
 }
 
@@ -52,7 +52,10 @@ static void rtas_put_term_char(struct kvm_cpu *vcpu,
 			       uint32_t nret, target_ulong rets)
 {
 	char c = rtas_ld(vcpu->kvm, args, 0);
-	term_putc(CONSOLE_HV, &c, 1, 0);
+
+	if (vcpu->kvm->cfg.active_console == CONSOLE_HV)
+		term_putc(&c, 1, 0);
+
 	rtas_st(vcpu->kvm, rets, 0, 0);
 }
 
@@ -62,8 +65,9 @@ static void rtas_get_term_char(struct kvm_cpu *vcpu,
 			       uint32_t nret, target_ulong rets)
 {
 	int c;
-	if (term_readable(CONSOLE_HV, 0) &&
-	    (c = term_getc(CONSOLE_HV, 0)) >= 0) {
+
+	if (vcpu->kvm->cfg.active_console == CONSOLE_HV && term_readable(0) &&
+	    (c = term_getc(vcpu->kvm, 0)) >= 0) {
 		rtas_st(vcpu->kvm, rets, 0, 0);
 		rtas_st(vcpu->kvm, rets, 1, c);
 	} else {
@@ -115,7 +119,7 @@ static void rtas_power_off(struct kvm_cpu *vcpu,
 		rtas_st(vcpu->kvm, rets, 0, -3);
 		return;
 	}
-	kvm_cpu__reboot();
+	kvm_cpu__reboot(vcpu->kvm);
 }
 
 static void rtas_query_cpu_stopped_state(struct kvm_cpu *vcpu,
-- 
1.7.9.5

[PATCH 2/3] kvm tools: Fix segfault on powerpc in xics_register()

[Michael Ellerman]

In commit 06e6648 "move kvm_cpus into struct kvm", kvm_cpu__init() became
kvm_cpu__arch_init() called from a new kvm_cpu__init(), and the call was moved
from the end of the init sequence to much earlier, and in particular prior to
irq__init().

This leads to a segfault on powerpc, because kvm_cpu__arch_init() calls into
xics_cpu_register(), which dereferences vcpu->kvm.icp which is uninitialised
until irq__init().

Later in commit a48488d "use init/exit where possible", irq__init() was pulled
out of the init sequence and made a dev_base_init() routine, on x86. On powerpc
the call to irq__init() was dropped entirely.

Finally, we now have a circular dependency between kvm_cpu__init() (which needs
kvm->arch.icp), and irq__init() (which needs kvm->nrcpus). This is caused by
the combination of commit 89f40a7 "move nrcpus into struct kvm_config",
which moved the global nrcpus into kvm->cfg, and commit 06e6648 "move kvm_cpus
into struct kvm", which moved the setup of kvm->nrcpus from kvm->cfg into
kvm_cpu__init().

To fix it we drop irq__init() entirely, if we ever have a non xics irq option
we can bring it back. We turn xics_system_init() into xics_init(), and have it
do the allocation and setup of the icp/ics, including the per-vcpu setup,
removing the dependency from kvm_cpu__init() (via kvm_cpu__arch_init()).

xics_init() is a base_init() routine, it can't be core, which should be early
enough, fingers crossed.

Finally drop irq__exit(), it does nothing and is never called.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 tools/kvm/powerpc/irq.c     |   19 -------------------
 tools/kvm/powerpc/kvm-cpu.c |    3 ---
 tools/kvm/powerpc/xics.c    |   38 +++++++++++++++++++++++---------------
 tools/kvm/powerpc/xics.h    |    5 -----
 4 files changed, 23 insertions(+), 42 deletions(-)

diff --git a/tools/kvm/powerpc/irq.c b/tools/kvm/powerpc/irq.c
index 6d134c5..e89fa3b 100644
--- a/tools/kvm/powerpc/irq.c
+++ b/tools/kvm/powerpc/irq.c
@@ -26,8 +26,6 @@
 #include "xics.h"
 #include "spapr_pci.h"
 
-#define XICS_IRQS               1024
-
 /*
  * FIXME: The code in this file assumes an SPAPR guest, using XICS.  Make
  * generic & cope with multiple PPC platform types.
@@ -51,23 +49,6 @@ int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line)
 	return 0;
 }
 
-int irq__init(struct kvm *kvm)
-{
-	/*
-	 * kvm->nr_cpus is now valid; for /now/, pass
-	 * this to xics_system_init(), which assumes servers
-	 * are numbered 0..nrcpus.  This may not really be true,
-	 * but it is OK currently.
-	 */
-	kvm->arch.icp = xics_system_init(XICS_IRQS, kvm->nrcpus);
-	return 0;
-}
-
-int irq__exit(struct kvm *kvm)
-{
-	return 0;
-}
-
 int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
 {
 	die(__FUNCTION__);
diff --git a/tools/kvm/powerpc/kvm-cpu.c b/tools/kvm/powerpc/kvm-cpu.c
index 6aaf424..8fce121 100644
--- a/tools/kvm/powerpc/kvm-cpu.c
+++ b/tools/kvm/powerpc/kvm-cpu.c
@@ -93,9 +93,6 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 	 */
 	vcpu->is_running = true;
 
-	/* Register with IRQ controller (FIXME, assumes XICS) */
-	xics_cpu_register(vcpu);
-
 	return vcpu;
 }
 
diff --git a/tools/kvm/powerpc/xics.c b/tools/kvm/powerpc/xics.c
index 1cf9558..d4b5caa 100644
--- a/tools/kvm/powerpc/xics.c
+++ b/tools/kvm/powerpc/xics.c
@@ -18,6 +18,8 @@
 #include <stdio.h>
 #include <malloc.h>
 
+#define XICS_NUM_IRQS	1024
+
 
 /* #define DEBUG_XICS yes */
 #ifdef DEBUG_XICS
@@ -441,26 +443,19 @@ static void rtas_int_on(struct kvm_cpu *vcpu, uint32_t token,
 	rtas_st(vcpu->kvm, rets, 0, 0); /* Success */
 }
 
-void xics_cpu_register(struct kvm_cpu *vcpu)
-{
-	if (vcpu->cpu_id < vcpu->kvm->arch.icp->nr_servers)
-		vcpu->kvm->arch.icp->ss[vcpu->cpu_id].cpu = vcpu;
-	else
-		die("Setting invalid server for cpuid %ld\n", vcpu->cpu_id);
-}
-
-struct icp_state *xics_system_init(unsigned int nr_irqs, unsigned int nr_cpus)
+static int xics_init(struct kvm *kvm)
 {
 	int max_server_num;
 	unsigned int i;
 	struct icp_state *icp;
 	struct ics_state *ics;
+	int j;
 
-	max_server_num = nr_cpus;
+	max_server_num = kvm->nrcpus;
 
 	icp = malloc(sizeof(*icp));
 	icp->nr_servers = max_server_num + 1;
-	icp->ss = malloc(icp->nr_servers*sizeof(struct icp_server_state));
+	icp->ss = malloc(icp->nr_servers * sizeof(struct icp_server_state));
 
 	for (i = 0; i < icp->nr_servers; i++) {
 		icp->ss[i].xirr = 0;
@@ -475,14 +470,14 @@ struct icp_state *xics_system_init(unsigned int nr_irqs, unsigned int nr_cpus)
 	 */
 
 	ics = malloc(sizeof(*ics));
-	ics->nr_irqs = nr_irqs;
+	ics->nr_irqs = XICS_NUM_IRQS;
 	ics->offset = XICS_IRQ_OFFSET;
-	ics->irqs = malloc(nr_irqs * sizeof(struct ics_irq_state));
+	ics->irqs = malloc(ics->nr_irqs * sizeof(struct ics_irq_state));
 
 	icp->ics = ics;
 	ics->icp = icp;
 
-	for (i = 0; i < nr_irqs; i++) {
+	for (i = 0; i < ics->nr_irqs; i++) {
 		ics->irqs[i].server = 0;
 		ics->irqs[i].priority = 0xff;
 		ics->irqs[i].saved_priority = 0xff;
@@ -500,8 +495,21 @@ struct icp_state *xics_system_init(unsigned int nr_irqs, unsigned int nr_cpus)
 	spapr_rtas_register("ibm,int-off", rtas_int_off);
 	spapr_rtas_register("ibm,int-on", rtas_int_on);
 
-	return icp;
+	for (j = 0; j < kvm->nrcpus; j++) {
+		struct kvm_cpu *vcpu = kvm->cpus[j];
+
+		if (vcpu->cpu_id >= icp->nr_servers)
+			die("Invalid server number for cpuid %ld\n", vcpu->cpu_id);
+
+		icp->ss[vcpu->cpu_id].cpu = vcpu;
+	}
+
+	kvm->arch.icp = icp;
+
+	return 0;
 }
+base_init(xics_init);
+
 
 void kvm__irq_line(struct kvm *kvm, int irq, int level)
 {
diff --git a/tools/kvm/powerpc/xics.h b/tools/kvm/powerpc/xics.h
index 144915b..d5bc6f9 100644
--- a/tools/kvm/powerpc/xics.h
+++ b/tools/kvm/powerpc/xics.h
@@ -13,11 +13,6 @@
 
 #define XICS_IPI        0x2
 
-struct kvm_cpu;
-struct icp_state;
-
-struct icp_state *xics_system_init(unsigned int nr_irqs, unsigned int nr_cpus);
-void xics_cpu_register(struct kvm_cpu *vcpu);
 int xics_alloc_irqnum(void);
 
 #endif
-- 
1.7.9.5

[PATCH 3/3] kvm tools: Do setup_fdt() later, get powerpc to boot again

[Michael Ellerman]

In commit e3d3ced "kernel load/firmware cleanup", the call to
kvm__arch_setup_firmware() was moved. Previously more or less at the end
of the init sequence, but that commit moved it into kvm__init() which
is a core_init() call and so runs quite early.

This broke booting powerpc guests, as setup_fdt() needs to be called
later in the setup sequence. In particular it looks at kvm->nrcpus,
which is uninitialised at that point.

In general setup_fdt() needs to run late in the sequence, as it encodes
the setup of the machine into the device tree.

So move setup_fdt() out of kvm__arch_setup_firmware() and make it a
firmware_init() call of its own.

With this patch I am able to boot guests again on HV KVM.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 tools/kvm/powerpc/kvm.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/powerpc/kvm.c b/tools/kvm/powerpc/kvm.c
index e4f5315..d675265 100644
--- a/tools/kvm/powerpc/kvm.c
+++ b/tools/kvm/powerpc/kvm.c
@@ -286,7 +286,7 @@ static void generate_segment_page_sizes(struct kvm_ppc_smmu_info *info, struct f
  * and whilst most PPC targets will require CPU/memory nodes, others like RTAS
  * should eventually be added separately.
  */
-static void setup_fdt(struct kvm *kvm)
+static int setup_fdt(struct kvm *kvm)
 {
 	uint64_t 	mem_reg_property[] = { 0, cpu_to_be64(kvm->ram_size) };
 	int 		smp_cpus = kvm->nrcpus;
@@ -488,7 +488,10 @@ static void setup_fdt(struct kvm *kvm)
 	_FDT(fdt_pack(fdt_dest));
 
 	free(segment_page_sizes.value);
+
+	return 0;
 }
+firmware_init(setup_fdt);
 
 /**
  * kvm__arch_setup_firmware
@@ -517,9 +520,6 @@ int kvm__arch_setup_firmware(struct kvm *kvm)
 
 	/* Load SLOF */
 
-	/* Init FDT */
-	setup_fdt(kvm);
-
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 1/3] kvm tools: Fix powerpc build errors caused by recent changes

[Pekka Enberg]

Applied all three patches, thanks Michael!

Re: [PATCH 1/3] kvm tools: Fix powerpc build errors caused by recent changes

[Michael Ellerman]

On Fri, 2012-10-05 at 09:30 +0300, Pekka Enberg wrote:
> Applied all three patches, thanks Michael!

Thanks Pekka.

cheers