[PATCH] KVM: arm64: ptdump: Flush the last region

[PATCH] KVM: arm64: ptdump: Flush the last region

[Wei-Lin Chang]

Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
This simply misses the output of the last region, because note_page()
only dumps output when it detects a change in level/prot, or when the
walk enters a next marker section. The last region in the guest IPA
space with the same level/prot is not dumped since there is no change
after it.

To dump the final region, manually issue a note_page() call with address
BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
change after the last region. Additionally treat level == -1 as a last
flushing note_page() call and avoid dumping the name of the next marker
for this case.

Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
Reported-by: Sashiko AI <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
---
 arch/arm64/kvm/ptdump.c | 8 +++++---
 arch/arm64/mm/ptdump.c  | 5 +++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
index c9140e22abcf..4096e4a92fae 100644
--- a/arch/arm64/kvm/ptdump.c
+++ b/arch/arm64/kvm/ptdump.c
@@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
 		.seq		= m,
 	};
 
-	write_lock(&kvm->mmu_lock);
+	guard(write_lock)(&kvm->mmu_lock);
 	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
-	write_unlock(&kvm->mmu_lock);
+	if (ret)
+		return ret;
+	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
 
-	return ret;
+	return 0;
 }
 
 static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
index ab9899ca1e5f..fed4e4407e0e 100644
--- a/arch/arm64/mm/ptdump.c
+++ b/arch/arm64/mm/ptdump.c
@@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
 	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
 	struct ptdump_pg_level *pg_level = st->pg_level;
 	static const char units[] = "KMGTPE";
+	bool flush = level == -1;
 	ptdesc_t prot = 0;
 
 	/* check if the current level has been folded dynamically */
@@ -234,7 +235,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
 				  pg_level[st->level].num);
 		pt_dump_seq_puts(st->seq, "\n");
 
-		if (addr >= st->marker[1].start_address) {
+		if (addr >= st->marker[1].start_address && !flush) {
 			st->marker++;
 			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 		}
@@ -244,7 +245,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
 		st->level = level;
 	}
 
-	if (addr >= st->marker[1].start_address) {
+	if (addr >= st->marker[1].start_address && !flush) {
 		st->marker++;
 		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 	}
-- 
2.43.0

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Mark Rutland]

On Sat, Jul 18, 2026 at 12:12:33AM +0100, Wei-Lin Chang wrote:
> Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
> This simply misses the output of the last region, because note_page()
> only dumps output when it detects a change in level/prot, or when the
> walk enters a next marker section. The last region in the guest IPA
> space with the same level/prot is not dumped since there is no change
> after it.
> 
> To dump the final region, manually issue a note_page() call with address
> BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
> change after the last region. Additionally treat level == -1 as a last
> flushing note_page() call and avoid dumping the name of the next marker
> for this case.

I think you can use note_page_flush() rather than doing that manually.

> Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
> Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
> Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> ---
>  arch/arm64/kvm/ptdump.c | 8 +++++---
>  arch/arm64/mm/ptdump.c  | 5 +++--
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> index c9140e22abcf..4096e4a92fae 100644
> --- a/arch/arm64/kvm/ptdump.c
> +++ b/arch/arm64/kvm/ptdump.c
> @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
>  		.seq		= m,
>  	};
>  
> -	write_lock(&kvm->mmu_lock);
> +	guard(write_lock)(&kvm->mmu_lock);
>  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> -	write_unlock(&kvm->mmu_lock);
> +	if (ret)
> +		return ret;
> +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);

This can be:

	note_page_flush(&st->parser_state.ptdump);

The level change alone should trigger the dump, so the address doesn't
need to be at the end of the guest IPA space.

Importantly, note_page_flush() will pass 0 as the address, which won't
trigger the checks you try to suppress below.

Please use note_page_flush() here, and drop the changes to
arch/arm64/mm/ptdump.c.

