[PATCH 0/2] KVM: PPC: support kvm selftests

[PATCH 0/2] KVM: PPC: support kvm selftests

[Nicholas Piggin]

Hi,

This series adds initial KVM selftests support for powerpc
(64-bit, BookS). It spans 3 maintainers but it does not really
affect arch/powerpc, and it is well contained in selftests
code, just touches some makefiles and a tiny bit headers so
conflicts should be unlikely and trivial.

I guess Paolo is the best point to merge these, if no comments
or objections?

Thanks,
Nick

Nicholas Piggin (2):
  KVM: PPC: Add kvm selftests support for powerpc
  KVM: PPC: Add basic framework tests for kvm selftests

 tools/testing/selftests/kvm/Makefile          |  14 +
 .../selftests/kvm/include/kvm_util_base.h     |  13 +
 .../selftests/kvm/include/powerpc/hcall.h     |  22 ++
 .../selftests/kvm/include/powerpc/processor.h |  13 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +
 .../testing/selftests/kvm/lib/powerpc/hcall.c |  45 +++
 .../selftests/kvm/lib/powerpc/processor.c     | 355 ++++++++++++++++++
 .../testing/selftests/kvm/lib/powerpc/ucall.c |  30 ++
 .../testing/selftests/kvm/powerpc/null_test.c | 186 +++++++++
 .../selftests/kvm/powerpc/rtas_hcall.c        | 146 +++++++
 10 files changed, 834 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/include/powerpc/hcall.h
 create mode 100644 tools/testing/selftests/kvm/include/powerpc/processor.h
 create mode 100644 tools/testing/selftests/kvm/lib/powerpc/hcall.c
 create mode 100644 tools/testing/selftests/kvm/lib/powerpc/processor.c
 create mode 100644 tools/testing/selftests/kvm/lib/powerpc/ucall.c
 create mode 100644 tools/testing/selftests/kvm/powerpc/null_test.c
 create mode 100644 tools/testing/selftests/kvm/powerpc/rtas_hcall.c

-- 
2.37.2

[PATCH 1/2] KVM: PPC: Add kvm selftests support for powerpc

[Nicholas Piggin]

Implement KVM selftests support for Book3S-64.

ucalls are implemented with an unsuppored PAPR hcall number which causes
KVM to exit to userspace.

Virtual memory is only implemented for 64K page size and the radix MMU,
and only the base page size is supported for now.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tools/testing/selftests/kvm/Makefile          |  12 +
 .../selftests/kvm/include/kvm_util_base.h     |  13 +
 .../selftests/kvm/include/powerpc/hcall.h     |  20 +
 .../selftests/kvm/include/powerpc/processor.h |  13 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +
 .../testing/selftests/kvm/lib/powerpc/hcall.c |  45 +++
 .../selftests/kvm/lib/powerpc/processor.c     | 355 ++++++++++++++++++
 .../testing/selftests/kvm/lib/powerpc/ucall.c |  30 ++
 8 files changed, 498 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/include/powerpc/hcall.h
 create mode 100644 tools/testing/selftests/kvm/include/powerpc/processor.h
 create mode 100644 tools/testing/selftests/kvm/lib/powerpc/hcall.c
 create mode 100644 tools/testing/selftests/kvm/lib/powerpc/processor.c
 create mode 100644 tools/testing/selftests/kvm/lib/powerpc/ucall.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 84a627c43795..081cee3ecc0c 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -55,6 +55,10 @@ LIBKVM_s390x += lib/s390x/ucall.c
 LIBKVM_riscv += lib/riscv/processor.c
 LIBKVM_riscv += lib/riscv/ucall.c
 
+LIBKVM_powerpc += lib/powerpc/processor.c
+LIBKVM_powerpc += lib/powerpc/ucall.c
+LIBKVM_powerpc += lib/powerpc/hcall.c
+
 # Non-compiled test targets
 TEST_PROGS_x86_64 += x86_64/nx_huge_pages_test.sh
 
@@ -176,6 +180,14 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test
 TEST_GEN_PROGS_riscv += set_memory_region_test
 TEST_GEN_PROGS_riscv += kvm_binary_stats_test
 
