[2/4] KVM: arm64: Don't serialize if the access flag isn't set

  Of course, if the PTE wasn't changed then there are absolutely no
serialization requirements. Skip the DSB for an unsuccessful update to
the access flag.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
  

On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> Of course, if the PTE wasn't changed then there are absolutely no
> serialization requirements. Skip the DSB for an unsuccessful update to
> the access flag.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/pgtable.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index b11cf2c618a6..9626f615d9b8 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1094,9 +1094,13 @@ int kvm_pgtable_stage2_wrprotect(struct kvm_pgtable *pgt, u64 addr, u64 size)
>  kvm_pte_t kvm_pgtable_stage2_mkyoung(struct kvm_pgtable *pgt, u64 addr)
>  {
>  	kvm_pte_t pte = 0;
> -	stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> -				 &pte, NULL, 0);
> -	dsb(ishst);
> +	int ret;
> +
> +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> +				       &pte, NULL, 0);
> +	if (!ret)
> +		dsb(ishst);

At the moment, the only reason for stage2_update_leaf_attrs() to not
update the PTE is if it's not valid:

	if (!kvm_pte_valid(pte))
			return 0;

I guess you could check that as well:

+	if (!ret || kvm_pte_valid(pte))
+		dsb(ishst);

> +
>  	return pte;
>  }
>  
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
> 
>
  

Hi Ricardo,

Thanks for having a look.

On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:

[...]

> > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > +				       &pte, NULL, 0);
> > +	if (!ret)
> > +		dsb(ishst);
> 
> At the moment, the only reason for stage2_update_leaf_attrs() to not
> update the PTE is if it's not valid:
> 
> 	if (!kvm_pte_valid(pte))
> 			return 0;
> 
> I guess you could check that as well:
> 
> +	if (!ret || kvm_pte_valid(pte))
> +		dsb(ishst);

Thanks for catching this.

Instead of pivoting on the returned PTE value, how about we return
-EAGAIN from the early return in stage2_attr_walker()? It would better
match the pattern used elsewhere in the pgtable code.

--
Thanks,
Oliver
  

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

That works, although I would use another return code (e.g., EINVAL)? as
that's not exactly a "try again" type of error.

> 
> --
> Thanks,
> Oliver
  

On Wed, 30 Nov 2022 01:23:20 +0000,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > Hi Ricardo,
> > 
> > Thanks for having a look.
> > 
> > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > 
> > [...]
> > 
> > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > +				       &pte, NULL, 0);
> > > > +	if (!ret)
> > > > +		dsb(ishst);
> > > 
> > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > update the PTE is if it's not valid:
> > > 
> > > 	if (!kvm_pte_valid(pte))
> > > 			return 0;
> > > 
> > > I guess you could check that as well:
> > > 
> > > +	if (!ret || kvm_pte_valid(pte))
> > > +		dsb(ishst);
> > 
> > Thanks for catching this.
> > 
> > Instead of pivoting on the returned PTE value, how about we return
> > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > match the pattern used elsewhere in the pgtable code.
> 
> That works, although I would use another return code (e.g., EINVAL)? as
> that's not exactly a "try again" type of error.

EINVAL usually is an indication of something that went horribly wrong.

But is that really a failure mode? Here, failing to update the PTE
should not be considered a failure, but just a benign race: access
fault being taken on a CPU and the page being evicted on another (not
unlikely, as the page was marked old before).

And if I'm correct above, this is definitely a "try again" situation:
you probably won't take the same type of fault the second time though.

Thanks,

	M.
  

On Wed, Nov 30, 2022 at 08:21:17AM +0000, Marc Zyngier wrote:
> On Wed, 30 Nov 2022 01:23:20 +0000,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> > > Hi Ricardo,
> > > 
> > > Thanks for having a look.
> > > 
> > > On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > > > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> > > 
> > > [...]
> > > 
> > > > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > > > +				       &pte, NULL, 0);
> > > > > +	if (!ret)
> > > > > +		dsb(ishst);
> > > > 
> > > > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > > > update the PTE is if it's not valid:
> > > > 
> > > > 	if (!kvm_pte_valid(pte))
> > > > 			return 0;
> > > > 
> > > > I guess you could check that as well:
> > > > 
> > > > +	if (!ret || kvm_pte_valid(pte))
> > > > +		dsb(ishst);
> > > 
> > > Thanks for catching this.
> > > 
> > > Instead of pivoting on the returned PTE value, how about we return
> > > -EAGAIN from the early return in stage2_attr_walker()? It would better
> > > match the pattern used elsewhere in the pgtable code.
> > 
> > That works, although I would use another return code (e.g., EINVAL)? as
> > that's not exactly a "try again" type of error.
> 
> EINVAL usually is an indication of something that went horribly wrong.
> 
> But is that really a failure mode? Here, failing to update the PTE
> should not be considered a failure, but just a benign race: access
> fault being taken on a CPU and the page being evicted on another (not
> unlikely, as the page was marked old before).

I see, I agree, what you describe not look like a failure.

> 
> And if I'm correct above, this is definitely a "try again" situation:
> you probably won't take the same type of fault the second time though.
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
>
  

On Tue, Nov 29, 2022 at 09:15:21PM +0000, Oliver Upton wrote:
> Hi Ricardo,
> 
> Thanks for having a look.
> 
> On Tue, Nov 29, 2022 at 12:52:12PM -0800, Ricardo Koller wrote:
> > On Tue, Nov 29, 2022 at 07:19:44PM +0000, Oliver Upton wrote:
> 
> [...]
> 
> > > +	ret = stage2_update_leaf_attrs(pgt, addr, 1, KVM_PTE_LEAF_ATTR_LO_S2_AF, 0,
> > > +				       &pte, NULL, 0);
> > > +	if (!ret)
> > > +		dsb(ishst);
> > 
> > At the moment, the only reason for stage2_update_leaf_attrs() to not
> > update the PTE is if it's not valid:
> > 
> > 	if (!kvm_pte_valid(pte))
> > 			return 0;
> > 
> > I guess you could check that as well:
> > 
> > +	if (!ret || kvm_pte_valid(pte))
> > +		dsb(ishst);
> 
> Thanks for catching this.
> 
> Instead of pivoting on the returned PTE value, how about we return
> -EAGAIN from the early return in stage2_attr_walker()? It would better
> match the pattern used elsewhere in the pgtable code.

Bugh...

Returning EAGAIN has some unfortunate consequences that I've missed
until now...

The stage2 attr walker is used to handle faults as well as range-based
operations. In the former case, EAGAIN is sane as we retry execution but
the latter is not. I stupidly got hung up on write protection not
working as intended for some time.

I think that callers into the page table walker should indicate whether
or not the walk is to address a fault. If it is not,
__kvm_pgtable_visit() and __kvm_pgtable_walk() should chug along instead
of bailing for EAGAIN.

Let me mess around with this and figure out what is least ugly.

--
Thanks,
Oliver