>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> index ab9899ca1e5f..fed4e4407e0e 100644
> --- a/arch/arm64/mm/ptdump.c
> +++ b/arch/arm64/mm/ptdump.c
> @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
>  	struct ptdump_pg_level *pg_level = st->pg_level;
>  	static const char units[] = "KMGTPE";
> +	bool flush = level == -1;
>  	ptdesc_t prot = 0;
>  
>  	/* check if the current level has been folded dynamically */
> @@ -234,7 +235,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  				  pg_level[st->level].num);
>  		pt_dump_seq_puts(st->seq, "\n");
>  
> -		if (addr >= st->marker[1].start_address) {
> +		if (addr >= st->marker[1].start_address && !flush) {
>  			st->marker++;
>  			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
>  		}

If you use note_page_flush(), addr will be 0. As the final marker is at
BIT(pgtable->ia_bits), this condition cannot fire.

> @@ -244,7 +245,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  		st->level = level;
>  	}
>  
> -	if (addr >= st->marker[1].start_address) {
> +	if (addr >= st->marker[1].start_address && !flush) {
>  		st->marker++;
>  		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
>  	}

Likewise here.

Mark.

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Wei-Lin Chang]

On Mon, Jul 20, 2026 at 09:35:01AM +0100, Mark Rutland wrote:
> On Sat, Jul 18, 2026 at 12:12:33AM +0100, Wei-Lin Chang wrote:
> > Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
> > This simply misses the output of the last region, because note_page()
> > only dumps output when it detects a change in level/prot, or when the
> > walk enters a next marker section. The last region in the guest IPA
> > space with the same level/prot is not dumped since there is no change
> > after it.
> > 
> > To dump the final region, manually issue a note_page() call with address
> > BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
> > change after the last region. Additionally treat level == -1 as a last
> > flushing note_page() call and avoid dumping the name of the next marker
> > for this case.
> 
> I think you can use note_page_flush() rather than doing that manually.
> 
> > Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
> > Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
> > Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> > ---
> >  arch/arm64/kvm/ptdump.c | 8 +++++---
> >  arch/arm64/mm/ptdump.c  | 5 +++--
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> > index c9140e22abcf..4096e4a92fae 100644
> > --- a/arch/arm64/kvm/ptdump.c
> > +++ b/arch/arm64/kvm/ptdump.c
> > @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> >  		.seq		= m,
> >  	};
> >  
> > -	write_lock(&kvm->mmu_lock);
> > +	guard(write_lock)(&kvm->mmu_lock);
> >  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > -	write_unlock(&kvm->mmu_lock);
> > +	if (ret)
> > +		return ret;
> > +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
> 
> This can be:
> 
> 	note_page_flush(&st->parser_state.ptdump);
> 
> The level change alone should trigger the dump, so the address doesn't
> need to be at the end of the guest IPA space.
> 
> Importantly, note_page_flush() will pass 0 as the address, which won't
> trigger the checks you try to suppress below.
> 
> Please use note_page_flush() here, and drop the changes to
> arch/arm64/mm/ptdump.c.

Sorry, I shouldn't have omitted this information, but I did try
note_page_flush(). And it gives something like this in the last row:

0x00000000ffc00000-0x0000000000000000   17592186040324M 2   R W px ux  AF BLK

The astronomical size is from (addr - st->start_address). As IA bits for
stage-2 are not close to 64, we'll have large sizes for the last row.
That's one of the reasons I chose to call note_page() with
BIT(pgtable->ia_bits) as addr, to end the ptdump at the end of the guest
IPA space.

Additionally, the last row is combining two ranges:

1. 0x00000000ffc00000-0x0000000100000000   4M       2    R W px ux  AF BLK
2. 0x0000000100000000-0x0000000000000000   BIG_SIZE -    (empty prot)

The attributes are wrong for the large range after
BIT(pgtable->ia_bits). This is because before dumping the last row, the
ptdump code is waiting to be notified of the end of the final region
with all those {R, W, px, ux, AF, BLK} attributes. Using
note_page_flush() essentially tells it the valid range ends at 1 << 64.
So actually using note_page() with BIT(pgtable->ia_bits) is required for
correctness.

The kernel ptdump is not affected by this (at least from my quick test):

0xffffffffff6fe000-0xffffffffff800000        1032K PTE
0xffffffffff800000-0x0000000000000000           8M PMD

Because it uses addresses close to the end of the address space, and its
last leaf PTE entry is an invalid one.

Thanks,
Wei-Lin Chang

