Re: [PATCH v3 22/33] KVM: PPC: Book3S HV: Handle page fault for a nested guest

On Thu, Oct 04, 2018 at 07:21:20PM +1000, Paul Mackerras wrote:

On Wed, Oct 03, 2018 at 03:39:13PM +1000, David Gibson wrote:

quoted

On Tue, Oct 02, 2018 at 09:31:21PM +1000, Paul Mackerras wrote:

quoted

From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

@@ -367,7 +367,9 @@ struct kvmppc_pte {
 	bool may_write		: 1;
 	bool may_execute	: 1;
 	unsigned long wimg;
+	unsigned long rc;
 	u8 page_size;		/* MMU_PAGE_xxx */
+	u16 page_shift;

It's a bit ugly that this has both page_size and page_shift, which is
redundant information AFAICT.  Also, why does page_shift need to be
u16 - given that 2^255 bytes is much more than our supported address
space, let alone a plausible page size.

These values are all essentially function outputs, so I don't think
it's ugly to have the same information in different forms.  I actually
don't like using the MMU_PAGE_xxx values, because the information in
the mmu_psize_defs[] array depends on the MMU mode of the host, but
KVM needs to be able to work with guests in both MMU modes.  More
generally I don't think it's a good idea that the KVM <-> guest
interface depends so much on what the host firmware tells us about the
physical machine we're on.  Thus I'm trying to move away from using
MMU_PSIZE_xxx values and mmu_psize_defs[] in KVM code.

Fair enough.

I'll change the type to u8.

quoted

quoted

diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index bd06a95..ee6f493 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c

@@ -29,43 +29,16 @@
  */
 static int p9_supported_radix_bits[4] = { 5, 9, 9, 13 };
 
-/*
- * Used to walk a partition or process table radix tree in guest memory
- * Note: We exploit the fact that a partition table and a process
- * table have the same layout, a partition-scoped page table and a
- * process-scoped page table have the same layout, and the 2nd
- * doubleword of a partition table entry has the same layout as
- * the PTCR register.
- */
-int kvmppc_mmu_radix_translate_table(struct kvm_vcpu *vcpu, gva_t eaddr,
-				     struct kvmppc_pte *gpte, u64 table,
-				     int table_index, u64 *pte_ret_p)
+int kvmppc_mmu_walk_radix_tree(struct kvm_vcpu *vcpu, gva_t eaddr,
+			       struct kvmppc_pte *gpte, u64 root,
+			       u64 *pte_ret_p)
 {
 	struct kvm *kvm = vcpu->kvm;
 	int ret, level, ps;
-	unsigned long ptbl, root;
-	unsigned long rts, bits, offset;
-	unsigned long size, index;
-	struct prtb_entry entry;
+	unsigned long rts, bits, offset, index;
 	u64 pte, base, gpa;
 	__be64 rpte;
 
-	if ((table & PRTS_MASK) > 24)
-		return -EINVAL;
-	size = 1ul << ((table & PRTS_MASK) + 12);
-
-	/* Is the table big enough to contain this entry? */
-	if ((table_index * sizeof(entry)) >= size)
-		return -EINVAL;
-
-	/* Read the table to find the root of the radix tree */
-	ptbl = (table & PRTB_MASK) + (table_index * sizeof(entry));
-	ret = kvm_read_guest(kvm, ptbl, &entry, sizeof(entry));
-	if (ret)
-		return ret;
-
-	/* Root is stored in the first double word */
-	root = be64_to_cpu(entry.prtb0);

This refactoring somewhat obscures the changes directly relevant to
the nested guest handling.  Ideally it would be nice to fold some of
this into the earlier reworkings.

True, but given the rapidly approaching merge window, I'm not inclined
to rework it.

Yeah, ok.

quoted

quoted

+	if (ret) {
+		/* We didn't find a pte */
+		if (ret == -EINVAL) {
+			/* Unsupported mmu config */
+			flags |= DSISR_UNSUPP_MMU;
+		} else if (ret == -ENOENT) {
+			/* No translation found */
+			flags |= DSISR_NOHPTE;
+		} else if (ret == -EFAULT) {
+			/* Couldn't access L1 real address */
+			flags |= DSISR_PRTABLE_FAULT;
+			vcpu->arch.fault_gpa = fault_addr;
+		} else {
+			/* Unknown error */
+			return ret;
+		}
+		goto resume_host;

This is effectively forwarding the fault to L1, yes?  In which case a
different name might be better than the ambiguous "resume_host".

I'll change it to "forward_to_l1".

Thanks.

Paul.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson