Re: sys/powerpc64: encode PTCR PATS correctly

Mark Kettenis <[email protected]> Mon, 13 Jul 2026 14:28:53 +0200
Newsgroups gmane.os.openbsd.tech
Message-ID <[email protected]>
> Date: Mon, 13 Jul 2026 13:46:49 +0200
> From: Kirill A. Korinsky <[email protected]>
> 
> On Wed, 08 Jul 2026 00:35:32 +0200,
> Kirill A. Korinsky <[email protected]> wrote:
> > 
> > Mark, George,
> > 
> > Shivang Upadhyay in qemu-devel
> > https://marc.info/?l=qemu-devel&m=178332702820587&w=2 pointed that we may
> > have a typo kind of bug in powerpc64's pmap.
> > 
> > The PTCR PATS field stores log2(partition table size) - 12.  Since
> > PATMEMSZ is a power of two, ffs(PATMEMSZ) is log2(PATMEMSZ) + 1; the
> > old expression wrote 5 for the 64 KiB table, describing 128 KiB to
> > Book3S v3 hardware.
> > 
> > Use fls(PATMEMSZ) - 1 - 12 to express the intended log2 calculation
> > and encode the 64 KiB partition table as PATS == 4.
> > 
> > Ok?
> > 
> 
> Ping?

I'd still like to see this tested on real hardware.  Seems George is
alive, so hopefully he'll get around to doing this soonish.

One minor nit about the proposed diff...

> Index: sys/arch/powerpc64/powerpc64/pmap.c
> ===================================================================
> RCS file: /home/cvs/src/sys/arch/powerpc64/powerpc64/pmap.c,v
> diff -u -p -r1.66 pmap.c
> --- sys/arch/powerpc64/powerpc64/pmap.c	21 Aug 2025 00:10:21 -0000	1.66
> +++ sys/arch/powerpc64/powerpc64/pmap.c	7 Jul 2026 20:20:55 -0000
> @@ -124,7 +124,7 @@ uint64_t pmap_ptab_mask;
>  struct pate *pmap_pat;
>  
>  #define PATMEMSZ	(64 * 1024)
> -#define PATSIZE		(ffs(PATMEMSZ) - 12)
> +#define PATSIZE		(fls(PATMEMSZ) - 1 - 12)

I think I'd prefer to keep using ffs().  As long as the size is a
power of two there should be no issue.  But if it isn't, ffs() would
pass a smaller size to the hardware instead of a larger size, which
seems safer.  I also would use an extra set of parentheses to express
that the "- 1" is part of the log2 calculation.  So...

#define PATSIZE		((ffs(PATMEMSZ) - 1) - 12)


>  
>  struct pte_desc {
>  	/* Linked list of phys -> virt entries */
> 
> 
> -- 
> wbr, Kirill
> 
>