Hi Oliver,
On Mon, Jul 10, 2023 at 1:18 PM Oliver Upton <oliver.upton@xxxxxxxxx> wrote:
>
> Jing,
>
> On Mon, Jul 10, 2023 at 07:24:26PM +0000, Jing Zhang wrote:
> > From: Oliver Upton <oliver.upton@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
>
> 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