> 
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
> > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> > index ab9899ca1e5f..fed4e4407e0e 100644
> > --- a/arch/arm64/mm/ptdump.c
> > +++ b/arch/arm64/mm/ptdump.c
> > @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> >  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
> >  	struct ptdump_pg_level *pg_level = st->pg_level;
> >  	static const char units[] = "KMGTPE";
> > +	bool flush = level == -1;
> >  	ptdesc_t prot = 0;
> >  
> >  	/* check if the current level has been folded dynamically */
> > @@ -234,7 +235,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> >  				  pg_level[st->level].num);
> >  		pt_dump_seq_puts(st->seq, "\n");
> >  
> > -		if (addr >= st->marker[1].start_address) {
> > +		if (addr >= st->marker[1].start_address && !flush) {
> >  			st->marker++;
> >  			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
> >  		}
> 
> If you use note_page_flush(), addr will be 0. As the final marker is at
> BIT(pgtable->ia_bits), this condition cannot fire.
> 
> > @@ -244,7 +245,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> >  		st->level = level;
> >  	}
> >  
> > -	if (addr >= st->marker[1].start_address) {
> > +	if (addr >= st->marker[1].start_address && !flush) {
> >  		st->marker++;
> >  		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
> >  	}
> 
> Likewise here.
> 
> Mark.

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Wei-Lin Chang]

On Mon, Jul 20, 2026 at 11:58:44AM +0100, Wei-Lin Chang wrote:
> On Mon, Jul 20, 2026 at 09:35:01AM +0100, Mark Rutland wrote:
> > On Sat, Jul 18, 2026 at 12:12:33AM +0100, Wei-Lin Chang wrote:
> > > Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
> > > This simply misses the output of the last region, because note_page()
> > > only dumps output when it detects a change in level/prot, or when the
> > > walk enters a next marker section. The last region in the guest IPA
> > > space with the same level/prot is not dumped since there is no change
> > > after it.
> > > 
> > > To dump the final region, manually issue a note_page() call with address
> > > BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
> > > change after the last region. Additionally treat level == -1 as a last
> > > flushing note_page() call and avoid dumping the name of the next marker
> > > for this case.
> > 
> > I think you can use note_page_flush() rather than doing that manually.
> > 
> > > Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
> > > Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> > > Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
> > > Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> > > ---
> > >  arch/arm64/kvm/ptdump.c | 8 +++++---
> > >  arch/arm64/mm/ptdump.c  | 5 +++--
> > >  2 files changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> > > index c9140e22abcf..4096e4a92fae 100644
> > > --- a/arch/arm64/kvm/ptdump.c
> > > +++ b/arch/arm64/kvm/ptdump.c
> > > @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> > >  		.seq		= m,
> > >  	};
> > >  
> > > -	write_lock(&kvm->mmu_lock);
> > > +	guard(write_lock)(&kvm->mmu_lock);
> > >  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > > -	write_unlock(&kvm->mmu_lock);
> > > +	if (ret)
> > > +		return ret;
> > > +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
> > 
> > This can be:
> > 
> > 	note_page_flush(&st->parser_state.ptdump);
> > 
> > The level change alone should trigger the dump, so the address doesn't
> > need to be at the end of the guest IPA space.
> > 
> > Importantly, note_page_flush() will pass 0 as the address, which won't
> > trigger the checks you try to suppress below.
> > 
> > Please use note_page_flush() here, and drop the changes to
> > arch/arm64/mm/ptdump.c.
> 
> Sorry, I shouldn't have omitted this information, but I did try
> note_page_flush(). And it gives something like this in the last row:
> 
> 0x00000000ffc00000-0x0000000000000000   17592186040324M 2   R W px ux  AF BLK
> 
> The astronomical size is from (addr - st->start_address). As IA bits for
> stage-2 are not close to 64, we'll have large sizes for the last row.
> That's one of the reasons I chose to call note_page() with
> BIT(pgtable->ia_bits) as addr, to end the ptdump at the end of the guest
> IPA space.
> 
> Additionally, the last row is combining two ranges:
> 
> 1. 0x00000000ffc00000-0x0000000100000000   4M       2    R W px ux  AF BLK
> 2. 0x0000000100000000-0x0000000000000000   BIG_SIZE -    (empty prot)
> 
> The attributes are wrong for the large range after
> BIT(pgtable->ia_bits). This is because before dumping the last row, the
> ptdump code is waiting to be notified of the end of the final region
> with all those {R, W, px, ux, AF, BLK} attributes. Using
> note_page_flush() essentially tells it the valid range ends at 1 << 64.
> So actually using note_page() with BIT(pgtable->ia_bits) is required for
> correctness.
> 
> The kernel ptdump is not affected by this (at least from my quick test):
> 
> 0xffffffffff6fe000-0xffffffffff800000        1032K PTE
> 0xffffffffff800000-0x0000000000000000           8M PMD
> 
> Because it uses addresses close to the end of the address space, and its
> last leaf PTE entry is an invalid one.