+TEST_GEN_PROGS_powerpc += demand_paging_test
+TEST_GEN_PROGS_powerpc += dirty_log_test
+TEST_GEN_PROGS_powerpc += kvm_create_max_vcpus
+TEST_GEN_PROGS_powerpc += kvm_page_table_test
+TEST_GEN_PROGS_powerpc += rseq_test
+TEST_GEN_PROGS_powerpc += set_memory_region_test
+TEST_GEN_PROGS_powerpc += kvm_binary_stats_test
+
 TEST_PROGS += $(TEST_PROGS_$(ARCH_DIR))
 TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(ARCH_DIR))
 TEST_GEN_PROGS_EXTENDED += $(TEST_GEN_PROGS_EXTENDED_$(ARCH_DIR))
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index fbc2a79369b8..f6807aea634f 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -105,6 +105,7 @@ struct kvm_vm {
 	bool pgd_created;
 	vm_paddr_t ucall_mmio_addr;
 	vm_paddr_t pgd;
+	vm_paddr_t prtb; // powerpc process table
 	vm_vaddr_t gdt;
 	vm_vaddr_t tss;
 	vm_vaddr_t idt;
@@ -160,6 +161,7 @@ enum vm_guest_mode {
 	VM_MODE_PXXV48_4K,	/* For 48bits VA but ANY bits PA */
 	VM_MODE_P47V64_4K,
 	VM_MODE_P44V64_4K,
+	VM_MODE_P52V52_64K,
 	VM_MODE_P36V48_4K,
 	VM_MODE_P36V48_16K,
 	VM_MODE_P36V48_64K,
@@ -197,6 +199,17 @@ extern enum vm_guest_mode vm_mode_default;
 #define MIN_PAGE_SHIFT			12U
 #define ptes_per_page(page_size)	((page_size) / 8)
 
+#elif defined(__powerpc64__)
+
+/* Radix guest EA and RA are 52-bit on POWER9 and POWER10 */
+#define VM_MODE_DEFAULT			VM_MODE_P52V52_64K
+#define MIN_PAGE_SHIFT			12U /// XXX: hack to allocate more page table memory because we aren't allocating page tables well on 64K base page size
+#define ptes_per_page(page_size)	((page_size) / 8)
+
+#else
+
+#error "KVM selftests not implemented for architecture"
+
 #endif
 
 #define MIN_PAGE_SIZE		(1U << MIN_PAGE_SHIFT)
diff --git a/tools/testing/selftests/kvm/include/powerpc/hcall.h b/tools/testing/selftests/kvm/include/powerpc/hcall.h
new file mode 100644
index 000000000000..bbad5904f37a
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/powerpc/hcall.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * powerpc hcall defines
+ */
+#ifndef SELFTEST_KVM_HCALL_H
+#define SELFTEST_KVM_HCALL_H
+
+#include <linux/compiler.h>
+
+/* Ucalls use unimplemented PAPR hcall 0 which exits KVM */
+#define H_UCALL	0
+#define UCALL_R4_UCALL	0x5715 // regular ucall, r5 contains ucall pointer
+#define UCALL_R4_EXCPT	0x1b0f // other exception, r5 contains vector, r6,7 SRRs
+			       // R4==0 is a simple asm exit
+
+int64_t hcall0(uint64_t token);
+int64_t hcall1(uint64_t token, uint64_t arg1);
+int64_t hcall2(uint64_t token, uint64_t arg1, uint64_t arg2);
+
+#endif
diff --git a/tools/testing/selftests/kvm/include/powerpc/processor.h b/tools/testing/selftests/kvm/include/powerpc/processor.h
new file mode 100644
index 000000000000..b800b565b638
--- /dev/null
+++ b/tools/testing/selftests/kvm/include/powerpc/processor.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * powerpc processor specific defines
+ */
+#ifndef SELFTEST_KVM_PROCESSOR_H
+#define SELFTEST_KVM_PROCESSOR_H
+
+#include <linux/compiler.h>
+
+struct kvm_vcpu;
+extern bool (*interrupt_handler)(struct kvm_vcpu *vcpu, unsigned trap);
+
+#endif
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 3ea24a5f4c43..28ece960a0bb 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -153,6 +153,7 @@ const char *vm_guest_mode_string(uint32_t i)
 		[VM_MODE_PXXV48_4K]	= "PA-bits:ANY, VA-bits:48,  4K pages",
 		[VM_MODE_P47V64_4K]	= "PA-bits:47,  VA-bits:64,  4K pages",
 		[VM_MODE_P44V64_4K]	= "PA-bits:44,  VA-bits:64,  4K pages",
+		[VM_MODE_P52V52_64K]	= "PA-bits:52,  VA-bits:52, 64K pages",
 		[VM_MODE_P36V48_4K]	= "PA-bits:36,  VA-bits:48,  4K pages",
 		[VM_MODE_P36V48_16K]	= "PA-bits:36,  VA-bits:48, 16K pages",
 		[VM_MODE_P36V48_64K]	= "PA-bits:36,  VA-bits:48, 64K pages",
@@ -178,6 +179,7 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = {
 	[VM_MODE_PXXV48_4K]	= {  0,  0,  0x1000, 12 },
 	[VM_MODE_P47V64_4K]	= { 47, 64,  0x1000, 12 },
 	[VM_MODE_P44V64_4K]	= { 44, 64,  0x1000, 12 },
+	[VM_MODE_P52V52_64K]	= { 52, 52, 0x10000, 16 },
 	[VM_MODE_P36V48_4K]	= { 36, 48,  0x1000, 12 },
 	[VM_MODE_P36V48_16K]	= { 36, 48,  0x4000, 14 },
 	[VM_MODE_P36V48_64K]	= { 36, 48, 0x10000, 16 },
@@ -279,6 +281,14 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
 	case VM_MODE_P44V64_4K:
 		vm->pgtable_levels = 5;
 		break;
+	case VM_MODE_P52V52_64K:
+#ifdef __powerpc__
+		TEST_ASSERT(getpagesize() == 64*1024,
+			    "KVM selftests requires 64K host page size\n");
+
+		vm->pgtable_levels = 4;
+#endif
+		break;
 	default:
 		TEST_FAIL("Unknown guest mode, mode: 0x%x", mode);
 	}
diff --git a/tools/testing/selftests/kvm/lib/powerpc/hcall.c b/tools/testing/selftests/kvm/lib/powerpc/hcall.c
new file mode 100644
index 000000000000..23a56aabad42
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/powerpc/hcall.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PAPR (pseries) hcall support.
+ */
+#include "kvm_util.h"
+#include "hcall.h"
+
+int64_t hcall0(uint64_t token)
+{
+	register uintptr_t r3 asm ("r3") = token;
+
+	asm volatile("sc 1" : "+r"(r3) :
+			    : "r0", "r4", "r5", "r6", "r7", "r8", "r9",
+			      "r10","r11", "r12", "ctr", "xer",
+			      "memory");
+
+	return r3;
+}
+
+int64_t hcall1(uint64_t token, uint64_t arg1)
+{
+	register uintptr_t r3 asm ("r3") = token;
+	register uintptr_t r4 asm ("r4") = arg1;
+
+	asm volatile("sc 1" : "+r"(r3), "+r"(r4) :
+			    : "r0", "r5", "r6", "r7", "r8", "r9",
+			      "r10","r11", "r12", "ctr", "xer",
+			      "memory");
+
+	return r3;
+}
+
+int64_t hcall2(uint64_t token, uint64_t arg1, uint64_t arg2)
+{
+	register uintptr_t r3 asm ("r3") = token;
+	register uintptr_t r4 asm ("r4") = arg1;
+	register uintptr_t r5 asm ("r5") = arg2;
+
+	asm volatile("sc 1" : "+r"(r3), "+r"(r4), "+r"(r5) :
+			    : "r0", "r6", "r7", "r8", "r9",
+			      "r10","r11", "r12", "ctr", "xer",
+			      "memory");
+
+	return r3;
+}
diff --git a/tools/testing/selftests/kvm/lib/powerpc/processor.c b/tools/testing/selftests/kvm/lib/powerpc/processor.c
new file mode 100644
index 000000000000..df9901d629cf
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/powerpc/processor.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KVM selftest powerpc library code - CPU-related functions (page tables...)
+ */
+
+#include "processor.h"
+#include "kvm_util.h"
+#include "kvm_util_base.h"
+#include "hcall.h"
+
+#define RTS ((0x2UL << 61) | (0x5UL << 5)) // 52-bits
+#define RADIX_PGD_INDEX_SIZE 13
+
+void virt_arch_pgd_alloc(struct kvm_vm *vm)
+{
+	struct kvm_ppc_mmuv3_cfg mmu_cfg;
+	vm_paddr_t prtb, pgtb;
+	uint64_t *proc_table, *page_table;
+	size_t pgd_pages;
+
+	TEST_ASSERT(vm->mode == VM_MODE_P52V52_64K, "Attempt to use "
+		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
+
+	/* If needed, create page table */
+	if (vm->pgd_created)
+		return;
+
+	prtb = vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
+				 vm->memslots[MEM_REGION_PT]);
+	proc_table = addr_gpa2hva(vm, prtb);
+	memset(proc_table, 0, vm->page_size);
+	vm->prtb = prtb;
+
+	pgd_pages = 1UL << ((RADIX_PGD_INDEX_SIZE + 3) >> vm->page_shift);
+	TEST_ASSERT(pgd_pages == 1, "PGD allocation must be single page");
+	pgtb = vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
+				 vm->memslots[MEM_REGION_PT]);
+	page_table = addr_gpa2hva(vm, pgtb);
+	memset(page_table, 0, vm->page_size * pgd_pages);
+	vm->pgd = pgtb;
+
+	/* Set the base page directory in the proc table */
+	proc_table[0] = cpu_to_be64(pgtb | RTS | RADIX_PGD_INDEX_SIZE);
+
+	mmu_cfg.process_table = prtb | 0x8000000000000000UL | 0x4; // 64K size
+	mmu_cfg.flags = KVM_PPC_MMUV3_RADIX | KVM_PPC_MMUV3_GTSE;
+
+	vm_ioctl(vm, KVM_PPC_CONFIGURE_V3_MMU, &mmu_cfg);
+
+	vm->pgd_created = true;
+}
+
+static int pt_shift(struct kvm_vm *vm, int level)
+{
+	switch (level) {
+	case 1:
+		return 13;
+	case 2:
+	case 3:
+		return 9;
+	case 4:
+		return 5;
+	default:
+		TEST_ASSERT(false, "Invalid page table level %d\n", level);
+		return 0;
+	}
+}
+
+static uint64_t pt_entry_coverage(struct kvm_vm *vm, int level)
+{
+	uint64_t size = vm->page_size;
+
+	if (level == 4)
+		return size;
+	size <<= pt_shift(vm, 4);
+	if (level == 3)
+		return size;
+	size <<= pt_shift(vm, 3);
+	if (level == 2)
+		return size;
+	size <<= pt_shift(vm, 2);
+	return size;
+}
+
+static int pt_idx(struct kvm_vm *vm, uint64_t vaddr, int level, uint64_t *nls)
+{
+	switch (level) {
+	case 1:
+		*nls = 0x9;
+		return (vaddr >> 39) & 0x1fff;
+	case 2:
+		*nls = 0x9;
+		return (vaddr >> 30) & 0x1ff;
+	case 3:
+// 4K		*nls = 0x9;
+		*nls = 0x5;
+		return (vaddr >> 21) & 0x1ff;
+	case 4:
+// 4K		return (vaddr >> 12) & 0x1ff;
+		return (vaddr >> 16) & 0x1f;
+	default:
+		TEST_ASSERT(false, "Invalid page table level %d\n", level);
+		return 0;
+	}
+}
+
+static uint64_t *virt_get_pte(struct kvm_vm *vm, vm_paddr_t pt,
+			  uint64_t vaddr, int level, uint64_t *nls)
+{
+	uint64_t *page_table = addr_gpa2hva(vm, pt);
+	int idx = pt_idx(vm, vaddr, level, nls);
+
+	return &page_table[idx];
+}
+
+#define PTE_VALID	0x8000000000000000ull
+#define PTE_LEAF	0x4000000000000000ull
+#define PTE_REFERENCED	0x0000000000000100ull
+#define PTE_CHANGED	0x0000000000000080ull
+#define PTE_PRIV	0x0000000000000008ull
+#define PTE_READ	0x0000000000000004ull
+#define PTE_RW		0x0000000000000002ull
+#define PTE_EXEC	0x0000000000000001ull
+#define PTE_PAGE_MASK	0x01fffffffffff000ull
+
+#define PDE_VALID	PTE_VALID
+#define PDE_NLS		0x0000000000000011ull
+#define PDE_PT_MASK	0x0fffffffffffff00ull
+
+void virt_arch_pg_map(struct kvm_vm *vm, uint64_t gva, uint64_t gpa)
+{
+	vm_paddr_t pt = vm->pgd;
+	uint64_t *ptep, pte;
+	int level;
+
+	for (level = 1; level <= 3; level++) {
+		uint64_t nls;
+		uint64_t *pdep = virt_get_pte(vm, pt, gva, level, &nls);
+		uint64_t pde = be64_to_cpu(*pdep);
+		uint64_t *page_table;
+
+		if (pde) {
+			TEST_ASSERT((pde & PDE_VALID) && !(pde & PTE_LEAF),
+				"Invalid PDE at level: %u gva: 0x%lx pde:0x%lx\n",
+				level, gva, pde);
+			pt = pde & PDE_PT_MASK;
+			continue;
+		}
+
+		// XXX: 64K geometry does not require full pages!
+		pt = vm_phy_page_alloc(vm,
+				       KVM_GUEST_PAGE_TABLE_MIN_PADDR,
+				       vm->memslots[MEM_REGION_PT]);
+		page_table = addr_gpa2hva(vm, pt);
+		memset(page_table, 0, vm->page_size);
+		pde = PDE_VALID | nls | pt;
+		*pdep = cpu_to_be64(pde);
+	}
+
+	ptep = virt_get_pte(vm, pt, gva, level, NULL);
+	pte = be64_to_cpu(*ptep);
+
+	TEST_ASSERT(!pte,
+		"PTE already present at level: %u gva: 0x%lx pte:0x%lx\n",
+		level, gva, pte);
+
+	pte = PTE_VALID | PTE_LEAF | PTE_REFERENCED | PTE_CHANGED | PTE_PRIV | PTE_READ | PTE_RW | PTE_EXEC | (gpa & PTE_PAGE_MASK);
+	*ptep = cpu_to_be64(pte);
+}
+
+vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
+{
+	vm_paddr_t pt = vm->pgd;
+	uint64_t *ptep, pte;
+	int level;
+
+	for (level = 1; level <= 3; level++) {
+		uint64_t nls;
+		uint64_t *pdep = virt_get_pte(vm, pt, gva, level, &nls);
+		uint64_t pde = be64_to_cpu(*pdep);
+
+		TEST_ASSERT((pde & PDE_VALID) && !(pde & PTE_LEAF),
+			"PDE not present at level: %u gva: 0x%lx pde:0x%lx\n",
+			level, gva, pde);
+		pt = pde & PDE_PT_MASK;
+	}
+
+	ptep = virt_get_pte(vm, pt, gva, level, NULL);
+	pte = be64_to_cpu(*ptep);
+
+	TEST_ASSERT(pte,
+		"PTE not present at level: %u gva: 0x%lx pte:0x%lx\n",
+		level, gva, pte);
+
+	TEST_ASSERT((pte & PTE_VALID) && (pte & PTE_LEAF) && (pte & PTE_READ) && (pte & PTE_RW) && (pte & PTE_EXEC),
+		"PTE not valid at level: %u gva: 0x%lx pte:0x%lx\n",
+		level, gva, pte);
+
+	return (pte & PTE_PAGE_MASK) + (gva & (vm->page_size - 1));
+}
+
+static void virt_arch_dump_pt(FILE *stream, struct kvm_vm *vm, vm_paddr_t pt, vm_vaddr_t va, int level, uint8_t indent)
+{
+	uint64_t *page_table;
+	int size, idx;
+
+	page_table = addr_gpa2hva(vm, pt);
+	size = 1U << pt_shift(vm, level);
+	for (idx = 0; idx < size; idx++) {
+		uint64_t pte = be64_to_cpu(page_table[idx]);
+		if (pte & PTE_VALID) {
+			if (pte & PTE_LEAF) {
+				fprintf(stream, "%*sgVA:0x%016lx -> gRA:0x%016llx\n", indent, "", va, pte & PTE_PAGE_MASK);
+			} else {
+				virt_arch_dump_pt(stream, vm, pte & PDE_PT_MASK, va, level + 1, indent);
+			}
+		}
+		va += pt_entry_coverage(vm, level);
+	}
+
+}
+
+void virt_arch_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
+{
+	vm_paddr_t pt = vm->pgd;
+
+	if (!vm->pgd_created)
+		return;
+
+	virt_arch_dump_pt(stream, vm, pt, 0, 1, indent);
+}
+
+struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
+				  void *guest_code)
+{
+	size_t stack_size =  64*1024;
+	uint64_t stack_vaddr;
+	struct kvm_regs regs;
+	struct kvm_vcpu *vcpu;
+	uint64_t lpcr;
+
+	TEST_ASSERT(vm->page_size == 64*1024, "Unsupported page size: 0x%x",
+		    vm->page_size);
+
+	stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
+				       DEFAULT_GUEST_STACK_VADDR_MIN,
+				       MEM_REGION_DATA);
+
+	vcpu = __vm_vcpu_add(vm, vcpu_id);
+
+	vcpu_enable_cap(vcpu, KVM_CAP_PPC_PAPR, 1);
+
+	/* Setup guest registers */
+	vcpu_regs_get(vcpu, &regs);
+	vcpu_get_reg(vcpu, KVM_REG_PPC_LPCR_64, &lpcr);
+
+	regs.pc = (uintptr_t)guest_code;
+	regs.gpr[12] = (uintptr_t)guest_code;
+	regs.msr = 0x8000000002103032ull;
+	regs.gpr[1] = stack_vaddr + stack_size - 256;
+
+	if (BYTE_ORDER == LITTLE_ENDIAN) {
+		regs.msr |= 0x1; // LE
+		lpcr |= 0x0000000002000000; // ILE
+	} else {
+		lpcr &= ~0x0000000002000000; // !ILE
+	}
+
+	vcpu_regs_set(vcpu, &regs);
+	vcpu_set_reg(vcpu, KVM_REG_PPC_LPCR_64, lpcr);
+
+	return vcpu;
+}
+
+void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
+{
+	va_list ap;
+	struct kvm_regs regs;
+	int i;
+
+	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"
+		    "  num: %u\n",
+		    num);
+
+	va_start(ap, num);
+	vcpu_regs_get(vcpu, &regs);
+
+	for (i = 0; i < num; i++)
+		regs.gpr[i + 3] = va_arg(ap, uint64_t);
+
+	vcpu_regs_set(vcpu, &regs);
+	va_end(ap);
+}
+
+void vcpu_arch_dump(FILE *stream, struct kvm_vcpu *vcpu, uint8_t indent)
+{
+	struct kvm_regs regs;
+
+	vcpu_regs_get(vcpu, &regs);
+
+	fprintf(stream, "%*sNIA: 0x%016llx  MSR: 0x%016llx\n", indent, "", regs.pc, regs.msr);
+	fprintf(stream, "%*sLR:  0x%016llx  CTR :0x%016llx\n", indent, "", regs.lr, regs.ctr);
+	fprintf(stream, "%*sCR:  0x%08llx          XER :0x%016llx\n", indent, "", regs.cr, regs.xer);
+}
+
+void kvm_arch_vm_post_create(struct kvm_vm *vm)
+{
+	uint32_t stub[] = {
+		0x38600000,		     // li	r3,0
+		0x38800000 | UCALL_R4_EXCPT, // li	r4,UCALL_R4_EXCPT
+		0x38a00000,		     // li	r5,0
+		0x7cda02a6,		     // mfspr	r5,SRR0
+		0x7cfb02a6,		     // mfspr	r6,SRR1
+		0x44000022,		     // sc	1
+	};
+	void *mem;
+	int i;
+
+	vm_paddr_t excp_paddr = vm_phy_page_alloc(vm, 0,
+				 vm->memslots[MEM_REGION_DATA]);
+	TEST_ASSERT(excp_paddr == 0, "excp_paddr = 0x%lx\n", excp_paddr);
+
+	mem = addr_gpa2hva(vm, excp_paddr);
+
+	/* Fill with branch-to-self so SRR0/1 don't get lost */
+	/* XXX: this requires 2 pages on 4K */
+	for (i = 0x100; i < 0x2000; i += 0x20) {
+		stub[2] = 0x38a00000 | i;	// li	r5,i
+		memcpy(mem + i, stub, sizeof(stub));
+	}
+}
+
+bool (*interrupt_handler)(struct kvm_vcpu *vcpu, unsigned trap);
+
+void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	struct kvm_regs regs;
+
+	if (!(run->exit_reason == KVM_EXIT_PAPR_HCALL &&
+	    run->papr_hcall.nr == H_UCALL)) {
+		return;
+	}
+	vcpu_regs_get(vcpu, &regs);
+	if (regs.gpr[4] != UCALL_R4_EXCPT)
+		return;
+
+	if (interrupt_handler) {
+		if (interrupt_handler(vcpu, regs.gpr[5]))
+			return; // handled
+	}
+
+	TEST_FAIL("Unhandled exception 0x%llx at NIA:0x%016llx MSR:0x%016llx\n",
+			regs.gpr[5], regs.gpr[6], regs.gpr[7]);
+}
diff --git a/tools/testing/selftests/kvm/lib/powerpc/ucall.c b/tools/testing/selftests/kvm/lib/powerpc/ucall.c
new file mode 100644
index 000000000000..ce0ddde45fef
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/powerpc/ucall.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ucall support. A ucall is a "hypercall to host userspace".
+ */
+#include "kvm_util.h"
+#include "hcall.h"
+
+void ucall_arch_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa)
+{
+}
+
+void ucall_arch_do_ucall(vm_vaddr_t uc)
+{
+	hcall2(H_UCALL, UCALL_R4_UCALL, (uintptr_t)(uc));
+}
+
+void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+
+	if (run->exit_reason == KVM_EXIT_PAPR_HCALL &&
+	    run->papr_hcall.nr == H_UCALL) {
+		struct kvm_regs regs;
+
+		vcpu_regs_get(vcpu, &regs);
+		if (regs.gpr[4] == UCALL_R4_UCALL)
+			return (void *)regs.gpr[5];
+	}
+	return NULL;
+}
-- 
2.37.2

