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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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);

Thanks,
Alex