Hmm no. It's because the kernel page table is capable of translating the
full range 0xffff000000000000-0x0000000000000000. This isn't true for
stage-2 page tables.

Thanks,
Wei-Lin Chang

> 
> Thanks,
> Wei-Lin Chang
> 
> > 
> > >  
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
> > > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> > > index ab9899ca1e5f..fed4e4407e0e 100644
> > > --- a/arch/arm64/mm/ptdump.c
> > > +++ b/arch/arm64/mm/ptdump.c
> > > @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> > >  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
> > >  	struct ptdump_pg_level *pg_level = st->pg_level;
> > >  	static const char units[] = "KMGTPE";
> > > +	bool flush = level == -1;
> > >  	ptdesc_t prot = 0;
> > >  
> > >  	/* check if the current level has been folded dynamically */
> > > @@ -234,7 +235,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> > >  				  pg_level[st->level].num);
> > >  		pt_dump_seq_puts(st->seq, "\n");
> > >  
> > > -		if (addr >= st->marker[1].start_address) {
> > > +		if (addr >= st->marker[1].start_address && !flush) {
> > >  			st->marker++;
> > >  			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
> > >  		}
> > 
> > If you use note_page_flush(), addr will be 0. As the final marker is at
> > BIT(pgtable->ia_bits), this condition cannot fire.
> > 
> > > @@ -244,7 +245,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> > >  		st->level = level;
> > >  	}
> > >  
> > > -	if (addr >= st->marker[1].start_address) {
> > > +	if (addr >= st->marker[1].start_address && !flush) {
> > >  		st->marker++;
> > >  		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
> > >  	}
> > 
> > Likewise here.
> > 
> > Mark.

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Mark Rutland]

On Mon, Jul 20, 2026 at 11:58:44AM +0100, Wei-Lin Chang wrote:
> On Mon, Jul 20, 2026 at 09:35:01AM +0100, Mark Rutland wrote:
> > On Sat, Jul 18, 2026 at 12:12:33AM +0100, Wei-Lin Chang wrote:
> > > @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> > >  		.seq		= m,
> > >  	};
> > >  
> > > -	write_lock(&kvm->mmu_lock);
> > > +	guard(write_lock)(&kvm->mmu_lock);
> > >  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > > -	write_unlock(&kvm->mmu_lock);
> > > +	if (ret)
> > > +		return ret;
> > > +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
> > 
> > This can be:
> > 
> > 	note_page_flush(&st->parser_state.ptdump);
> > 
> > The level change alone should trigger the dump, so the address doesn't
> > need to be at the end of the guest IPA space.
> > 
> > Importantly, note_page_flush() will pass 0 as the address, which won't
> > trigger the checks you try to suppress below.
> > 
> > Please use note_page_flush() here, and drop the changes to
> > arch/arm64/mm/ptdump.c.
> 
> Sorry, I shouldn't have omitted this information, but I did try
> note_page_flush(). And it gives something like this in the last row:
> 
> 0x00000000ffc00000-0x0000000000000000   17592186040324M 2   R W px ux  AF BLK
> 
> The astronomical size is from (addr - st->start_address). As IA bits for
> stage-2 are not close to 64, we'll have large sizes for the last row.

Ok. That's a bug in the current implementation of note_page_flush(),
then. The *intent* is that note_page_flush() is used to terminate
output, and we should mak it work.

Do we need to pass additional information, or do we have the necessary
values in (or accessible from) struct ptdump_state?

> That's one of the reasons I chose to call note_page() with
> BIT(pgtable->ia_bits) as addr, to end the ptdump at the end of the guest
> IPA space.
> 
> Additionally, the last row is combining two ranges:
> 
> 1. 0x00000000ffc00000-0x0000000100000000   4M       2    R W px ux  AF BLK
> 2. 0x0000000100000000-0x0000000000000000   BIG_SIZE -    (empty prot)
> 
> The attributes are wrong for the large range after
> BIT(pgtable->ia_bits). This is because before dumping the last row, the
> ptdump code is waiting to be notified of the end of the final region
> with all those {R, W, px, ux, AF, BLK} attributes. Using
> note_page_flush() essentially tells it the valid range ends at 1 << 64.
> So actually using note_page() with BIT(pgtable->ia_bits) is required for
> correctness.
> 
> The kernel ptdump is not affected by this (at least from my quick test):
> 
> 0xffffffffff6fe000-0xffffffffff800000        1032K PTE
> 0xffffffffff800000-0x0000000000000000           8M PMD