[PATCH 2/2] KVM: PPC: Add basic framework tests for kvm selftests

[Nicholas Piggin]

Add some tests that exercise basic functions of the kvm selftests
framework, guest creation, ucalls, hcalls, copying data between guest
and host, interrupts and page faults.

These don't test KVM so much, but were useful when developing the
selftests support for powerpc.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tools/testing/selftests/kvm/Makefile          |   2 +
 .../selftests/kvm/include/powerpc/hcall.h     |   4 +-
 .../testing/selftests/kvm/powerpc/null_test.c | 186 ++++++++++++++++++
 .../selftests/kvm/powerpc/rtas_hcall.c        | 146 ++++++++++++++
 4 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/kvm/powerpc/null_test.c
 create mode 100644 tools/testing/selftests/kvm/powerpc/rtas_hcall.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 081cee3ecc0c..1d9eb4f3284d 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -180,6 +180,8 @@ TEST_GEN_PROGS_riscv += kvm_page_table_test
 TEST_GEN_PROGS_riscv += set_memory_region_test
 TEST_GEN_PROGS_riscv += kvm_binary_stats_test
 
+TEST_GEN_PROGS_powerpc += powerpc/null_test
+TEST_GEN_PROGS_powerpc += powerpc/rtas_hcall
 TEST_GEN_PROGS_powerpc += demand_paging_test
 TEST_GEN_PROGS_powerpc += dirty_log_test
 TEST_GEN_PROGS_powerpc += kvm_create_max_vcpus
