Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Andrew Jones]

On Fri, Aug 14, 2020 at 12:28:25PM -0700, Richard Henderson wrote:
> On 8/11/20 9:49 AM, Andrew Jones wrote:
> > Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID
> > register bit instead like "sve_supported" does.
> 
> On a related note, I think we have a latent bug, or at least a mis-feature:
> 
>     sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 
> 0;
> ...
>     /* Add feature bits that can't appear until after VCPU init. */
>     if (sve_supported) {
>         t = ahcf->isar.id_aa64pfr0;
>         t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
>         ahcf->isar.id_aa64pfr0 = t;
>     }
> 
> 
> Should it in fact be
> 
>     if (!sve_supported) {
>         t = ahcf->isar.id_aa64pfr0;
>         t = FIELD_DP64(t, ID_AA64PFR0, SVE, 0);
>         ahcf->isar.id_aa64pfr0 = t;
>     }
> 
> ?
> 
> Forcing the value to 1 here is going to be wrong the moment we have an SVE2
> enabled cpu.

Indeed. I think KVM should add a KVM_CAP_ARM_SVE2 cap. Then, we keep the
code similar to the way it is now, but set ID_AA64PFR0 appropriately
based on which CAPs are present. I think we probably need that CAP anyway
in order to ensure a guest that is using SVE2 cannot be migrated to a host
that only has SVE.

> 
> Similarly, SPE has more than one "enabled" value for PMSVer.
>

I'll take a look at the KVM series for SPE next week to see if
the UAPI will allow us to determine what values are possible.

Thanks for the heads up about this.

drew