Yes, it's not affected because its final VA is 0xffffffffffffffff (i.e.
2^64i - 1), and so using 0 as the next address does the right thing
modulo 64 bits.

We use ptdump for non-kernel tables today (e.g. the EFI mm), so
presumably they suffer the same problem?

Mark.

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Wei-Lin Chang]

On Mon, Jul 20, 2026 at 01:37:51PM +0100, Mark Rutland wrote:
> On Mon, Jul 20, 2026 at 11:58:44AM +0100, Wei-Lin Chang wrote:
> > On Mon, Jul 20, 2026 at 09:35:01AM +0100, Mark Rutland wrote:
> > > On Sat, Jul 18, 2026 at 12:12:33AM +0100, Wei-Lin Chang wrote:
> > > > @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> > > >  		.seq		= m,
> > > >  	};
> > > >  
> > > > -	write_lock(&kvm->mmu_lock);
> > > > +	guard(write_lock)(&kvm->mmu_lock);
> > > >  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > > > -	write_unlock(&kvm->mmu_lock);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
> > > 
> > > This can be:
> > > 
> > > 	note_page_flush(&st->parser_state.ptdump);
> > > 
> > > The level change alone should trigger the dump, so the address doesn't
> > > need to be at the end of the guest IPA space.
> > > 
> > > Importantly, note_page_flush() will pass 0 as the address, which won't
> > > trigger the checks you try to suppress below.
> > > 
> > > Please use note_page_flush() here, and drop the changes to
> > > arch/arm64/mm/ptdump.c.
> > 
> > Sorry, I shouldn't have omitted this information, but I did try
> > note_page_flush(). And it gives something like this in the last row:
> > 
> > 0x00000000ffc00000-0x0000000000000000   17592186040324M 2   R W px ux  AF BLK
> > 
> > The astronomical size is from (addr - st->start_address). As IA bits for
> > stage-2 are not close to 64, we'll have large sizes for the last row.
> 
> Ok. That's a bug in the current implementation of note_page_flush(),
> then. The *intent* is that note_page_flush() is used to terminate
> output, and we should mak it work.

Thanks for the suggestion, I agree.

> 
> Do we need to pass additional information, or do we have the necessary
> values in (or accessible from) struct ptdump_state?

There is struct ptdump_state.range[], and I think we can get the end
address of the ptdump from that.

> 
> > That's one of the reasons I chose to call note_page() with
> > BIT(pgtable->ia_bits) as addr, to end the ptdump at the end of the guest
> > IPA space.
> > 
> > Additionally, the last row is combining two ranges:
> > 
> > 1. 0x00000000ffc00000-0x0000000100000000   4M       2    R W px ux  AF BLK
> > 2. 0x0000000100000000-0x0000000000000000   BIG_SIZE -    (empty prot)
> > 
> > The attributes are wrong for the large range after
> > BIT(pgtable->ia_bits). This is because before dumping the last row, the
> > ptdump code is waiting to be notified of the end of the final region
> > with all those {R, W, px, ux, AF, BLK} attributes. Using
> > note_page_flush() essentially tells it the valid range ends at 1 << 64.
> > So actually using note_page() with BIT(pgtable->ia_bits) is required for
> > correctness.
> > 
> > The kernel ptdump is not affected by this (at least from my quick test):
> > 
> > 0xffffffffff6fe000-0xffffffffff800000        1032K PTE
> > 0xffffffffff800000-0x0000000000000000           8M PMD
> 
> Yes, it's not affected because its final VA is 0xffffffffffffffff (i.e.
> 2^64i - 1), and so using 0 as the next address does the right thing
> modulo 64 bits.
> 
> We use ptdump for non-kernel tables today (e.g. the EFI mm), so
> presumably they suffer the same problem?

Cool, I didn't know about efi_mm and its ptdump! After looking at it, I
think yes it also has this problem, since its a ttbr0 ptdump.