diff --git a/tools/testing/selftests/kvm/include/powerpc/hcall.h b/tools/testing/selftests/kvm/include/powerpc/hcall.h
index bbad5904f37a..cbcaf180c427 100644
--- a/tools/testing/selftests/kvm/include/powerpc/hcall.h
+++ b/tools/testing/selftests/kvm/include/powerpc/hcall.h
@@ -11,7 +11,9 @@
 #define H_UCALL	0
 #define UCALL_R4_UCALL	0x5715 // regular ucall, r5 contains ucall pointer
 #define UCALL_R4_EXCPT	0x1b0f // other exception, r5 contains vector, r6,7 SRRs
-			       // R4==0 is a simple asm exit
+#define UCALL_R4_SIMPLE	0x0000 // simple exit usable by asm with no ucall data
+
+#define H_RTAS		0xf000
 
 int64_t hcall0(uint64_t token);
 int64_t hcall1(uint64_t token, uint64_t arg1);
diff --git a/tools/testing/selftests/kvm/powerpc/null_test.c b/tools/testing/selftests/kvm/powerpc/null_test.c
new file mode 100644
index 000000000000..ee3cf20a5cf3
--- /dev/null
+++ b/tools/testing/selftests/kvm/powerpc/null_test.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Tests for guest creation, run, ucall, interrupt, and vm dumping.
+ */
+
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "kselftest.h"
+#include "processor.h"
+
+extern void guest_code_asm(void);
+asm(".global guest_code_asm");
+asm(".balign 4");
+asm("guest_code_asm:");
+asm("li 3,0"); // H_UCALL
+asm("li 4,0"); // UCALL_R4_SIMPLE
+asm("sc 1");
+
+static void guest_code_ucall(void)
+{
+	GUEST_DONE();
+}
+
+static void guest_code_trap(void)
+{
+	asm volatile("trap");
+}
+
+static void guest_code_dsi(void)
+{
+	*(volatile int *)0;
+}
+
+static void test_asm(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_asm);
+
+	ret = _vcpu_run(vcpu);
+
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+	TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_NONE,
+		    "Invalid guest done status: exit_reason=%s\n",
+		    exit_reason_str(vcpu->run->exit_reason));
+
+	kvm_vm_free(vm);
+}
+
+static void test_ucall(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_ucall);
+
+	ret = _vcpu_run(vcpu);
+
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+	TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_DONE,
+		    "Invalid guest done status: exit_reason=%s\n",
+		    exit_reason_str(vcpu->run->exit_reason));
+
+	kvm_vm_free(vm);
+}
+
+static bool got_trap;
+static bool trap_handler(struct kvm_vcpu *vcpu, unsigned trap)
+{
+	if (trap == 0x700) {
+		got_trap = true;
+		return true;
+	}
+	return false;
+}
+
+static void test_trap(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	interrupt_handler = trap_handler;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_trap);
+
+	ret = _vcpu_run(vcpu);
+
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+	TEST_ASSERT(got_trap,
+		    "Invalid guest done status: exit_reason=%s\n",
+		    exit_reason_str(vcpu->run->exit_reason));
+
+	kvm_vm_free(vm);
+
+	interrupt_handler = NULL;
+}
+
+static bool got_dsi;
+static bool dsi_handler(struct kvm_vcpu *vcpu, unsigned trap)
+{
+	if (trap == 0x300) {
+		got_dsi = true;
+		return true;
+	}
+	return false;
+}
+
+static void test_dsi(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	interrupt_handler = dsi_handler;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_dsi);
+
+	ret = _vcpu_run(vcpu);
+
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+	TEST_ASSERT(got_dsi,
+		    "Invalid guest done status: exit_reason=%s\n",
+		    exit_reason_str(vcpu->run->exit_reason));
+
+	vm_dump(stderr, vm, 2);
+
+	kvm_vm_free(vm);
+
+	interrupt_handler = NULL;
+}
+
+static void test_dump(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int ret;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code_ucall);
+
+	ret = _vcpu_run(vcpu);
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+
+	printf("Testing vm_dump...\n");
+	vm_dump(stderr, vm, 2);
+
+	kvm_vm_free(vm);
+}
+
+
+struct testdef {
+	const char *name;
+	void (*test)(void);
+} testlist[] = {
+	{ "null asm test", test_asm},
+	{ "null ucall test", test_ucall},
+	{ "trap test", test_trap},
+	{ "page fault test", test_dsi},
+	{ "vm dump test", test_dump},
+};
+
+int main(int argc, char *argv[])
+{
+	int idx;
+
+	ksft_print_header();
+
+	ksft_set_plan(ARRAY_SIZE(testlist));
+
+	for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
+		testlist[idx].test();
+		ksft_test_result_pass("%s\n", testlist[idx].name);
+	}
+
+	ksft_finished();	/* Print results and exit() accordingly */
+}
diff --git a/tools/testing/selftests/kvm/powerpc/rtas_hcall.c b/tools/testing/selftests/kvm/powerpc/rtas_hcall.c
new file mode 100644
index 000000000000..17a580d7fa55
--- /dev/null
+++ b/tools/testing/selftests/kvm/powerpc/rtas_hcall.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Test the KVM H_RTAS hcall and copying buffers between guest and host.
+ */
+
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "kselftest.h"
+#include "hcall.h"
+
+struct rtas_args {
+	__be32 token;
+	__be32 nargs;
+	__be32 nret;
+	__be32 args[16];
+        __be32 *rets;     /* Pointer to return values in args[]. */
+};
+
+static void guest_code(void)
+{
+	struct rtas_args r;
+	int64_t rc;
+
+	r.token = cpu_to_be32(0xdeadbeef);
+	r.nargs = cpu_to_be32(3);
+	r.nret = cpu_to_be32(2);
+	r.rets = &r.args[3];
+	r.args[0] = cpu_to_be32(0x1000);
+	r.args[1] = cpu_to_be32(0x1001);
+	r.args[2] = cpu_to_be32(0x1002);
+	rc = hcall1(H_RTAS, (uint64_t)&r);
+	GUEST_ASSERT(rc == 0);
+	GUEST_ASSERT_1(be32_to_cpu(r.rets[0]) == 0xabc, be32_to_cpu(r.rets[0]));
+	GUEST_ASSERT_1(be32_to_cpu(r.rets[1]) == 0x123, be32_to_cpu(r.rets[1]));
+
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_regs regs;
+	struct rtas_args *r;
+	vm_vaddr_t rtas_vaddr;
+	struct ucall uc;
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	uint64_t tmp;
+	int ret;
+
+	ksft_print_header();
+
+	ksft_set_plan(1);
+
+	/* Create VM */
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	printf("Running H_RTAS guest vcpu.\n");
+
+	ret = _vcpu_run(vcpu);
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+	switch ((tmp = get_ucall(vcpu, &uc))) {
+	case UCALL_NONE:
+		break; // good
+	case UCALL_DONE:
+		TEST_FAIL("Unexpected final guest exit %lu\n", tmp);
+		break;
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT_N(uc, "values: %lu (0x%lx)\n",
+				      GUEST_ASSERT_ARG(uc, 0),
+				      GUEST_ASSERT_ARG(uc, 0));
+		break;
+	default:
+		TEST_FAIL("Unexpected guest exit %lu\n", tmp);
+	}
+
+	TEST_ASSERT(vcpu->run->exit_reason == KVM_EXIT_PAPR_HCALL,
+		    "Expected PAPR_HCALL exit, got %s\n",
+		    exit_reason_str(vcpu->run->exit_reason));
+	TEST_ASSERT(vcpu->run->papr_hcall.nr == H_RTAS,
+		    "Expected H_RTAS exit, got %lld\n",
+		    vcpu->run->papr_hcall.nr);
+
+	printf("Got H_RTAS exit.\n");
+
+	vcpu_regs_get(vcpu, &regs);
+	rtas_vaddr = regs.gpr[4];
+	printf("H_RTAS rtas_args at gEA=0x%lx\n", rtas_vaddr);
+
+	r = addr_gva2hva(vm, rtas_vaddr);
+
+	TEST_ASSERT(r->token == cpu_to_be32(0xdeadbeef),
+		    "Expected RTAS token 0xdeadbeef, got 0x%x\n",
+		    be32_to_cpu(r->token));
+	TEST_ASSERT(r->nargs == cpu_to_be32(3),
+		    "Expected RTAS nargs 3, got %u\n",
+		    be32_to_cpu(r->nargs));
+	TEST_ASSERT(r->nret == cpu_to_be32(2),
+		    "Expected RTAS nret 2, got %u\n",
+		    be32_to_cpu(r->nret));
+	TEST_ASSERT(r->args[0] == cpu_to_be32(0x1000),
+		    "Expected args[0] to be 0x1000, got 0x%x\n",
+		    be32_to_cpu(r->args[0]));
+	TEST_ASSERT(r->args[1] == cpu_to_be32(0x1001),
+		    "Expected args[1] to be 0x1001, got 0x%x\n",
+		    be32_to_cpu(r->args[1]));
+	TEST_ASSERT(r->args[2] == cpu_to_be32(0x1002),
+		    "Expected args[2] to be 0x1002, got 0x%x\n",
+		    be32_to_cpu(r->args[2]));
+
+	printf("Guest rtas_args is correct, setting rets.\n");
+
+	r->args[3] = cpu_to_be32(0xabc);
+	r->args[4] = cpu_to_be32(0x123);
+
+	regs.gpr[3] = 0;
+	vcpu_regs_set(vcpu, &regs);
+
+	printf("Running H_RTAS guest vcpu again (hcall return H_SUCCESS).\n");
+
+	ret = _vcpu_run(vcpu);
+	TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
+	switch ((tmp = get_ucall(vcpu, &uc))) {
+	case UCALL_DONE:
+		printf("Got final guest exit.\n");
+		break;
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT_N(uc, "values: %lu (0x%lx)\n",
+				      GUEST_ASSERT_ARG(uc, 0),
+				      GUEST_ASSERT_ARG(uc, 0));
+		break;
+	default:
+		TEST_FAIL("Unexpected guest exit %lu\n", tmp);
+	}
+
+	kvm_vm_free(vm);
+
+	ksft_test_result_pass("%s\n", "null test");
+	ksft_finished();	/* Print results and exit() accordingly */
+}
-- 
2.37.2

