On Thu, 25 Jul 2024 16:13:25 +0100,
Alexandru Elisei <alexandru.elisei@xxxxxxx> 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@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);
>
Ah, much nicer indeed!
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
