[RFC PATCH v3 04/29] KVM: arm64: Make ID_AA64PFR0_EL1 writable

[Reiji Watanabe]

Hi Eric,

On Thu, Dec 2, 2021 at 5:02 AM Eric Auger <eauger at redhat.com> wrote:
>
> Hi Reiji,
>
> On 11/30/21 2:29 AM, Reiji Watanabe wrote:
> > Hi Eric,
> >
> > On Thu, Nov 25, 2021 at 7:35 AM Eric Auger <eauger at redhat.com> wrote:
> >>
> >> Hi Reiji,
> >>
> >> On 11/17/21 7:43 AM, Reiji Watanabe wrote:
> >>> This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by
> >>> userspace.
> >>>
> >>> The CSV2/CSV3 fields of the register were already writable and values
> >>> that were written for them affected all vCPUs before. Now they only
> >>> affect the vCPU.
> >>> Return an error if userspace tries to set SVE/GIC field of the register
> >>> to a value that conflicts with SVE/GIC configuration for the guest.
> >>> SIMD/FP/SVE fields of the requested value are validated according to
> >>> Arm ARM.
> >>>
> >>> Signed-off-by: Reiji Watanabe <reijiw at google.com>
> >>> ---
> >>>  arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++--------------
> >>>  1 file changed, 103 insertions(+), 56 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >>> index 1552cd5581b7..35400869067a 100644
> >>> --- a/arch/arm64/kvm/sys_regs.c
> >>> +++ b/arch/arm64/kvm/sys_regs.c
> >>> @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg)
> >>>               id_reg->init(id_reg);
> >>>  }
> >>>
> >>> +#define      kvm_has_gic3(kvm)               \
> >>> +     (irqchip_in_kernel(kvm) &&      \
> >>> +      (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >> you may move this macro to kvm/arm_vgic.h as this may be used in
> >> vgic/vgic-v3.c too
> >
> > Thank you for the suggestion. I will move that to kvm/arm_vgic.h.
> >
> >
> >>> +
> >>> +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> >>> +                                 const struct id_reg_info *id_reg, u64 val)
> >>> +{
> >>> +     int fp, simd;
> >>> +     bool vcpu_has_sve = vcpu_has_sve(vcpu);
> >>> +     bool pfr0_has_sve = id_aa64pfr0_sve(val);
> >>> +     int gic;
> >>> +
> >>> +     simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
> >>> +     fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
> >>> +     if (simd != fp)
> >>> +             return -EINVAL;
> >>> +
> >>> +     /* fp must be supported when sve is supported */
> >>> +     if (pfr0_has_sve && (fp < 0))
> >>> +             return -EINVAL;
> >>> +
> >>> +     /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */
> >>> +     if (vcpu_has_sve ^ pfr0_has_sve)
> >>> +             return -EPERM;
> >>> +
> >>> +     gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT);
> >>> +     if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm))
> >>> +             return -EPERM;
> >>
> >> Sometimes from a given architecture version, some lower values are not
> >> allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3.
> >> Shouldn't we handle that kind of check?
> >
> > As far as I know, there is no way for guests to identify the
> > architecture revision (e.g. v8.1, v8.2, etc).  It might be able
> > to indirectly infer the revision though (from features that are
> > available or etc).
>
> OK. That sounds weird to me as we do many checks accross different IDREG
> settings but we may eventually have a wrong "CPU model" exposed by the
> user space violating those spec revision minima. Shouldn't we introduce
> some way for the userspace to provide his requirements? via new VCPU
> targets for instance?

Thank you for sharing your thoughts and providing the suggestion !

Does the "new vCPU targets" mean Armv8.0, armv8.1, and so on ?

The ID registers' consistency checking in the series is to not
promise more to userspace than what KVM (on the host) can provide,
and to not expose ID register values that are not supported on
any ARM v8 architecture for guests (I think those are what the
current KVM is trying to assure).  I'm not trying to have KVM
provide full consistency checking of ID registers to completely
prevent userspace's bugs in setting ID registers.

I agree that it's quite possible that userspace exposes such wrong
CPU models, and KVM's providing more consistency checking would be
nicer in general.  But should it be KVM's responsibility to completely
prevent such ID register issues due to userspace bugs ?

Honestly, I'm a bit reluctant to do that so far yet:)
If that is something useful that userspace or we (KVM developers)
really want or need, or such userspace issue could affect KVM,
I would be happy to add such extra consistency checking though.

Thanks,
Reiji