Re: [PATCH 0/2] KVM: PPC: support kvm selftests

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> Hi,
>
> This series adds initial KVM selftests support for powerpc
> (64-bit, BookS).

Awesome.
 
> It spans 3 maintainers but it does not really
> affect arch/powerpc, and it is well contained in selftests
> code, just touches some makefiles and a tiny bit headers so
> conflicts should be unlikely and trivial.
>
> I guess Paolo is the best point to merge these, if no comments
> or objections?

Yeah. If it helps:

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

Re: [PATCH 0/2] KVM: PPC: support kvm selftests

[Sean Christopherson]

On Thu, Mar 16, 2023, Michael Ellerman wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> > Hi,
> >
> > This series adds initial KVM selftests support for powerpc
> > (64-bit, BookS).
> 
> Awesome.
>  
> > It spans 3 maintainers but it does not really
> > affect arch/powerpc, and it is well contained in selftests
> > code, just touches some makefiles and a tiny bit headers so
> > conflicts should be unlikely and trivial.
> >
> > I guess Paolo is the best point to merge these, if no comments
> > or objections?
> 
> Yeah. If it helps:
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

What is the long term plan for KVM PPC maintenance?  I was under the impression
that KVM PPC was trending toward "bug fixes only", but the addition of selftests
support suggests otherwise.

I ask primarily because routing KVM PPC patches through the PPC tree is going to
be problematic if KVM PPC sees signficiant development.  The current situation is
ok because the volume of patches is low and KVM PPC isn't trying to drive anything
substantial into common KVM code, but if that changes... 

My other concern is that for selftests specifically, us KVM folks are taking on
more maintenance burden by supporting PPC.  AFAIK, none of the people that focus
on KVM selftests in any meaningful capacity have access to PPC hardware, let alone
know enough about the architecture to make intelligent code changes.

Don't get me wrong, I'm very much in favor of more testing, I just don't want KVM
to get left holding the bag.

Re: [PATCH 0/2] KVM: PPC: support kvm selftests

[Nicholas Piggin]

On Thu Mar 23, 2023 at 3:41 AM AEST, Sean Christopherson wrote:
> On Thu, Mar 16, 2023, Michael Ellerman wrote:
> > Nicholas Piggin <npiggin@gmail.com> writes:
> > > Hi,
> > >
> > > This series adds initial KVM selftests support for powerpc
> > > (64-bit, BookS).
> > 
> > Awesome.
> >  
> > > It spans 3 maintainers but it does not really
> > > affect arch/powerpc, and it is well contained in selftests
> > > code, just touches some makefiles and a tiny bit headers so
> > > conflicts should be unlikely and trivial.
> > >
> > > I guess Paolo is the best point to merge these, if no comments
> > > or objections?
> > 
> > Yeah. If it helps:
> > 
> > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
>
> What is the long term plan for KVM PPC maintenance?  I was under the impression
> that KVM PPC was trending toward "bug fixes only", but the addition of selftests
> support suggests otherwise.

We plan to continue maintaining it. New support and features has been a
bit low in the past couple of years, hopefully that will pick up a bit
though.

> I ask primarily because routing KVM PPC patches through the PPC tree is going to
> be problematic if KVM PPC sees signficiant development.  The current situation is
> ok because the volume of patches is low and KVM PPC isn't trying to drive anything
> substantial into common KVM code, but if that changes... 

Michael has done KVM topic branches to pull from a few times when such
conflicts came up (at smaller scale). If we end up with larger changes
or regular conflicts we might start up a kvm-ppc tree again I guess.

> My other concern is that for selftests specifically, us KVM folks are taking on
> more maintenance burden by supporting PPC.  AFAIK, none of the people that focus
> on KVM selftests in any meaningful capacity have access to PPC hardware, let alone
> know enough about the architecture to make intelligent code changes.
>
> Don't get me wrong, I'm very much in favor of more testing, I just don't want KVM
> to get left holding the bag.

Understood. I'll be happy to maintain powerpc part of kvm selftests and
do any fixes that are needed for core code changes.If support fell away
you could leave it broken (or rm -rf it if you prefer) -- I wouldn't ask
anybody to work on archs they don't know or aren't paid to.

Not sure if anything more can be done to help your process or ease your
mind. It (KVM and kvm-selftests) can run in QEMU at least.

Thanks,
Nick

Re: [PATCH 0/2] KVM: PPC: support kvm selftests

[Sean Christopherson]

On Mon, Mar 27, 2023, Nicholas Piggin wrote:
> On Thu Mar 23, 2023 at 3:41 AM AEST, Sean Christopherson wrote:
> > On Thu, Mar 16, 2023, Michael Ellerman wrote:
> > > Nicholas Piggin <npiggin@gmail.com> writes:
> > > > Hi,
> > > >
> > > > This series adds initial KVM selftests support for powerpc
> > > > (64-bit, BookS).
> > > 
> > > Awesome.
> > >  
> > > > It spans 3 maintainers but it does not really
> > > > affect arch/powerpc, and it is well contained in selftests
> > > > code, just touches some makefiles and a tiny bit headers so
> > > > conflicts should be unlikely and trivial.
> > > >
> > > > I guess Paolo is the best point to merge these, if no comments
> > > > or objections?
> > > 
> > > Yeah. If it helps:
> > > 
> > > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> >
> > What is the long term plan for KVM PPC maintenance?  I was under the impression
> > that KVM PPC was trending toward "bug fixes only", but the addition of selftests
> > support suggests otherwise.
> 
> We plan to continue maintaining it. New support and features has been a
> bit low in the past couple of years, hopefully that will pick up a bit
> though.

Partly out of curiosity, but also to get a general feel for what types of changes
we might see, what are the main use cases for KVM PPC these days?  E.g. is it mainly
a vehicle for developing and testing, hosting VMs in the cloud, something else?

> > I ask primarily because routing KVM PPC patches through the PPC tree is going to
> > be problematic if KVM PPC sees signficiant development.  The current situation is
> > ok because the volume of patches is low and KVM PPC isn't trying to drive anything
> > substantial into common KVM code, but if that changes... 
> 
> Michael has done KVM topic branches to pull from a few times when such
> conflicts came up (at smaller scale). If we end up with larger changes
> or regular conflicts we might start up a kvm-ppc tree again I guess.

A wait-and-see approach works for me.  I don't have any complaints with the current
process, I was just caught off guard.

> > My other concern is that for selftests specifically, us KVM folks are taking on
> > more maintenance burden by supporting PPC.  AFAIK, none of the people that focus
> > on KVM selftests in any meaningful capacity have access to PPC hardware, let alone
> > know enough about the architecture to make intelligent code changes.
> >
> > Don't get me wrong, I'm very much in favor of more testing, I just don't want KVM
> > to get left holding the bag.
> 
> Understood. I'll be happy to maintain powerpc part of kvm selftests and
> do any fixes that are needed for core code changes.If support fell away
> you could leave it broken (or rm -rf it if you prefer) -- I wouldn't ask
> anybody to work on archs they don't know or aren't paid to.
> 
> Not sure if anything more can be done to help your process or ease your
> mind. It (KVM and kvm-selftests) can run in QEMU at least.