So I plan:
1. In note_page_flush, use ptdump_state.range[] to determine the end
   address (the last range with start != end).
2. In KVM ptdump, initialize ptdump_state.range[], and call
   note_page_flush() at the end of the walk.

For the extra marker name dumps, we can simply change the second
marker's start_address from BIT(pgtable->ia_bits) to ~0UL so it doesn't
trigger the marker name dump.

Hope these sound good!

Thanks,
Wei-Lin Chang

> 
> Mark.

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Dev Jain]

On 18/07/26 4:42 am, Wei-Lin Chang wrote:
> Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
> This simply misses the output of the last region, because note_page()
> only dumps output when it detects a change in level/prot, or when the
> walk enters a next marker section. The last region in the guest IPA
> space with the same level/prot is not dumped since there is no change
> after it.
> 
> To dump the final region, manually issue a note_page() call with address
> BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
> change after the last region. Additionally treat level == -1 as a last
> flushing note_page() call and avoid dumping the name of the next marker
> for this case.
> 
> Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
> Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
> Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> ---
>  arch/arm64/kvm/ptdump.c | 8 +++++---
>  arch/arm64/mm/ptdump.c  | 5 +++--
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> index c9140e22abcf..4096e4a92fae 100644
> --- a/arch/arm64/kvm/ptdump.c
> +++ b/arch/arm64/kvm/ptdump.c
> @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
>  		.seq		= m,
>  	};
>  
> -	write_lock(&kvm->mmu_lock);
> +	guard(write_lock)(&kvm->mmu_lock);

The guard clause change has nothing to do with the fix I think? Although the change
is small and the fix is for a recent commit so maybe we don't care having this
in the same patch - not sure how strict kvm reviewers are about this : )


>  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> -	write_unlock(&kvm->mmu_lock);
> +	if (ret)
> +		return ret;
> +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> index ab9899ca1e5f..fed4e4407e0e 100644
> --- a/arch/arm64/mm/ptdump.c
> +++ b/arch/arm64/mm/ptdump.c
> @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
>  	struct ptdump_pg_level *pg_level = st->pg_level;
>  	static const char units[] = "KMGTPE";
> +	bool flush = level == -1;

Can we do something similar to what S1 ptdump does (note_page_flush in ptdump_walk_pgd).


>  	ptdesc_t prot = 0;
>  
>  	/* check if the current level has been folded dynamically */
> @@ -234,7 +235,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  				  pg_level[st->level].num);
>  		pt_dump_seq_puts(st->seq, "\n");
>  
> -		if (addr >= st->marker[1].start_address) {
> +		if (addr >= st->marker[1].start_address && !flush) {
>  			st->marker++;
>  			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
>  		}
> @@ -244,7 +245,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>  		st->level = level;
>  	}
>  
> -	if (addr >= st->marker[1].start_address) {
> +	if (addr >= st->marker[1].start_address && !flush) {
>  		st->marker++;
>  		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
>  	}

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Marc Zyngier]

On Mon, 20 Jul 2026 09:53:29 +0100,
Dev Jain <dev.jain@arm.com> wrote:
> 
> 
> 
> On 18/07/26 4:42 am, Wei-Lin Chang wrote:
> > Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
> > This simply misses the output of the last region, because note_page()
> > only dumps output when it detects a change in level/prot, or when the
> > walk enters a next marker section. The last region in the guest IPA
> > space with the same level/prot is not dumped since there is no change
> > after it.
> > 
> > To dump the final region, manually issue a note_page() call with address
> > BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
> > change after the last region. Additionally treat level == -1 as a last
> > flushing note_page() call and avoid dumping the name of the next marker
> > for this case.
> > 
> > Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
> > Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
> > Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
> > ---
> >  arch/arm64/kvm/ptdump.c | 8 +++++---
> >  arch/arm64/mm/ptdump.c  | 5 +++--
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> > index c9140e22abcf..4096e4a92fae 100644
> > --- a/arch/arm64/kvm/ptdump.c
> > +++ b/arch/arm64/kvm/ptdump.c
> > @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
> >  		.seq		= m,
> >  	};
> >  
> > -	write_lock(&kvm->mmu_lock);
> > +	guard(write_lock)(&kvm->mmu_lock);
> 
> The guard clause change has nothing to do with the fix I think? Although the change
> is small and the fix is for a recent commit so maybe we don't care having this
> in the same patch - not sure how strict kvm reviewers are about this : )
>

