Thread (3 messages) 3 messages, 3 authors, 2022-02-15

Re: [PATCH] powerpc/ptdump: Fix sparse warning in hashpagetable.c

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2022-02-01 12:25:21
Also in: lkml
Subsystem: linux for powerpc (32-bit and 64-bit), the rest · Maintainers: Madhavan Srinivasan, Michael Ellerman, Linus Torvalds

Christophe Leroy [off-list ref] writes:
  arch/powerpc/mm/ptdump/hashpagetable.c:264:29: warning: restricted __be64 degrades to integer
  arch/powerpc/mm/ptdump/hashpagetable.c:265:49: warning: restricted __be64 degrades to integer
  arch/powerpc/mm/ptdump/hashpagetable.c:267:36: warning: incorrect type in assignment (different base types)
  arch/powerpc/mm/ptdump/hashpagetable.c:267:36:    expected unsigned long long [usertype]
  arch/powerpc/mm/ptdump/hashpagetable.c:267:36:    got restricted __be64 [usertype] v
  arch/powerpc/mm/ptdump/hashpagetable.c:268:36: warning: incorrect type in assignment (different base types)
  arch/powerpc/mm/ptdump/hashpagetable.c:268:36:    expected unsigned long long [usertype]
  arch/powerpc/mm/ptdump/hashpagetable.c:268:36:    got restricted __be64 [usertype] r

struct hash_pte fields have type __be64. Convert them to
regular long before using them.
Your patch changes one side of the comparison but not the other, which
implies the code doesn't work at the moment, ie. it should never be
matching.

But it does work at the moment, so there must be something else going on.
quoted hunk ↗ jump to hunk
diff --git a/arch/powerpc/mm/ptdump/hashpagetable.c b/arch/powerpc/mm/ptdump/hashpagetable.c
index c7f824d294b2..bf60ab1bedb9 100644
--- a/arch/powerpc/mm/ptdump/hashpagetable.c
+++ b/arch/powerpc/mm/ptdump/hashpagetable.c
@@ -261,11 +261,11 @@ static int pseries_find(unsigned long ea, int psize, bool primary, u64 *v, u64 *
Expanding the context a little:

	for (i = 0; i < HPTES_PER_GROUP; i += 4, hpte_group += 4) {
		lpar_rc = plpar_pte_read_4(0, hpte_group, (void *)ptes);
 		if (lpar_rc)
 			continue;
 		for (j = 0; j < 4; j++) {
-			if (HPTE_V_COMPARE(ptes[j].v, want_v) &&
-					(ptes[j].v & HPTE_V_VALID)) {
+			if (HPTE_V_COMPARE(be64_to_cpu(ptes[j].v), want_v) &&
+			    (be64_to_cpu(ptes[j].v) & HPTE_V_VALID)) {
 				/* HPTE matches */
-				*v = ptes[j].v;
-				*r = ptes[j].r;
+				*v = be64_to_cpu(ptes[j].v);
+				*r = be64_to_cpu(ptes[j].r);
 				return 0;
 			}
 		}
Turns out the values returned from plpar_pte_read_4() are already in CPU
endian.

We pass an on-stack buffer to plpar_pte_read_4():

  plpar_pte_read_4(0, hpte_group, (void *)ptes);

Which makes it look like the hypercall is writing to memory (our
buffer), so we'd expect the values to need an endian swap.

But plpar_pte_read_4() writes into that buffer from another on-stack
buffer:

static inline long plpar_pte_read_4(unsigned long flags, unsigned long ptex,
				    unsigned long *ptes)

{
	long rc;
	unsigned long retbuf[PLPAR_HCALL9_BUFSIZE];

	rc = plpar_hcall9(H_READ, retbuf, flags | H_READ_4, ptex);

	memcpy(ptes, retbuf, 8*sizeof(unsigned long));

	return rc;
}


And the values in that stack buffer are actually returned from the
hypervisor in registers, r4-r11, and written into retbuf by the asm
wrapper:

_GLOBAL_TOC(plpar_hcall9)
	HMT_MEDIUM

	mfcr	r0
	stw	r0,8(r1)

	HCALL_BRANCH(plpar_hcall9_trace)

	std     r4,STK_PARAM(R4)(r1)     /* Save ret buffer */ 		<- this is retbuf

	mr	r4,r5
	mr	r5,r6
	mr	r6,r7
	mr	r7,r8
	mr	r8,r9
	mr	r9,r10
	ld	r10,STK_PARAM(R11)(r1)	 /* put arg7 in R10 */
	ld	r11,STK_PARAM(R12)(r1)	 /* put arg8 in R11 */
	ld	r12,STK_PARAM(R13)(r1)    /* put arg9 in R12 */

	HVSC				/* invoke the hypervisor */

	mr	r0,r12
	ld	r12,STK_PARAM(R4)(r1)		<- reload retbuf into r12
	std	r4,  0(r12)
	std	r5,  8(r12)
	std	r6, 16(r12)
	std	r7, 24(r12)
	std	r8, 32(r12)
	std	r9, 40(r12)
	std	r10,48(r12)
	std	r11,56(r12)
	std	r0, 64(r12)


Although the values are BE in memory in the actual HPT, they're read by
the hypervisor which does the byte swap for us, and then when the
hypervisor returns they're returned in the registers. So there's no
extra byte swap needed.

Possibly we should move struct hash_pte into hash_native.c, which is
where it's almost exclusively used, and is used to point to actual HPTEs
in memory.

But for now I think the patch below is a minimal fix for this sparse
warning, it's what other callers of plpar_pte_read_4() are doing.

cheers

diff --git a/arch/powerpc/mm/ptdump/hashpagetable.c b/arch/powerpc/mm/ptdump/hashpagetable.c
index c7f824d294b2..9a601587836b 100644
--- a/arch/powerpc/mm/ptdump/hashpagetable.c
+++ b/arch/powerpc/mm/ptdump/hashpagetable.c
@@ -238,7 +238,10 @@ static int native_find(unsigned long ea, int psize, bool primary, u64 *v, u64
 
 static int pseries_find(unsigned long ea, int psize, bool primary, u64 *v, u64 *r)
 {
-	struct hash_pte ptes[4];
+	struct {
+		unsigned long v;
+		unsigned long r;
+	} ptes[4];
 	unsigned long vsid, vpn, hash, hpte_group, want_v;
 	int i, j, ssize = mmu_kernel_ssize;
 	long lpar_rc = 0;

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help