Updating the KVM/powerpc to include selftests would be very helpful, e.g

  F:	tools/testing/selftests/kvm/*/powerpc/
  F:	tools/testing/selftests/kvm/powerpc/

and ideally there would be one or more M: (and R:) entries as well.  I'm not
all that concerned about the selftests support being abandoned, but the lack of
specific contacts makes it look like KVM PPC is in maintenance-only mode, and it
sounds like that's not the case.

Thanks!

Re: [PATCH 0/2] KVM: PPC: support kvm selftests

[Nicholas Piggin]

On Tue Mar 28, 2023 at 3:43 AM AEST, Sean Christopherson wrote:
> On Mon, Mar 27, 2023, Nicholas Piggin wrote:
> > On Thu Mar 23, 2023 at 3:41 AM AEST, Sean Christopherson wrote:
> > > On Thu, Mar 16, 2023, Michael Ellerman wrote:
> > > > Nicholas Piggin <npiggin@gmail.com> writes:
> > > > > Hi,
> > > > >
> > > > > This series adds initial KVM selftests support for powerpc
> > > > > (64-bit, BookS).
> > > > 
> > > > Awesome.
> > > >  
> > > > > It spans 3 maintainers but it does not really
> > > > > affect arch/powerpc, and it is well contained in selftests
> > > > > code, just touches some makefiles and a tiny bit headers so
> > > > > conflicts should be unlikely and trivial.
> > > > >
> > > > > I guess Paolo is the best point to merge these, if no comments
> > > > > or objections?
> > > > 
> > > > Yeah. If it helps:
> > > > 
> > > > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> > >
> > > What is the long term plan for KVM PPC maintenance?  I was under the impression
> > > that KVM PPC was trending toward "bug fixes only", but the addition of selftests
> > > support suggests otherwise.
> > 
> > We plan to continue maintaining it. New support and features has been a
> > bit low in the past couple of years, hopefully that will pick up a bit
> > though.
>
> Partly out of curiosity, but also to get a general feel for what types of changes
> we might see, what are the main use cases for KVM PPC these days?  E.g. is it mainly
> a vehicle for developing and testing, hosting VMs in the cloud, something else?

I didn't have too much specific in mind, just generally a bit more
development activity.

As for use cases, I think broadly KVM is increasingly seen as a Linux
feature and expected to be there, even in so-called enterprise world.
For software and workflows designed around Linux virt, it can be
difficult or impossible to substitue the proprietary partitioning layer
in our firmware. So there is always someone who wants KVM for something.
It could be all of the above, and even just as a traditional hypervisor
hosting VMs in-house, there are people who would like to use KVM.

>
> > > I ask primarily because routing KVM PPC patches through the PPC tree is going to
> > > be problematic if KVM PPC sees signficiant development.  The current situation is
> > > ok because the volume of patches is low and KVM PPC isn't trying to drive anything
> > > substantial into common KVM code, but if that changes... 
> > 
> > Michael has done KVM topic branches to pull from a few times when such
> > conflicts came up (at smaller scale). If we end up with larger changes
> > or regular conflicts we might start up a kvm-ppc tree again I guess.
>
> A wait-and-see approach works for me.  I don't have any complaints with the current
> process, I was just caught off guard.

Okay. Complaints and suggestions to improve the process are always
welcome, so if something could be done better, feel free to ping
the list or powerpc maintainers offline.

> > > My other concern is that for selftests specifically, us KVM folks are taking on
> > > more maintenance burden by supporting PPC.  AFAIK, none of the people that focus
> > > on KVM selftests in any meaningful capacity have access to PPC hardware, let alone
> > > know enough about the architecture to make intelligent code changes.
> > >
> > > Don't get me wrong, I'm very much in favor of more testing, I just don't want KVM
> > > to get left holding the bag.
> > 
> > Understood. I'll be happy to maintain powerpc part of kvm selftests and
> > do any fixes that are needed for core code changes.If support fell away
> > you could leave it broken (or rm -rf it if you prefer) -- I wouldn't ask
> > anybody to work on archs they don't know or aren't paid to.
> > 
> > Not sure if anything more can be done to help your process or ease your
> > mind. It (KVM and kvm-selftests) can run in QEMU at least.
>
> Updating the KVM/powerpc to include selftests would be very helpful, e.g
>
>   F:	tools/testing/selftests/kvm/*/powerpc/
>   F:	tools/testing/selftests/kvm/powerpc/

Oh good point, I should have included that. I'll send a patch.

> and ideally there would be one or more M: (and R:) entries as well.  I'm not
> all that concerned about the selftests support being abandoned, but the lack of
> specific contacts makes it look like KVM PPC is in maintenance-only mode, and it
> sounds like that's not the case.

Yeah, I guess the intention was to bring it a bit more under general
arch/powerpc maintainership but it does look a bit odd having a top
level entry and no contacts. We'll reconsider what to do with that.

Thanks,
Nick

Re: [PATCH 0/2] KVM: PPC: support kvm selftests

[Michael Ellerman]

"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Tue Mar 28, 2023 at 3:43 AM AEST, Sean Christopherson wrote:
>> On Mon, Mar 27, 2023, Nicholas Piggin wrote:
>> > On Thu Mar 23, 2023 at 3:41 AM AEST, Sean Christopherson wrote:
...
>> > >
>> > > What is the long term plan for KVM PPC maintenance?  I was under the impression
>> > > that KVM PPC was trending toward "bug fixes only", but the addition of selftests
>> > > support suggests otherwise.
...
>
>> and ideally there would be one or more M: (and R:) entries as well.  I'm not
>> all that concerned about the selftests support being abandoned, but the lack of
>> specific contacts makes it look like KVM PPC is in maintenance-only mode, and it
>> sounds like that's not the case.
>
> Yeah, I guess the intention was to bring it a bit more under general
> arch/powerpc maintainership but it does look a bit odd having a top
> level entry and no contacts. We'll reconsider what to do with that.

Yeah I agree it ends up looking a bit weird.

The intention was just to make it clear that Paul's tree was no longer
where patches were being handled, and that they'd be handled as regular
powerpc patches.

At the time I hoped we'd relatively quickly be able to add someone as at
least a KVM "R:"eviewer, but circumstances intervened.

Hopefully we can fix that soon.

cheers

Re: [PATCH 1/2] KVM: PPC: Add kvm selftests support for powerpc

[Sean Christopherson]

