| Message ID | 20230710192430.1992246-4-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable writable for idregs DFR0,PFR0, MMFR{0,1,2, 3} | expand |
Jing, On Mon, Jul 10, 2023 at 07:24:26PM +0000, Jing Zhang wrote: > From: Oliver Upton <oliver.upton@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@linux.dev> > Signed-off-by: Jing Zhang <jingzhangos@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.
Hi Oliver, On Mon, Jul 10, 2023 at 1:18 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > Jing, > > On Mon, Jul 10, 2023 at 07:24:26PM +0000, Jing Zhang wrote: > > From: Oliver Upton <oliver.upton@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@linux.dev> > > Signed-off-by: Jing Zhang <jingzhangos@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
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 0160ef9cfe18..c44504038ae9 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1496,6 +1496,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, u64 val) { + u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val); u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val); /* @@ -1515,6 +1516,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) val &= ~ID_AA64DFR0_EL1_PMUVer_MASK; + /* + * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a + * nonzero minimum safe value. + */ + if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP) + return -EINVAL; + return set_id_reg(vcpu, rd, val); } @@ -1536,6 +1544,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, u64 val) { u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val); + u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val); if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) { val &= ~ID_DFR0_EL1_PerfMon_MASK; @@ -1551,6 +1560,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu, if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3) return -EINVAL; + if (copdbg < ID_DFR0_EL1_CopDbg_Armv8) + return -EINVAL; + return set_id_reg(vcpu, rd, val); }