[PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[Wei-Lin Chang]

We don't expect to ever encounter a stage-2 read permission fault,
because read permission is always granted when mapping stage-2. However,
this isn't certainly the case when NV is involved. The key is the shadow
mapping built for L2 must have the same or stricter permissions than
those that L1 built for L2, hence opening the possibility of mappings
without read permission.

Let's continue handling the read fault if we're handling a shadow
stage-2 fault, instead of erroring out.

The following illustrates an example, vertical lines stand for either
L0, L1, or L2 running, with left: L0, middle: L1, and right: L2.
Horizontal arrows stand for traps and erets between them.

'''
L0                               L1                               L2
                                               L2                 |
                                               writes to an L2 PA |
|<----------------------------------------------------------------|
|
| L0
| finds no mapping in
| L1's s2pt for L2
|
| L0
| injects data abort
|------------------------------->|
                                 | L1
                                 | for whatever reason
                                 | treats this abort specially,
                                 | only gives it W permission
                                 |
       eret traps to L0          |
|<-------------------------------|
|
|      eret back to L2
|---------------------------------------------------------------->|
                                                                  |
                                                   L2             |
                                                   writes to same |
                                                   L2 PA (replay) |
|<----------------------------------------------------------------|
|
| L0
| finds mapping in s2pt that
| L1 built for L2, create
| shadow mapping for L2,
| but only gives it W to match
| what L1 had given
|
|
|      eret back to L2
|---------------------------------------------------------------->|
                                                                  |
                                                   L2             |
                                                   writes to same |
                                                   L2 PA (replay) |
                                                   success        |
|<----------------------------------------------------------------|
|
|
|------------------------------->| L1
                                 | for some reason, relaxes
                                 | L2's permission from W
                                 | to RW, AND, doesn't flush
                                 | TLB
                                 |
       eret traps to L0          |
|<-------------------------------|
|
|      eret back to L2
|---------------------------------------------------------------->|
                                                                  |
                                                    L2            |
                                                    read to L2 PA |
|<----------------------------------------------------------------|
| stage-2 read fault
|
'''

Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
---

I am able to trigger this error with a modified L1 KVM, but I do realize
this requires L1 to be very strange (or even just wrong) so I understand
if we don't want to handle this kind of edge case. On the other hand,
could there also be other ways to trigger this that I have not thought
of?

Another thing is that this change lets L1 get away with not flushing the
TLB, but TLBs are ephemeral so it's fine in this aspect, however I'm not
sure if there are other considerations.

---
 arch/arm64/kvm/mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 1c78864767c5c..41017ca579b19 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1508,8 +1508,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
 	VM_BUG_ON(write_fault && exec_fault);
 
-	if (fault_is_perm && !write_fault && !exec_fault) {
-		kvm_err("Unexpected L2 read permission error\n");
+	if (fault_is_perm && !write_fault && !exec_fault && !nested) {
+		kvm_err("Unexpected S2 read permission error\n");
 		return -EFAULT;
 	}
 
-- 
2.50.1

Re: [PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[Oliver Upton]

Hi Wei-Lin,

You've been finding some good stuff with nested, thank you :)

On Fri, Aug 22, 2025 at 11:18:53AM +0800, Wei-Lin Chang wrote:
> I am able to trigger this error with a modified L1 KVM, but I do realize
> this requires L1 to be very strange (or even just wrong) so I understand
> if we don't want to handle this kind of edge case. On the other hand,
> could there also be other ways to trigger this that I have not thought
> of?

The architecture is pretty unambiguous here that the stage-2 can
represent a translation w/o read permission.

> Another thing is that this change lets L1 get away with not flushing the
> TLB, but TLBs are ephemeral so it's fine in this aspect, however I'm not
> sure if there are other considerations.

FEAT_ETS3 is an interesting one since it provides software with ordering
expectations around MMU faults (including permission faults) and updates
to the translation tables. For KVM's shadow stage-2 to comply we need to
re-walk the guest's stage-2 before injecting the fault at L1.

> ---
>  arch/arm64/kvm/mmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 1c78864767c5c..41017ca579b19 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1508,8 +1508,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
>  	VM_BUG_ON(write_fault && exec_fault);
>  
> -	if (fault_is_perm && !write_fault && !exec_fault) {
> -		kvm_err("Unexpected L2 read permission error\n");
> +	if (fault_is_perm && !write_fault && !exec_fault && !nested) {
> +		kvm_err("Unexpected S2 read permission error\n");
>  		return -EFAULT;
>  	}

Hmm... I'm also willing to just delete this check altogether. The
likelihood of KVM creating a stage-2 PTE w/o read permission is rather
low.

Thanks,
Oliver

Re: [PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[Wei-Lin Chang]

Hi Oliver,

On Fri, Aug 22, 2025 at 02:25:43AM -0700, Oliver Upton wrote:
> Hi Wei-Lin,
> 
> You've been finding some good stuff with nested, thank you :)
> 
> On Fri, Aug 22, 2025 at 11:18:53AM +0800, Wei-Lin Chang wrote:
> > I am able to trigger this error with a modified L1 KVM, but I do realize
> > this requires L1 to be very strange (or even just wrong) so I understand
> > if we don't want to handle this kind of edge case. On the other hand,
> > could there also be other ways to trigger this that I have not thought
> > of?
> 
> The architecture is pretty unambiguous here that the stage-2 can
> represent a translation w/o read permission.
> 
> > Another thing is that this change lets L1 get away with not flushing the
> > TLB, but TLBs are ephemeral so it's fine in this aspect, however I'm not
> > sure if there are other considerations.
> 
> FEAT_ETS3 is an interesting one since it provides software with ordering
> expectations around MMU faults (including permission faults) and updates
> to the translation tables. For KVM's shadow stage-2 to comply we need to
> re-walk the guest's stage-2 before injecting the fault at L1.

Thanks for telling me this, I haven't noticed ETS before, let me learn
about it.

> 
> > ---
> >  arch/arm64/kvm/mmu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 1c78864767c5c..41017ca579b19 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1508,8 +1508,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >  	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
> >  	VM_BUG_ON(write_fault && exec_fault);
> >  
> > -	if (fault_is_perm && !write_fault && !exec_fault) {
> > -		kvm_err("Unexpected L2 read permission error\n");
> > +	if (fault_is_perm && !write_fault && !exec_fault && !nested) {
> > +		kvm_err("Unexpected S2 read permission error\n");
> >  		return -EFAULT;
> >  	}
> 
> Hmm... I'm also willing to just delete this check altogether. The
> likelihood of KVM creating a stage-2 PTE w/o read permission is rather
> low.

Agreed, and this is in agreement with Marc, let me drop this in the next
version.

Thanks,
Wei-Lin Chang

> 
> Thanks,
> Oliver

Re: [PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[Marc Zyngier]

Hey Wei-Lin,

On Fri, 22 Aug 2025 04:18:53 +0100,
Wei-Lin Chang <r09922117@csie.ntu.edu.tw> wrote:
> 
> We don't expect to ever encounter a stage-2 read permission fault,
> because read permission is always granted when mapping stage-2. However,
> this isn't certainly the case when NV is involved. The key is the shadow
> mapping built for L2 must have the same or stricter permissions than
> those that L1 built for L2, hence opening the possibility of mappings
> without read permission.
> 
> Let's continue handling the read fault if we're handling a shadow
> stage-2 fault, instead of erroring out.
> 
> The following illustrates an example, vertical lines stand for either
> L0, L1, or L2 running, with left: L0, middle: L1, and right: L2.
> Horizontal arrows stand for traps and erets between them.
> 
> '''
> L0                               L1                               L2
>                                                L2                 |
>                                                writes to an L2 PA |
> |<----------------------------------------------------------------|
> |
> | L0
> | finds no mapping in
> | L1's s2pt for L2
> |
> | L0
> | injects data abort
> |------------------------------->|
>                                  | L1
>                                  | for whatever reason
>                                  | treats this abort specially,
>                                  | only gives it W permission
>                                  |
>        eret traps to L0          |
> |<-------------------------------|
> |
> |      eret back to L2
> |---------------------------------------------------------------->|
>                                                                   |
>                                                    L2             |
>                                                    writes to same |
>                                                    L2 PA (replay) |
> |<----------------------------------------------------------------|
> |
> | L0
> | finds mapping in s2pt that
> | L1 built for L2, create
> | shadow mapping for L2,
> | but only gives it W to match
> | what L1 had given
> |
> |
> |      eret back to L2
> |---------------------------------------------------------------->|
>                                                                   |
>                                                    L2             |
>                                                    writes to same |
>                                                    L2 PA (replay) |
>                                                    success        |
> |<----------------------------------------------------------------|
> |
> |
> |------------------------------->| L1
>                                  | for some reason, relaxes
>                                  | L2's permission from W
>                                  | to RW, AND, doesn't flush
>                                  | TLB
>                                  |
>        eret traps to L0          |
> |<-------------------------------|
> |
> |      eret back to L2
> |---------------------------------------------------------------->|
>                                                                   |
>                                                     L2            |
>                                                     read to L2 PA |
> |<----------------------------------------------------------------|
> | stage-2 read fault
> |
> '''
> 
> Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>

Excellent detective work! Bonus points for the ASCII art -- I'm not
sure how practical it will be to keep it in the final commit, but this
definitely helps understanding the issue.

> ---
> 
> I am able to trigger this error with a modified L1 KVM, but I do realize
> this requires L1 to be very strange (or even just wrong) so I understand
> if we don't want to handle this kind of edge case. On the other hand,
> could there also be other ways to trigger this that I have not thought
> of?
> 
> Another thing is that this change lets L1 get away with not flushing the
> TLB, but TLBs are ephemeral so it's fine in this aspect, however I'm not
> sure if there are other considerations.

Well, it depends whose TLBs we're talking about. The CPU TLBs are
definitely ephemeral. But the KVM-managed TLBs (aka the shadow S2) is
pretty static, and I am a bit worried to relax this.

What would happen on actual HW? L1 would take a S2 fault, because the
TLBs are out of sync with the S2 PTs. And yes, maybe the TLB pressure
woulds make it so that it works *sometimes*, but there wouldn't be any
guarantee of forward progress as long as EL2 doesn't issue a TLBI.

I reckon we should implement that particular behaviour whenever
possible, and handle permission faults early, possibly ahead of the
guest S2 walk (the way we handle VNCR_EL2 faults is IMO a good model).

This would imply taking the guest's S2 permission at face value, and
only drop W permission when the memslot is RO -- you'd then need to
keep track of the original W bit somewhere. And that's where things
become much harder, because KVM can decide to remap arbitrary ranges
of IPA space as RO, which implies we should track the W bit at all
times, most likely as one of the SW bits in the PTE.

We'll need exactly that if we ever want to implement the
Hardware-managed Dirty Bit, but I have the feeling we need an actual
design for this, and not a quick hack. Your approach is therefore the
correct one for the time being.

> 
> ---
>  arch/arm64/kvm/mmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 1c78864767c5c..41017ca579b19 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1508,8 +1508,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
>  	VM_BUG_ON(write_fault && exec_fault);
>  
> -	if (fault_is_perm && !write_fault && !exec_fault) {
> -		kvm_err("Unexpected L2 read permission error\n");
> +	if (fault_is_perm && !write_fault && !exec_fault && !nested) {
> +		kvm_err("Unexpected S2 read permission error\n");
>  		return -EFAULT;
>  	}
>  

Honestly, I'd rather kill this check altogether rather than adding
another condition to it -- I suggested it to Fuad a while ago. This is
the first time we ever see it firing, and I don't think it is bringing
much.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[Wei-Lin Chang]

Hi Marc,

On Fri, Aug 22, 2025 at 10:40:07AM +0100, Marc Zyngier wrote:
> Hey Wei-Lin,
> 
> On Fri, 22 Aug 2025 04:18:53 +0100,
> Wei-Lin Chang <r09922117@csie.ntu.edu.tw> wrote:
> > 
> > We don't expect to ever encounter a stage-2 read permission fault,
> > because read permission is always granted when mapping stage-2. However,
> > this isn't certainly the case when NV is involved. The key is the shadow
> > mapping built for L2 must have the same or stricter permissions than
> > those that L1 built for L2, hence opening the possibility of mappings
> > without read permission.
> > 
> > Let's continue handling the read fault if we're handling a shadow
> > stage-2 fault, instead of erroring out.
> > 
> > The following illustrates an example, vertical lines stand for either
> > L0, L1, or L2 running, with left: L0, middle: L1, and right: L2.
> > Horizontal arrows stand for traps and erets between them.
> > 
> > '''
> > L0                               L1                               L2
> >                                                L2                 |
> >                                                writes to an L2 PA |
> > |<----------------------------------------------------------------|
> > |
> > | L0
> > | finds no mapping in
> > | L1's s2pt for L2
> > |
> > | L0
> > | injects data abort
> > |------------------------------->|
> >                                  | L1
> >                                  | for whatever reason
> >                                  | treats this abort specially,
> >                                  | only gives it W permission
> >                                  |
> >        eret traps to L0          |
> > |<-------------------------------|
> > |
> > |      eret back to L2
> > |---------------------------------------------------------------->|
> >                                                                   |
> >                                                    L2             |
> >                                                    writes to same |
> >                                                    L2 PA (replay) |
> > |<----------------------------------------------------------------|
> > |
> > | L0
> > | finds mapping in s2pt that
> > | L1 built for L2, create
> > | shadow mapping for L2,
> > | but only gives it W to match
> > | what L1 had given
> > |
> > |
> > |      eret back to L2
> > |---------------------------------------------------------------->|
> >                                                                   |
> >                                                    L2             |
> >                                                    writes to same |
> >                                                    L2 PA (replay) |
> >                                                    success        |
> > |<----------------------------------------------------------------|
> > |
> > |
> > |------------------------------->| L1
> >                                  | for some reason, relaxes
> >                                  | L2's permission from W
> >                                  | to RW, AND, doesn't flush
> >                                  | TLB
> >                                  |
> >        eret traps to L0          |
> > |<-------------------------------|
> > |
> > |      eret back to L2
> > |---------------------------------------------------------------->|
> >                                                                   |
> >                                                     L2            |
> >                                                     read to L2 PA |
> > |<----------------------------------------------------------------|
> > | stage-2 read fault
> > |
> > '''
> > 
> > Signed-off-by: Wei-Lin Chang <r09922117@csie.ntu.edu.tw>
> 
> Excellent detective work! Bonus points for the ASCII art -- I'm not
> sure how practical it will be to keep it in the final commit, but this
> definitely helps understanding the issue.

Thanks, I tried to make it clear for those that aren't familiar with NV,
including myself :P

> 
> > ---
> > 
> > I am able to trigger this error with a modified L1 KVM, but I do realize
> > this requires L1 to be very strange (or even just wrong) so I understand
> > if we don't want to handle this kind of edge case. On the other hand,
> > could there also be other ways to trigger this that I have not thought
> > of?
> > 
> > Another thing is that this change lets L1 get away with not flushing the
> > TLB, but TLBs are ephemeral so it's fine in this aspect, however I'm not
> > sure if there are other considerations.
> 
> Well, it depends whose TLBs we're talking about. The CPU TLBs are
> definitely ephemeral. But the KVM-managed TLBs (aka the shadow S2) is
> pretty static, and I am a bit worried to relax this.
> 
> What would happen on actual HW? L1 would take a S2 fault, because the
> TLBs are out of sync with the S2 PTs. And yes, maybe the TLB pressure
> woulds make it so that it works *sometimes*, but there wouldn't be any
> guarantee of forward progress as long as EL2 doesn't issue a TLBI.
> 
> I reckon we should implement that particular behaviour whenever
> possible, and handle permission faults early, possibly ahead of the
> guest S2 walk (the way we handle VNCR_EL2 faults is IMO a good model).

The parts above make complete sense.

> 
> This would imply taking the guest's S2 permission at face value, and
> only drop W permission when the memslot is RO -- you'd then need to
> keep track of the original W bit somewhere. And that's where things
> become much harder, because KVM can decide to remap arbitrary ranges
> of IPA space as RO, which implies we should track the W bit at all
> times, most likely as one of the SW bits in the PTE.

But sorry, I struggle to understand this paragraph after reading it many
times, probably my experience with the code isn't enough for me to make
the connection. Why are we talking about the W bit suddenly?
If you don't mind, can you reword what's discussed here?
I only very vaguely get that there will be 2 W bits, one from what L1 set,
and one from the L0 memslot, if I didn't completely miss the point..

> 
> We'll need exactly that if we ever want to implement the
> Hardware-managed Dirty Bit, but I have the feeling we need an actual
> design for this, and not a quick hack. Your approach is therefore the
> correct one for the time being.
> 
> > 
> > ---
> >  arch/arm64/kvm/mmu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 1c78864767c5c..41017ca579b19 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -1508,8 +1508,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> >  	exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
> >  	VM_BUG_ON(write_fault && exec_fault);
> >  
> > -	if (fault_is_perm && !write_fault && !exec_fault) {
> > -		kvm_err("Unexpected L2 read permission error\n");
> > +	if (fault_is_perm && !write_fault && !exec_fault && !nested) {
> > +		kvm_err("Unexpected S2 read permission error\n");
> >  		return -EFAULT;
> >  	}
> >  
> 
> Honestly, I'd rather kill this check altogether rather than adding
> another condition to it -- I suggested it to Fuad a while ago. This is
> the first time we ever see it firing, and I don't think it is bringing
> much.

No problem and agreed. Let me prepare another version.

Thanks,
Wei-Lin Chang

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Jazz isn't dead. It just smells funny.

Re: [PATCH] KVM: arm64: nv: Allow shadow stage 2 read fault

[Marc Zyngier]

On Tue, 26 Aug 2025 14:49:27 +0100,
Wei-Lin Chang <r09922117@csie.ntu.edu.tw> wrote:
> 
> Hi Marc,
> 
> On Fri, Aug 22, 2025 at 10:40:07AM +0100, Marc Zyngier wrote:
> >
> > This would imply taking the guest's S2 permission at face value, and
> > only drop W permission when the memslot is RO -- you'd then need to
> > keep track of the original W bit somewhere. And that's where things
> > become much harder, because KVM can decide to remap arbitrary ranges
> > of IPA space as RO, which implies we should track the W bit at all
> > times, most likely as one of the SW bits in the PTE.
> 
> But sorry, I struggle to understand this paragraph after reading it many
> times, probably my experience with the code isn't enough for me to make
> the connection. Why are we talking about the W bit suddenly?
> If you don't mind, can you reword what's discussed here?
> I only very vaguely get that there will be 2 W bits, one from what L1 set,
> and one from the L0 memslot, if I didn't completely miss the point..

Sorry, I quickly drifted into something related.

My take on this category of problems is that we're better off always
using the permissions that the guest gives us. This is the scheme that
we have adopted with VNCR. It means we wouldn't have to rewalk the
guest S2 on permission fault, since we'd be guaranteed to have the
latest update.

However, S2 management implies that a S2 mapping can be made read-only
at any point (dirty log, for example). Which means that on a
permission fault, you'd need to find out whether the page is R/O
because the guest said so, or because the host decided to make it so.

Which means that somehow you need to work out why you have taken a
permission fault. You can either

- rewalk the guest S2 as if you missed in the TLB
- or keep a copy of the W bit in the shadow SW

> > We'll need exactly that if we ever want to implement the
> > Hardware-managed Dirty Bit, but I have the feeling we need an actual
> > design for this, and not a quick hack. Your approach is therefore the
> > correct one for the time being.

And that's why I brought this up: to support HD in the guest S2, we
need to mark the full shadow S2 as R/O, and update the guest S2 on the
back of that fault.

Thanks,

	M.

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