On Thu, Mar 16, 2023, Nicholas Piggin wrote:
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 3ea24a5f4c43..28ece960a0bb 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -153,6 +153,7 @@ const char *vm_guest_mode_string(uint32_t i)
>  		[VM_MODE_PXXV48_4K]	= "PA-bits:ANY, VA-bits:48,  4K pages",
>  		[VM_MODE_P47V64_4K]	= "PA-bits:47,  VA-bits:64,  4K pages",
>  		[VM_MODE_P44V64_4K]	= "PA-bits:44,  VA-bits:64,  4K pages",
> +		[VM_MODE_P52V52_64K]	= "PA-bits:52,  VA-bits:52, 64K pages",
>  		[VM_MODE_P36V48_4K]	= "PA-bits:36,  VA-bits:48,  4K pages",
>  		[VM_MODE_P36V48_16K]	= "PA-bits:36,  VA-bits:48, 16K pages",
>  		[VM_MODE_P36V48_64K]	= "PA-bits:36,  VA-bits:48, 64K pages",
> @@ -178,6 +179,7 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = {
>  	[VM_MODE_PXXV48_4K]	= {  0,  0,  0x1000, 12 },
>  	[VM_MODE_P47V64_4K]	= { 47, 64,  0x1000, 12 },
>  	[VM_MODE_P44V64_4K]	= { 44, 64,  0x1000, 12 },
> +	[VM_MODE_P52V52_64K]	= { 52, 52, 0x10000, 16 },
>  	[VM_MODE_P36V48_4K]	= { 36, 48,  0x1000, 12 },
>  	[VM_MODE_P36V48_16K]	= { 36, 48,  0x4000, 14 },
>  	[VM_MODE_P36V48_64K]	= { 36, 48, 0x10000, 16 },
> @@ -279,6 +281,14 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
>  	case VM_MODE_P44V64_4K:
>  		vm->pgtable_levels = 5;
>  		break;
> +	case VM_MODE_P52V52_64K:
> +#ifdef __powerpc__
> +		TEST_ASSERT(getpagesize() == 64*1024,

This can use SZ_64K (we really need to convert a bunch of open coded stuff...)

> +			    "KVM selftests requires 64K host page size\n");

What is the actual requirement?  E.g. is it that the host and guest page sizes
must match, or is that the selftest setup itself only supports 64KiB pages?  If
it's the former, would it make sense to assert outside of the switch statement, e.g.

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 298c4372fb1a..920813a71be0 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -291,6 +291,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
 #ifdef __aarch64__
        if (vm->pa_bits != 40)
                vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
+#endif
+#ifdef __powerpc__
+       TEST_ASSERT(getpagesize() == vm->page_size, "blah blah blah");
+
 #endif
 
        vm_open(vm);

If it's the latter (selftests limitation), can you add a comment explaining the
limitation?

> +void virt_arch_pgd_alloc(struct kvm_vm *vm)
> +{
> +	struct kvm_ppc_mmuv3_cfg mmu_cfg;
> +	vm_paddr_t prtb, pgtb;
> +	uint64_t *proc_table, *page_table;
> +	size_t pgd_pages;
> +
> +	TEST_ASSERT(vm->mode == VM_MODE_P52V52_64K, "Attempt to use "
> +		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);

Please don't split quoted lines, especially when it's easily avoided, e.g.

	TEST_ASSERT(vm->mode == VM_MODE_P52V52_64K,
		    "PPC doesn't support guest mode '0x%x', vm->mode);

> +
> +	/* If needed, create page table */
> +	if (vm->pgd_created)
> +		return;

Heh, every arch has this.  Any objection to moving the check to virt_pgd_alloc()
as a prep patch?

> +
> +	prtb = vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
> +				 vm->memslots[MEM_REGION_PT]);
> +	proc_table = addr_gpa2hva(vm, prtb);
> +	memset(proc_table, 0, vm->page_size);
> +	vm->prtb = prtb;
> +
> +	pgd_pages = 1UL << ((RADIX_PGD_INDEX_SIZE + 3) >> vm->page_shift);
> +	TEST_ASSERT(pgd_pages == 1, "PGD allocation must be single page");
> +	pgtb = vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR,
> +				 vm->memslots[MEM_REGION_PT]);
> +	page_table = addr_gpa2hva(vm, pgtb);
> +	memset(page_table, 0, vm->page_size * pgd_pages);
> +	vm->pgd = pgtb;
> +
> +	/* Set the base page directory in the proc table */
> +	proc_table[0] = cpu_to_be64(pgtb | RTS | RADIX_PGD_INDEX_SIZE);
> +
> +	mmu_cfg.process_table = prtb | 0x8000000000000000UL | 0x4; // 64K size
> +	mmu_cfg.flags = KVM_PPC_MMUV3_RADIX | KVM_PPC_MMUV3_GTSE;
> +
> +	vm_ioctl(vm, KVM_PPC_CONFIGURE_V3_MMU, &mmu_cfg);
> +
> +	vm->pgd_created = true;
> +}
> +
> +static int pt_shift(struct kvm_vm *vm, int level)
> +{
> +	switch (level) {
> +	case 1:
> +		return 13;
> +	case 2:
> +	case 3:
> +		return 9;
> +	case 4:
> +		return 5;
> +	default:
> +		TEST_ASSERT(false, "Invalid page table level %d\n", level);
> +		return 0;
> +	}
> +}
> +
> +static uint64_t pt_entry_coverage(struct kvm_vm *vm, int level)
> +{
> +	uint64_t size = vm->page_size;
> +
> +	if (level == 4)
> +		return size;
> +	size <<= pt_shift(vm, 4);
> +	if (level == 3)
> +		return size;
> +	size <<= pt_shift(vm, 3);
> +	if (level == 2)
> +		return size;
> +	size <<= pt_shift(vm, 2);
> +	return size;
> +}
> +
> +static int pt_idx(struct kvm_vm *vm, uint64_t vaddr, int level, uint64_t *nls)
> +{
> +	switch (level) {
> +	case 1:
> +		*nls = 0x9;
> +		return (vaddr >> 39) & 0x1fff;
> +	case 2:
> +		*nls = 0x9;
> +		return (vaddr >> 30) & 0x1ff;
> +	case 3:
> +// 4K		*nls = 0x9;
> +		*nls = 0x5;
> +		return (vaddr >> 21) & 0x1ff;
> +	case 4:
> +// 4K		return (vaddr >> 12) & 0x1ff;
> +		return (vaddr >> 16) & 0x1f;
> +	default:
> +		TEST_ASSERT(false, "Invalid page table level %d\n", level);
> +		return 0;
> +	}
> +}
> +
> +static uint64_t *virt_get_pte(struct kvm_vm *vm, vm_paddr_t pt,
> +			  uint64_t vaddr, int level, uint64_t *nls)
> +{
> +	uint64_t *page_table = addr_gpa2hva(vm, pt);
> +	int idx = pt_idx(vm, vaddr, level, nls);
> +
> +	return &page_table[idx];
> +}
> +
> +#define PTE_VALID	0x8000000000000000ull
> +#define PTE_LEAF	0x4000000000000000ull
> +#define PTE_REFERENCED	0x0000000000000100ull
> +#define PTE_CHANGED	0x0000000000000080ull
> +#define PTE_PRIV	0x0000000000000008ull
> +#define PTE_READ	0x0000000000000004ull
> +#define PTE_RW		0x0000000000000002ull
> +#define PTE_EXEC	0x0000000000000001ull
> +#define PTE_PAGE_MASK	0x01fffffffffff000ull
> +
> +#define PDE_VALID	PTE_VALID
> +#define PDE_NLS		0x0000000000000011ull
> +#define PDE_PT_MASK	0x0fffffffffffff00ull
> +
> +void virt_arch_pg_map(struct kvm_vm *vm, uint64_t gva, uint64_t gpa)
> +{
> +	vm_paddr_t pt = vm->pgd;
> +	uint64_t *ptep, pte;
> +	int level;
> +
> +	for (level = 1; level <= 3; level++) {
> +		uint64_t nls;
> +		uint64_t *pdep = virt_get_pte(vm, pt, gva, level, &nls);
> +		uint64_t pde = be64_to_cpu(*pdep);
> +		uint64_t *page_table;
> +
> +		if (pde) {
> +			TEST_ASSERT((pde & PDE_VALID) && !(pde & PTE_LEAF),
> +				"Invalid PDE at level: %u gva: 0x%lx pde:0x%lx\n",
> +				level, gva, pde);
> +			pt = pde & PDE_PT_MASK;
> +			continue;
> +		}
> +
> +		// XXX: 64K geometry does not require full pages!
> +		pt = vm_phy_page_alloc(vm,
> +				       KVM_GUEST_PAGE_TABLE_MIN_PADDR,
> +				       vm->memslots[MEM_REGION_PT]);
> +		page_table = addr_gpa2hva(vm, pt);
> +		memset(page_table, 0, vm->page_size);
> +		pde = PDE_VALID | nls | pt;
> +		*pdep = cpu_to_be64(pde);
> +	}
> +
> +	ptep = virt_get_pte(vm, pt, gva, level, NULL);
> +	pte = be64_to_cpu(*ptep);
> +
> +	TEST_ASSERT(!pte,
> +		"PTE already present at level: %u gva: 0x%lx pte:0x%lx\n",
> +		level, gva, pte);
> +
> +	pte = PTE_VALID | PTE_LEAF | PTE_REFERENCED | PTE_CHANGED | PTE_PRIV | PTE_READ | PTE_RW | PTE_EXEC | (gpa & PTE_PAGE_MASK);

Please wrap at 80 chars when it's convenient.  The general/unofficial style in
KVM is to honor the old 80 char limit unless there's a good reason not to.  E.g.
wrapping a line just because the terminating semicolon bumped past 80 is absurd.

> +	*ptep = cpu_to_be64(pte);
> +}
> +
> +vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> +{
> +	vm_paddr_t pt = vm->pgd;
> +	uint64_t *ptep, pte;
> +	int level;
> +
> +	for (level = 1; level <= 3; level++) {
> +		uint64_t nls;
> +		uint64_t *pdep = virt_get_pte(vm, pt, gva, level, &nls);
> +		uint64_t pde = be64_to_cpu(*pdep);
> +
> +		TEST_ASSERT((pde & PDE_VALID) && !(pde & PTE_LEAF),
> +			"PDE not present at level: %u gva: 0x%lx pde:0x%lx\n",
> +			level, gva, pde);
> +		pt = pde & PDE_PT_MASK;
> +	}
> +
> +	ptep = virt_get_pte(vm, pt, gva, level, NULL);
> +	pte = be64_to_cpu(*ptep);
> +
> +	TEST_ASSERT(pte,
> +		"PTE not present at level: %u gva: 0x%lx pte:0x%lx\n",
> +		level, gva, pte);
> +
> +	TEST_ASSERT((pte & PTE_VALID) && (pte & PTE_LEAF) && (pte & PTE_READ) && (pte & PTE_RW) && (pte & PTE_EXEC),

Wrap here as well.

> +		"PTE not valid at level: %u gva: 0x%lx pte:0x%lx\n",
> +		level, gva, pte);
> +
> +	return (pte & PTE_PAGE_MASK) + (gva & (vm->page_size - 1));
> +}
> +
> +static void virt_arch_dump_pt(FILE *stream, struct kvm_vm *vm, vm_paddr_t pt, vm_vaddr_t va, int level, uint8_t indent)

