Re: [PATCH 13/22] KVM: arm64: Set dirty state at stage-1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware management of the dirty state (wi->hd) is incorrectly enabled for stage-1 even if hardware management of the access flag (wi->ha) is disabled.
- [Medium] The walk result's descriptor and dirty state are not updated after a successful hardware update, leading to spurious secondary faults.
--

commit bfdb5faf043bd9330372f7cd1a89affe2bb61b35
Author: Oliver Upton <oupton@kernel.org>
KVM: arm64: Set dirty state at stage-1

This commit updates the KVM ARM64 software page table walker to clear
the PTE_RDONLY bit in guest page tables when write access is permitted
and DBM (Dirty Bit Management) is enabled. This simulates hardware
management of the dirty state during stage-1 translation walks.

> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 0218176107b52..bfba31b270a95 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -412,6 +412,10 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	wi->ha &= (wi->regime == TR_EL2 ?
>  		  FIELD_GET(TCR_EL2_HA, tcr) :
>  		  FIELD_GET(TCR_HA, tcr));
> +	wi->hd  = kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, HAFDBS, DBM);
> +	wi->hd &= (wi->regime == TR_EL2 ?
> +		  FIELD_GET(TCR_EL2_HD, tcr) :
> +		  FIELD_GET(TCR_HD, tcr));

[Severity: High]
Does this code incorrectly enable hardware management of the dirty state (wi->hd)
when hardware management of the access flag (wi->ha) is disabled?

The ARM Architecture Reference Manual (D8.2.7) states that if HA is 0, HD is
effectively 0 and no hardware updates are permitted.

If a guest enables TCR_ELx.HD but leaves TCR_ELx.HA disabled, wi->hd evaluates
to true while wi->ha is false in setup_s1_walk(). Upon a write fault,
handle_desc_update() would clear PTE_RDONLY but skip setting PTE_AF, causing KVM
to inject an Access Flag fault despite having already modified the guest page
table to clear PTE_RDONLY.

>  
>  	return 0;

[ ... ]

> @@ -467,6 +487,9 @@ static int handle_desc_update(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	if (wi->ha)
>  		new |= PTE_AF;
>  
> +	if (should_set_dirty_state(wi, ws, wr, access))
> +		new &= ~PTE_RDONLY;
> +
>  	if (new == old)
>  		return 0;

[Severity: Medium]
Are we failing to update the walk result (wr) after a successful hardware update?

When handle_desc_update() successfully clears PTE_RDONLY, it updates the
descriptor in guest memory and ws->desc, but wr->desc and wr->dirty are not
updated to reflect this change for walk_s1().

If kvm_translate_vncr() subsequently uses the stale wr.dirty == false, it maps
the VNCR page as PAGE_KERNEL_RO. When the guest retries the write, it triggers
a permission fault, forcing a second translation walk which then sees the updated
descriptor and maps it RW. Does this omission cause spurious secondary page
faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623184201.1518871-1-oupton@kernel.org?part=13