Received-SPF: pass (google.com: domain of linux-kernel+bounces-212677-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 13 Jun 2024 09:19:51 +0200
From: Oscar Salvador <osalvador@suse.de>
To: LEROY Christophe <christophe.leroy2@cs-soprasteria.com>
Cc: Peter Xu <peterx@redhat.com>, Andrew Morton <akpm@linux-foundation.org>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Nicholas Piggin <npiggin@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v5 02/18] mm: Define __pte_leaf_size() to also take a PMD
 entry
Message-ID: <Zmqdl1aqmU9BgYzo@localhost.localdomain>
References: <cover.1717955558.git.christophe.leroy@csgroup.eu>
 <172b11c93e0de7a84937af2da9f80bd17c56b8c9.1717955558.git.christophe.leroy@csgroup.eu>
 <ZmgaHyS0izhtKbx6@localhost.localdomain>
 <ZmhcepJrkDpJ7mSC@x1n>
 <ZmhofWIiMC3I0aMF@localhost.localdomain>
 <ZmhrIdh3PLzvZU07@x1n>
 <Zmh282yJjxc7zqbL@localhost.localdomain>
 <e9583aa5-4ad7-4bcf-b3ff-f42b983231f5@cs-soprasteria.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e9583aa5-4ad7-4bcf-b3ff-f42b983231f5@cs-soprasteria.com>

On Tue, Jun 11, 2024 at 07:00:14PM +0000, LEROY Christophe wrote:
> We have space available in PMD if we need more flags, but in PTE I can't 
> see anything possible without additional churn that would require 
> additional instructions in TLB miss handlers, which is what I want to 
> avoid most.
> 
> Bits mapped to HW PTE:
> 
> #define _PAGE_PRESENT	0x0001	/* V: Page is valid */
> #define _PAGE_NO_CACHE	0x0002	/* CI: cache inhibit */
> #define _PAGE_SH	0x0004	/* SH: No ASID (context) compare */
> #define _PAGE_SPS	0x0008	/* SPS: Small Page Size (1 if 16k, 512k or 8M)*/
> #define _PAGE_DIRTY	0x0100	/* C: page changed */
> #define _PAGE_NA	0x0200	/* Supervisor NA, User no access */
> #define _PAGE_RO	0x0600	/* Supervisor RO, User no access */
> 
> SW bits masked out in TLB miss handler:
> 
> #define _PAGE_GUARDED	0x0010	/* Copied to L1 G entry in DTLB */
> #define _PAGE_ACCESSED	0x0020	/* Copied to L1 APG 1 entry in I/DTLB */
> #define _PAGE_EXEC	0x0040	/* Copied to PP (bit 21) in ITLB */
> #define _PAGE_SPECIAL	0x0080	/* SW entry */
> #define _PAGE_HUGE	0x0800	/* Copied to L1 PS bit 29 */
> 
> All bits are used. The only thing would could do but that would have a 
> performance cost is to retrieve _PAGE_SH from the PMD and use that bit 
> for something else.

I guess that this would be the last resort if we run out of options.
But at least it is good to know that there is a plan B (or Z if you will
:-))

> But I was maybe thinking another way. Lets take the exemple of 
> pmd_write() helper:
> 
> #define pmd_write(pmd)		pte_write(pmd_pte(pmd))
> 
> At the time being we have
> 
> static inline pte_t pmd_pte(pmd_t pmd)
> {
> 	return __pte(pmd_val(pmd));
> }
> 
> But what about something like
> 
> static inline pte_t pmd_pte(pmd_t pmd)
> {
> 	return *(pte_t *)pmd_page_vaddr(pmd);
> }

I think this could work, yes.

So, we should define all pmd_*(pmd) operations for 8xx the way they are defined
in include/asm/book3s/64/pgtable.h.

Other page size would not interfere because they already can perform
operations on pte level.

Ok, I think we might have a shot here.

I would help testing, but I do not have 8xx hardware, and Qemu does not support
8xx emulation, but I think that if we are careful enough, this can work.

Actually, as a smoketest would be enough to have a task with a 8MB huge
mapped, and then do:

 static const struct mm_walk_ops test_walk_ops = {
         .pmd_entry = test_8mbp_hugepage,
         .pte_entry = test_16k_and_512k_hugepage,
         .hugetlb_entry = check_hugetlb_entry,
         .walk_lock = PGWALK_RDLOCK,
 };

 static int test(void) 
 {
 
          pr_info("%s: %s [0 - %lx]\n", __func__, current->comm, TASK_SIZE);
          mmap_read_lock(current->mm);
          ret = walk_page_range(current->mm, 0, TASK_SIZE, &test_walk_ops, NULL);
          mmap_read_unlock(current->mm);
          
          pr_info("%s: %s ret: %d\n", __func__, current->comm, ret);
          
          return 0;
 }

This is an extract of a debugging mechanism I have to check that I am
not going off rails when unifying hugetlb and normal walkers.

test_8mbp_hugepage() could so some checks with pmd_ operations, print
the results, and then compare them with those that check_hugetlb_entry()
would give us.
If everything is alright, both results should be the same.

I can write the tests up, so we run some sort of smoketests.

So yes, I do think that this is a good initiative.

Thanks a lot Christoph

-- 
Oscar Salvador
SUSE Labs