And here.  Actually, why bother with the helper?  There's one caller, and that
callers checks pgd_created, i.e. is already assuming its dumping only page tables.
Ooh, nevermind, it's recursive.

Can you drop "arch" from the name?  Selftests uses "arch" to tag functions that
are provided by arch code for use in generic code.

> +{
> +	uint64_t *page_table;
> +	int size, idx;
> +
> +	page_table = addr_gpa2hva(vm, pt);
> +	size = 1U << pt_shift(vm, level);
> +	for (idx = 0; idx < size; idx++) {
> +		uint64_t pte = be64_to_cpu(page_table[idx]);

Newline after variable declaration.

> +		if (pte & PTE_VALID) {
> +			if (pte & PTE_LEAF) {

Curly braces aren't necessary.

> +				fprintf(stream, "%*sgVA:0x%016lx -> gRA:0x%016llx\n", indent, "", va, pte & PTE_PAGE_MASK);

Probably worth wrapping here too.

> +			} else {
> +				virt_arch_dump_pt(stream, vm, pte & PDE_PT_MASK, va, level + 1, indent);
> +			}
> +		}
> +		va += pt_entry_coverage(vm, level);

The shift is constant for vm+level, correct?  In that case, can't this be written
as

	for (idx = 0; idx < size; idx++, va += va_coverage) {

or even without a snapshot

	for (idx = 0; idx < size; idx++, va += pt_entry_coverage(vm, level)) {

That would allow

		if (!(pte & PTE_VALID)
			continue

to reduce the indentation of the printing.

> +	}
> +
> +}
> +
> +void virt_arch_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
> +{
> +	vm_paddr_t pt = vm->pgd;
> +
> +	if (!vm->pgd_created)
> +		return;
> +
> +	virt_arch_dump_pt(stream, vm, pt, 0, 1, indent);
> +}
> +
> +struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id,
> +				  void *guest_code)
> +{
> +	size_t stack_size =  64*1024;

SZ_64K

> +	uint64_t stack_vaddr;
> +	struct kvm_regs regs;
> +	struct kvm_vcpu *vcpu;
> +	uint64_t lpcr;
> +
> +	TEST_ASSERT(vm->page_size == 64*1024, "Unsupported page size: 0x%x",

SZ_64K

> +		    vm->page_size);
> +
> +	stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
> +				       DEFAULT_GUEST_STACK_VADDR_MIN,
> +				       MEM_REGION_DATA);
> +
> +	vcpu = __vm_vcpu_add(vm, vcpu_id);
> +
> +	vcpu_enable_cap(vcpu, KVM_CAP_PPC_PAPR, 1);
> +
> +	/* Setup guest registers */
> +	vcpu_regs_get(vcpu, &regs);
> +	vcpu_get_reg(vcpu, KVM_REG_PPC_LPCR_64, &lpcr);
> +
> +	regs.pc = (uintptr_t)guest_code;
> +	regs.gpr[12] = (uintptr_t)guest_code;
> +	regs.msr = 0x8000000002103032ull;
> +	regs.gpr[1] = stack_vaddr + stack_size - 256;
> +
> +	if (BYTE_ORDER == LITTLE_ENDIAN) {
> +		regs.msr |= 0x1; // LE
> +		lpcr |= 0x0000000002000000; // ILE

Would it be appropriate to add #defines to processor.h instead of open coding the
magic numbers?

> +	} else {
> +		lpcr &= ~0x0000000002000000; // !ILE
> +	}
> +
> +	vcpu_regs_set(vcpu, &regs);
> +	vcpu_set_reg(vcpu, KVM_REG_PPC_LPCR_64, lpcr);
> +
> +	return vcpu;
> +}
> +
> +void vcpu_args_set(struct kvm_vcpu *vcpu, unsigned int num, ...)
> +{
> +	va_list ap;
> +	struct kvm_regs regs;
> +	int i;
> +
> +	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args,\n"

Newlines in TEST_ASSERT() usually lead to weird formatting.

> +		    "  num: %u\n",

No quoted line wrap please.  And in this case, not wrapping is better IMO.

	TEST_ASSERT(num >= 1 && num <= 5, "Unsupported number of args: %u", num);

> +		    num);
> +
> +	va_start(ap, num);
> +	vcpu_regs_get(vcpu, &regs);
> +
> +	for (i = 0; i < num; i++)
> +		regs.gpr[i + 3] = va_arg(ap, uint64_t);
> +
> +	vcpu_regs_set(vcpu, &regs);
> +	va_end(ap);
> +}

Re: [PATCH 1/2] KVM: PPC: Add kvm selftests support for powerpc

[Nicholas Piggin]

Hey thanks for the review. Points about formatting and style all
valid, I'll tidy those up. For the others,

On Thu Mar 30, 2023 at 6:19 AM AEST, Sean Christopherson wrote:
> On Thu, Mar 16, 2023, Nicholas Piggin wrote:
> > +#ifdef __powerpc__
> > +		TEST_ASSERT(getpagesize() == 64*1024,
>
> This can use SZ_64K (we really need to convert a bunch of open coded stuff...)
>
> > +			    "KVM selftests requires 64K host page size\n");
>
> What is the actual requirement?  E.g. is it that the host and guest page sizes
> must match, or is that the selftest setup itself only supports 64KiB pages?  If
> it's the former, would it make sense to assert outside of the switch statement, e.g.
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 298c4372fb1a..920813a71be0 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -291,6 +291,10 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
>  #ifdef __aarch64__
>         if (vm->pa_bits != 40)
>                 vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
> +#endif
> +#ifdef __powerpc__
> +       TEST_ASSERT(getpagesize() == vm->page_size, "blah blah blah");
> +
>  #endif
>  
>         vm_open(vm);
>
> If it's the latter (selftests limitation), can you add a comment explaining the
> limitation?

It's the selftests setup, requires both host and guest to be 64k page
size. I think it shouldn't be *too* hard to add any mix of 64k/4k, but
there are a few quirks like requiring pgd to have 64k size allocation.
64/64 is the most important for us, but it would be nice to get other
combos working soon if nothing else than because they don't get as much
testing in other ways.

I can add a comment.

> > +
> > +	/* If needed, create page table */
> > +	if (vm->pgd_created)
> > +		return;
>
> Heh, every arch has this.  Any objection to moving the check to virt_pgd_alloc()
> as a prep patch?

I have no objection, I can do that for the next spin.

> > +		"PTE not valid at level: %u gva: 0x%lx pte:0x%lx\n",
> > +		level, gva, pte);
> > +
> > +	return (pte & PTE_PAGE_MASK) + (gva & (vm->page_size - 1));
> > +}
> > +
> > +static void virt_arch_dump_pt(FILE *stream, struct kvm_vm *vm, vm_paddr_t pt, vm_vaddr_t va, int level, uint8_t indent)
>
> And here.  Actually, why bother with the helper?  There's one caller, and that
> callers checks pgd_created, i.e. is already assuming its dumping only page tables.
> Ooh, nevermind, it's recursive.
>
> Can you drop "arch" from the name?  Selftests uses "arch" to tag functions that
> are provided by arch code for use in generic code.

Yeah agree, I'll drop that.

> > +			} else {
> > +				virt_arch_dump_pt(stream, vm, pte & PDE_PT_MASK, va, level + 1, indent);
> > +			}
> > +		}
> > +		va += pt_entry_coverage(vm, level);
>
> The shift is constant for vm+level, correct?  In that case, can't this be written
> as
>
> 	for (idx = 0; idx < size; idx++, va += va_coverage) {
>
> or even without a snapshot
>
> 	for (idx = 0; idx < size; idx++, va += pt_entry_coverage(vm, level)) {
>
> That would allow
>
> 		if (!(pte & PTE_VALID)
> 			continue
>
> to reduce the indentation of the printing.

It is constant for a given (vm, level). Good thinking, that should work.

> > +	stack_vaddr = __vm_vaddr_alloc(vm, stack_size,
> > +				       DEFAULT_GUEST_STACK_VADDR_MIN,
> > +				       MEM_REGION_DATA);
> > +
> > +	vcpu = __vm_vcpu_add(vm, vcpu_id);
> > +
> > +	vcpu_enable_cap(vcpu, KVM_CAP_PPC_PAPR, 1);
> > +
> > +	/* Setup guest registers */
> > +	vcpu_regs_get(vcpu, &regs);
> > +	vcpu_get_reg(vcpu, KVM_REG_PPC_LPCR_64, &lpcr);
> > +
> > +	regs.pc = (uintptr_t)guest_code;
> > +	regs.gpr[12] = (uintptr_t)guest_code;
> > +	regs.msr = 0x8000000002103032ull;
> > +	regs.gpr[1] = stack_vaddr + stack_size - 256;
> > +
> > +	if (BYTE_ORDER == LITTLE_ENDIAN) {
> > +		regs.msr |= 0x1; // LE
> > +		lpcr |= 0x0000000002000000; // ILE
>
> Would it be appropriate to add #defines to processor.h instead of open coding the
> magic numbers?

Yes it would. I should have not been lazy about it from the start, will
fix.

(Other comments snipped but agreed for all)

Thanks,
Nick