Re: [PATCH 10/12] KVM: arm64: nv: Add SW walker for AT S1 emulation

[Marc Zyngier <maz@kernel.org>]

On Thu, 25 Jul 2024 16:13:25 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi,
> 
> On Thu, Jul 25, 2024 at 03:30:00PM +0100, Marc Zyngier wrote:
> > On Thu, 25 Jul 2024 15:16:12 +0100,
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > > 
> > > Hi Marc,
> > > 
> > > On Mon, Jul 08, 2024 at 05:57:58PM +0100, Marc Zyngier wrote:
> > > > +	if (perm_fail) {
> > > > +		struct s1_walk_result tmp;
> > > 
> > > I was wondering if you would consider initializing 'tmp' to the empty struct
> > > here. That makes it consistent with the initialization of 'wr' in the !perm_fail
> > > case and I think it will make the code more robust wrt to changes to
> > > compute_par_s1() and what fields it accesses.
> > 
> > I think there is a slightly better way, with something like this:
> > 
> > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> > index b02d8dbffd209..36fa2801ab4ef 100644
> > --- a/arch/arm64/kvm/at.c
> > +++ b/arch/arm64/kvm/at.c
> > @@ -803,12 +803,12 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> >  	}
> >  
> >  	if (perm_fail) {
> > -		struct s1_walk_result tmp;
> > -
> > -		tmp.failed = true;
> > -		tmp.fst = ESR_ELx_FSC_PERM | wr.level;
> > -		tmp.s2 = false;
> > -		tmp.ptw = false;
> > +		struct s1_walk_result tmp = (struct s1_walk_result){
> > +			.failed	= true,
> > +			.fst	= ESR_ELx_FSC_PERM | wr.level,
> > +			.s2	= false,
> > +			.ptw	= false,
> > +		};
> >  
> >  		wr = tmp;
> >  	}
> > 
> > Thoughts?
> 
> How about (diff against your kvm-arm64/nv-at-pan-WIP branch, in case something
> looks off):
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index b02d8dbffd20..74ebe3223a13 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -802,16 +802,8 @@ static u64 handle_at_slow(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>                 BUG();
>         }
> 
> -       if (perm_fail) {
> -               struct s1_walk_result tmp;
> -
> -               tmp.failed = true;
> -               tmp.fst = ESR_ELx_FSC_PERM | wr.level;
> -               tmp.s2 = false;
> -               tmp.ptw = false;
> -
> -               wr = tmp;
> -       }
> +       if (perm_fail)
> +               fail_s1_walk(&wr, ESR_ELx_FSC_PERM | wr.level, false, false);
> 
>  compute_par:
>         return compute_par_s1(vcpu, &wr);
> 

Ah, much nicer indeed!

Thanks,

	M.

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