Look at the quality spaghetti code this would otherwise result in:

	write_lock();
	ret = kvm_pgtable_walk();
	if (ret) {
		write_unlock();
		return ret;
	}
	note_page_flush();
	write_unlock();

	return 0;

How is that better? So it has *everything* to do with the fix.

And FWIW, the policy for KVM is that we don't do lock->guard
conversions as a separate patches. Only when we need to change the
code, and as part of the patch that changes that code.  Which is
exactly what is happening here.

> 
> >  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> > -	write_unlock(&kvm->mmu_lock);
> > +	if (ret)
> > +		return ret;
> > +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
> > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
> > index ab9899ca1e5f..fed4e4407e0e 100644
> > --- a/arch/arm64/mm/ptdump.c
> > +++ b/arch/arm64/mm/ptdump.c
> > @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> >  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
> >  	struct ptdump_pg_level *pg_level = st->pg_level;
> >  	static const char units[] = "KMGTPE";
> > +	bool flush = level == -1;
> 
> Can we do something similar to what S1 ptdump does (note_page_flush in ptdump_walk_pgd).

See Mark's email.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Dev Jain]

On 20/07/26 2:33 pm, Marc Zyngier wrote:
> On Mon, 20 Jul 2026 09:53:29 +0100,
> Dev Jain <dev.jain@arm.com> wrote:
>>
>>
>>
>> On 18/07/26 4:42 am, Wei-Lin Chang wrote:
>>> Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
>>> This simply misses the output of the last region, because note_page()
>>> only dumps output when it detects a change in level/prot, or when the
>>> walk enters a next marker section. The last region in the guest IPA
>>> space with the same level/prot is not dumped since there is no change
>>> after it.
>>>
>>> To dump the final region, manually issue a note_page() call with address
>>> BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
>>> change after the last region. Additionally treat level == -1 as a last
>>> flushing note_page() call and avoid dumping the name of the next marker
>>> for this case.
>>>
>>> Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
>>> Reported-by: Sashiko AI <sashiko-bot@kernel.org>
>>> Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
>>> Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
>>> ---
>>>  arch/arm64/kvm/ptdump.c | 8 +++++---
>>>  arch/arm64/mm/ptdump.c  | 5 +++--
>>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
>>> index c9140e22abcf..4096e4a92fae 100644
>>> --- a/arch/arm64/kvm/ptdump.c
>>> +++ b/arch/arm64/kvm/ptdump.c
>>> @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
>>>  		.seq		= m,
>>>  	};
>>>  
>>> -	write_lock(&kvm->mmu_lock);
>>> +	guard(write_lock)(&kvm->mmu_lock);
>>
>> The guard clause change has nothing to do with the fix I think? Although the change
>> is small and the fix is for a recent commit so maybe we don't care having this
>> in the same patch - not sure how strict kvm reviewers are about this : )
>>
> 
> Look at the quality spaghetti code this would otherwise result in:
> 
> 	write_lock();
> 	ret = kvm_pgtable_walk();
> 	if (ret) {
> 		write_unlock();
> 		return ret;
> 	}
> 	note_page_flush();
> 	write_unlock();
> 
> 	return 0;
> 
> How is that better? So it has *everything* to do with the fix.
> 
> And FWIW, the policy for KVM is that we don't do lock->guard
> conversions as a separate patches. Only when we need to change the
> code, and as part of the patch that changes that code.  Which is
> exactly what is happening here.

I agree, thanks.


> 
>>
>>>  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
>>> -	write_unlock(&kvm->mmu_lock);
>>> +	if (ret)
>>> +		return ret;
>>> +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
>>>  
>>> -	return ret;
>>> +	return 0;
>>>  }
>>>  
>>>  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
>>> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
>>> index ab9899ca1e5f..fed4e4407e0e 100644
>>> --- a/arch/arm64/mm/ptdump.c
>>> +++ b/arch/arm64/mm/ptdump.c
>>> @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>>>  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
>>>  	struct ptdump_pg_level *pg_level = st->pg_level;
>>>  	static const char units[] = "KMGTPE";
>>> +	bool flush = level == -1;
>>
>> Can we do something similar to what S1 ptdump does (note_page_flush in ptdump_walk_pgd).
> 
> See Mark's email.
> 
> 	M.
> 

Re: [PATCH] KVM: arm64: ptdump: Flush the last region

