[PATCH v5 3/6] KVM: arm64: Reject attempts to set invalid debug arch version

[Jing Zhang]

Hi Oliver,

On Mon, Jul 10, 2023 at 1:18 PM Oliver Upton <oliver.upton at linux.dev> wrote:
>
> Jing,
>
> On Mon, Jul 10, 2023 at 07:24:26PM +0000, Jing Zhang wrote:
> > From: Oliver Upton <oliver.upton at linux.dev>
> >
> > The debug architecture is mandatory in ARMv8, so KVM should not allow
> > userspace to configure a vCPU with less than that. Of course, this isn't
> > handled elegantly by the generic ID register plumbing, as the respective
> > ID register fields have a nonzero starting value.
> >
> > Add an explicit check for debug versions less than v8 of the
> > architecture.
> >
> > Signed-off-by: Oliver Upton <oliver.upton at linux.dev>
> > Signed-off-by: Jing Zhang <jingzhangos at google.com>
>
> This patch should be ordered before the change that permits writes to
> the DebugVer field (i.e. the previous patch). While we're at it, there's
> another set of prerequisites for actually making the field writable.
>
> As Suraj pointed out earlier, we need to override the type of the field
> to be FTR_LOWER_SAFE instead of EXACT. Beyond that, KVM limits the field
> to 0x6, which is the minimum value for an ARMv8 implementation. We can
> relax this limitation up to v8p8, as I believe all of the changes are to
> external debug and wouldn't affect a KVM guest.
>
> Below is my (untested) diff on top of your series for both of these
> changes.
>
> --
> Thanks,
> Oliver
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 78ccc95624fa..35c4ab8cb5c8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1216,8 +1216,14 @@ static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
>         /* Some features have different safe value type in KVM than host features */
>         switch (id) {
>         case SYS_ID_AA64DFR0_EL1:
> -               if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT)
> +               switch (kvm_ftr.shift) {
> +               case ID_AA64DFR0_EL1_PMUVer_SHIFT:
>                         kvm_ftr.type = FTR_LOWER_SAFE;
> +                       break;
> +               case ID_AA64DFR0_EL1_DebugVer_SHIFT:
> +                       kvm_ftr.type = FTR_LOWER_SAFE;
> +                       break;
> +               }
>                 break;
>         case SYS_ID_DFR0_EL1:
>                 if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT)
> @@ -1466,14 +1472,22 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>         return val;
>  }
>
> +#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)                                \
> +({                                                                             \
> +       u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);                     \
> +       (val) &= ~reg##_##field##_MASK;                                         \
> +       (val) |= FIELD_PREP(reg##_##field##_MASK,                               \
> +                         min(__f_val, (u64)reg##_##field##_##limit));          \
> +       (val);                                                                  \
> +})
> +
>  static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>                                           const struct sys_reg_desc *rd)
>  {
>         u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>
>         /* Limit debug to ARMv8.0 */
> -       val &= ~ID_AA64DFR0_EL1_DebugVer_MASK;
> -       val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DebugVer, IMP);
> +       val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
>
>         /*
>          * Only initialize the PMU version if the vCPU was configured with one.
> @@ -1529,6 +1543,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
>         u8 perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
>         u64 val = read_sanitised_ftr_reg(SYS_ID_DFR0_EL1);
>
> +       val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
> +
>         val &= ~ID_DFR0_EL1_PerfMon_MASK;
>         if (kvm_vcpu_has_pmu(vcpu))
>                 val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
>
Thanks for the details. Will improve it in the next version.

Jing