Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host

[Marc Zyngier <maz@kernel.org>]

Hi Richard,

On Wed, 05 Jan 2022 21:36:55 +0000,
Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> On 1/3/22 10:05 AM, Marc Zyngier wrote:
> > -        /*
> > -         * KVM does not support modifications to this feature.
> > -         * We have not registered the cpu properties when KVM
> > -         * is in use, so the user will not be able to set them.
> > -         */
> > -        if (!kvm_enabled()) {
> > -            arm_cpu_pauth_finalize(cpu, &local_err);
> > -            if (local_err != NULL) {
> > +	arm_cpu_pauth_finalize(cpu, &local_err);
> > +	if (local_err != NULL) {
> >                   error_propagate(errp, local_err);
> >                   return;
> > -            }
> > -        }
> > +	}
> 
> Looks like the indentation is off?

Most probably. I only just discovered how to use the QEMU style for
Emacs, and was indenting things by hand before that (yes, pretty
painful and likely to lead to issues (there is a TAB instead of a set
of spaces there...).

> 
> > +static bool kvm_arm_pauth_supported(void)
> > +{
> > +    return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
> > +            kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
> > +}
> 
> Do we really need to have them both set to play the game?  Given that
> the only thing that happens is that we disable whatever host support
> exists, can we have "pauth enabled" mean whatever subset the host has?

The host will always expose either both features or none, and that's
part of the ABI. From the bit of kernel documentation located in
Documentation/virt/kvm/api.rst:

<quote>
4.82 KVM_ARM_VCPU_INIT
----------------------
[...]
        - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.

        - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
          for arm64 only.
          Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
          If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
          both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
          KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
          requested.
</quote>

KVM will reject the initialisation if only one of the features is
requested, so checking and enabling both makes sense to me.

> 
> > @@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> >        */
> >       struct kvm_vcpu_init init = { .target = -1, };
> >   +    /*
> > +     * Ask for Pointer Authentication if supported. We can't play the
> > +     * SVE trick of synthetising the ID reg as KVM won't tell us
> 
> synthesizing

Yup.

> 
> > +     * whether we have the architected or IMPDEF version of PAuth, so
> > +     * we have to use the actual ID regs.
> > +     */
> > +    if (kvm_arm_pauth_supported()) {
> > +        init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
> > +			     1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);
> 
> Align the two 1's.

Gah, another of these... Will fix.

> 
> Otherwise, it looks good.

Thanks,

	M.

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