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@xxxxxxx> 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
