[PATCH v1] KVM: arm64: Allocate for maximum vector length for pKVM host sve state

[Fuad Tabba]

Hi Mark,

On Wed, Jun 5, 2024 at 5:52 PM Mark Brown <broonie at kernel.org> wrote:
>
> On Wed, Jun 05, 2024 at 05:14:23PM +0100, Fuad Tabba wrote:
> > On Wed, Jun 5, 2024 at 4:46 PM Mark Brown <broonie at kernel.org> wrote:
> > > On Wed, Jun 05, 2024 at 04:03:12PM +0100, Fuad Tabba wrote:
>
> > > > +     /* The maximum vl encountered for any cpu in the system. */
> > > > +     int max_system_vl;
> > > > +
>
> > > As previously mentioned I'd really prefer not to call this _system_ due
> > > to the potential confusion with the maximum VL we're using.  Naming is
> > > hard.
>
> > I agree :) I have no strong preference really. It's just that for me
> > max_cpu_vl sounds like it's per cpu. But now I'm just bikeshedding...
>
> seen?
>
> > > > @@ -1031,7 +1031,12 @@ static void vec_probe_vqs(struct vl_info *info,
> > > >
> > > >               vq = sve_vq_from_vl(vl); /* skip intervening lengths */
> > > >               set_bit(__vq_to_bit(vq), map);
> > > > +
> > > > +             if (!max_vl)
> > > > +                     max_vl = vl;
> > > >       }
> > > > +
> > > > +     info->max_system_vl = max((int) max_vl, info->max_system_vl);
>
> > > It's not clear to me why we stage this in a local variable prior to
> > > updating the global value, and like I say it does feel cleaner and safer
> > > to keep the current split between enumeration of the PE and integration
> > > of the results.  This should *work* but it doesn't feel consistent with
> > > the rest of the code.
>
> > To me it felt more clear like this, but I'm fine with not doing the
> > staging first.
>
> That was the start of the thought path that lead me to implement based
> on splitting it out of vec_probe_vqs() like the rest of the code does,
> the logic doesn't really fit in here for multiple reasons.

The alternative entails duplicating code in vec_init_vq_map() and
vec_update_vq_map().

To me at least, it's straightforward to understand how max_vl is
computed if we set it as soon as we find it. This as opposed to first
having the vq bits set in a map, then in two different functions find
the first set bit in the map, which really corresponds to the vq, to
then convert it back to a vl.

Thanks,
/fuad



> >
> > >
> > > > -               kvm_host_sve_max_vl = sve_max_vl();
> > > > +               kvm_host_sve_max_vl = sve_max_system_vl();
> > >
> > > I appreciate that we're adding a host_ on the front of it but it would
> > > feel safer to keep the name of the hypervisor copy of the value
> > > consistent with where we copied it from, sve_max_vl is a different thing
> > > in the host kernel.
> >
> > I agree, this should be renamed.
> >
> > Thanks!
> > /fuad