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

[Dev Jain <dev.jain@arm.com>]

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.
>>
>