[Dev Jain]

On 20/07/26 2:40 pm, Dev Jain wrote:
> 
> 
> On 20/07/26 2:33 pm, Marc Zyngier wrote:
>> On Mon, 20 Jul 2026 09:53:29 +0100,
>> Dev Jain <dev.jain@arm.com> wrote:
>>>
>>>
>>>
>>> On 18/07/26 4:42 am, Wei-Lin Chang wrote:
>>>> Currently the stage-2 ptdump calls note_page() at each leaf entry visit.
>>>> This simply misses the output of the last region, because note_page()
>>>> only dumps output when it detects a change in level/prot, or when the
>>>> walk enters a next marker section. The last region in the guest IPA
>>>> space with the same level/prot is not dumped since there is no change
>>>> after it.
>>>>
>>>> To dump the final region, manually issue a note_page() call with address
>>>> BIT(ia_bits) (end of guest IPA space) and level == -1 to trigger a level
>>>> change after the last region. Additionally treat level == -1 as a last
>>>> flushing note_page() call and avoid dumping the name of the next marker
>>>> for this case.
>>>>
>>>> Fixes: 7c4f73548ed1 ("KVM: arm64: Register ptdump with debugfs on guest creation")
>>>> Reported-by: Sashiko AI <sashiko-bot@kernel.org>
>>>> Closes: https://lore.kernel.org/kvmarm/20260630122758.891011F00A3A@smtp.kernel.org/
>>>> Signed-off-by: Wei-Lin Chang <weilin.chang@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/ptdump.c | 8 +++++---
>>>>  arch/arm64/mm/ptdump.c  | 5 +++--
>>>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
>>>> index c9140e22abcf..4096e4a92fae 100644
>>>> --- a/arch/arm64/kvm/ptdump.c
>>>> +++ b/arch/arm64/kvm/ptdump.c
>>>> @@ -155,11 +155,13 @@ static int kvm_ptdump_guest_show(struct seq_file *m, void *unused)
>>>>  		.seq		= m,
>>>>  	};
>>>>  
>>>> -	write_lock(&kvm->mmu_lock);
>>>> +	guard(write_lock)(&kvm->mmu_lock);
>>>
>>> The guard clause change has nothing to do with the fix I think? Although the change
>>> is small and the fix is for a recent commit so maybe we don't care having this
>>> in the same patch - not sure how strict kvm reviewers are about this : )
>>>
>>
>> Look at the quality spaghetti code this would otherwise result in:
>>
>> 	write_lock();
>> 	ret = kvm_pgtable_walk();
>> 	if (ret) {
>> 		write_unlock();
>> 		return ret;
>> 	}
>> 	note_page_flush();
>> 	write_unlock();
>>
>> 	return 0;
>>
>> How is that better? So it has *everything* to do with the fix.
>>
>> And FWIW, the policy for KVM is that we don't do lock->guard
>> conversions as a separate patches. Only when we need to change the
>> code, and as part of the patch that changes that code.  Which is
>> exactly what is happening here.
> 
> I agree, thanks.

Yeah in my head I was thinking that the guard clause is a "functional change"
but it really isn't, the fix implies that without the guard you will get
horrible code, and had the code been correct in the first place, it would
have been written *with* the guard, so yes that was a stupid
observation from my side : )

> 
> 
>>
>>>
>>>>  	ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
>>>> -	write_unlock(&kvm->mmu_lock);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	note_page(&st->parser_state.ptdump, BIT(mmu->pgt->ia_bits), -1, 0);
>>>>  
>>>> -	return ret;
>>>> +	return 0;
>>>>  }
>>>>  
>>>>  static int kvm_ptdump_guest_open(struct inode *m, struct file *file)
>>>> diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c
>>>> index ab9899ca1e5f..fed4e4407e0e 100644
>>>> --- a/arch/arm64/mm/ptdump.c
>>>> +++ b/arch/arm64/mm/ptdump.c
>>>> @@ -194,6 +194,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
>>>>  	struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump);
>>>>  	struct ptdump_pg_level *pg_level = st->pg_level;
>>>>  	static const char units[] = "KMGTPE";
>>>> +	bool flush = level == -1;
>>>
>>> Can we do something similar to what S1 ptdump does (note_page_flush in ptdump_walk_pgd).
>>
>> See Mark's email.
>>
>> 	M.
>>
>