Re: [RFC v4 12/24] target/arm/cpu: Set number of breakpoints and watchpoints in KVM

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Fri, Oct 02, 2026 at 02:06:24PM +1000, Gavin Shan wrote:
> On 9/4/26 5:35 AM, Mathieu Poirier wrote:
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > 
> > Add "num-breakpoints" and "num-watchpoints" CPU parameters to configure
> > the debug features that KVM presents to the guest. The KVM vCPU
> > configuration is modified by calling SET_ONE_REG on the ID register.
> > 
> > This is needed for Realm VMs, whose parameters include breakpoints and
> > watchpoints, and influence the Realm Initial Measurement.
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >   target/arm/arm-qmp-cmds.c |  1 +
> >   target/arm/cpu.h          |  4 ++
> >   target/arm/cpu64.c        | 81 +++++++++++++++++++++++++++++++++++++++
> >   target/arm/kvm.c          | 56 +++++++++++++++++++++++++++
> >   4 files changed, 142 insertions(+)
> > 
> > diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> > index 83ec95c290f6..9ed65eb090d7 100644
> > --- a/target/arm/arm-qmp-cmds.c
> > +++ b/target/arm/arm-qmp-cmds.c
> > @@ -76,6 +76,7 @@ static const char *cpu_model_advertised_features[] = {
> >       "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> >       "kvm-no-adjvtime", "kvm-steal-time",
> >       "pauth", "pauth-impdef", "pauth-qarma3", "pauth-qarma5",
> > +    "num-breakpoints", "num-watchpoints",
> >       NULL
> >   };
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 6bbc942f9a59..d256d66a3e1b 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1173,6 +1173,10 @@ struct ArchCPU {
> >       /* Generic timer counter frequency, in Hz */
> >       uint64_t gt_cntfrq_hz;
> > +
> > +    /* Allows to override the default configuration */
> > +    uint8_t num_bps;
> > +    uint8_t num_wps;
> >   };
> 
> May improve the comments to indicate: these two fields are only needed
> by RME-vCPUs.
> 
>        /* Override the default configuration, only for RME vCPUs */
> 
> >   typedef struct ARMCPUInfo {
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 4e8c47253086..18adb06d78ad 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -666,6 +666,86 @@ void aarch64_add_pauth_properties(Object *obj)
> >       }
> >   }
> > +#if defined(CONFIG_KVM)
> > +static void arm_cpu_get_num_wps(Object *obj, Visitor *v, const char *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    uint8_t val;
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    val = cpu->num_wps;
> > +    if (val == 0) {
> > +        val = FIELD_EX64(cpu->isar.idregs[ID_AA64DFR0_EL1_IDX],
> > +                         ID_AA64DFR0, WRPS) + 1;
> > +    }
> > +
> > +    visit_type_uint8(v, name, &val, errp);
> > +}
> > +
> > +static void arm_cpu_set_num_wps(Object *obj, Visitor *v, const char *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    uint8_t val;
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    uint8_t max_wps = FIELD_EX64(cpu->isar.idregs[ID_AA64DFR0_EL1_IDX],
> > +                                 ID_AA64DFR0, WRPS) + 1;
> > +
> > +    if (!visit_type_uint8(v, name, &val, errp)) {
> > +        return;
> > +    }
> > +
> > +    if (val < 2 || val > max_wps) {
> > +        error_setg(errp, "invalid number of watchpoints");
> > +        return;
> > +   }
> > +
> > +    cpu->num_wps = val;
> > +}
> > +
> > +static void arm_cpu_get_num_bps(Object *obj, Visitor *v, const char *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    uint8_t val;
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    val = cpu->num_bps;
> > +    if (val == 0) {
> > +        val = FIELD_EX64(cpu->isar.idregs[ID_AA64DFR0_EL1_IDX],
> > +                         ID_AA64DFR0, BRPS) + 1;
> > +    }
> > +
> > +    visit_type_uint8(v, name, &val, errp);
> > +}
> > +
> > +static void arm_cpu_set_num_bps(Object *obj, Visitor *v, const char *name,
> > +                                void *opaque, Error **errp)
> > +{
> > +    uint8_t val;
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +    uint8_t max_bps = FIELD_EX64(cpu->isar.idregs[ID_AA64DFR0_EL1_IDX],
> > +                                 ID_AA64DFR0, BRPS) + 1;
> > +
> > +    if (!visit_type_uint8(v, name, &val, errp)) {
> > +        return;
> > +    }
> > +
> > +    if (val < 2 || val > max_bps) {
> > +        error_setg(errp, "invalid number of breakpoints");
>           ^^^^^
> 
> Code alignment issue here.
> 
> > +       return;
> > +    }
> > +
> > +    cpu->num_bps = val;
> > +}
> > +
> > +static void aarch64_add_kvm_writable_properties(Object *obj)
> > +{
> > +    object_property_add(obj, "num-breakpoints", "uint8", arm_cpu_get_num_bps,
> > +                        arm_cpu_set_num_bps, NULL, NULL);
> > +    object_property_add(obj, "num-watchpoints", "uint8", arm_cpu_get_num_wps,
> > +                        arm_cpu_set_num_wps, NULL, NULL);
> > +}
> > +#endif /* CONFIG_KVM */
> > +
> >   void aarch64_cpu_lpa2_finalize(ARMCPU *cpu, Error **errp)
> >   {
> >       uint64_t t;
> > @@ -807,6 +887,7 @@ void aarch64_host_initfn(Object *obj)
> >       kvm_arm_set_cpreg_mig_tolerances(cpu);
> >       kvm_arm_set_cpu_features_from_host(cpu);
> >       aarch64_add_sve_properties(obj);
> > +    aarch64_add_kvm_writable_properties(obj);
> >   #elif defined(CONFIG_HVF)
> >       hvf_arm_set_cpu_features_from_host(cpu);
> >   #elif defined(CONFIG_WHPX)
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index ca25b707a0a8..93686b038263 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -912,6 +912,56 @@ out:
> >       return ret;
> >   }
> > +#define KVM_REG_ARM_ID_AA64DFR0_EL1     ARM64_SYS_REG(3, 0, 0, 5, 0)
> > +
> > +static void kvm_arm_configure_aa64dfr0(ARMCPU *cpu)
> > +{
> > +    int ret;
> > +    uint64_t val, newval;
> > +    CPUState *cs = CPU(cpu);
> > +
> > +    if (!cpu->num_bps && !cpu->num_wps) {
> > +        return;
> > +    }
> > +
> > +    newval = cpu->isar.idregs[ID_AA64DFR0_EL1_IDX];
> > +    if (cpu->num_bps) {
> > +        uint64_t ctx_cmps = FIELD_EX64(newval, ID_AA64DFR0, CTX_CMPS);
> > +
> > +        /* CTX_CMPs is never greater than BRPs */
> > +        ctx_cmps = MIN(ctx_cmps, cpu->num_bps - 1);
> > +        newval = FIELD_DP64(newval, ID_AA64DFR0, BRPS, cpu->num_bps - 1);
> > +        newval = FIELD_DP64(newval, ID_AA64DFR0, CTX_CMPS, ctx_cmps);
> > +    }
> > +    if (cpu->num_wps) {
> > +        newval = FIELD_DP64(newval, ID_AA64DFR0, WRPS, cpu->num_wps - 1);
> > +    }
> > +    ret = kvm_set_one_reg(cs, KVM_REG_ARM_ID_AA64DFR0_EL1, &newval);
> > +    if (ret) {
> > +        error_report("Failed to set KVM_REG_ARM_ID_AA64DFR0_EL1");
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * Check if the write succeeded. KVM does offer the writable mask for this
> > +     * register, but this way we also check if the value we wrote was sane.
> > +     */
> > +    ret = kvm_get_one_reg(cs, KVM_REG_ARM_ID_AA64DFR0_EL1, &val);
> > +    if (ret) {
> > +        error_report("Failed to get KVM_REG_ARM_ID_AA64DFR0_EL1");
> > +        return;
> > +    }
> > +
> > +    if (val != newval) {
> > +        error_report("Failed to update KVM_REG_ARM_ID_AA64DFR0_EL1");
> > +    }
> 
> These two checks could be combined since we needn't to know how the write has
> been failing.
> 
>        if (ret || val != newval) {
>            error_report("Failed to update KVM_REG_ARM_ID_AA64DFR0_EL1");
>        }
> 
> > +}
> > +
> > +static void kvm_arm_configure_vcpu_regs(ARMCPU *cpu)
> > +{
> > +    kvm_arm_configure_aa64dfr0(cpu);
> > +}
> > +
> >   /**
> >    * kvm_arm_cpreg_level:
> >    * @regidx: KVM register index
> > @@ -1128,6 +1178,12 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
> >           fprintf(stderr, "kvm_arm_vcpu_init failed: %s\n", strerror(-ret));
> >           abort();
> >       }
> > +
> > +    /*
> > +     * Before loading the KVM values into CPUState, update the KVM configuration
> > +     */
> 
> Can be squeezed into one line:
> 
>        /* Update KVM state before it's loaded to CPUState */
> 
> > +    kvm_arm_configure_vcpu_regs(cpu);
> > +
> 
> The newly added properties ("num-breakpoints", "num-watchpoints") are only needed by
> RME-vCPUs, but they're also visible to the normal vCPUs. So do we need to bail without
> adding the properties if RME isn't supported by the vCPUs in kvm_arm_configure_vcpu_regs() ?
>

I added a check for confidential-guest-support in the get() functions of both
options.  I also addressed the rest of your comments above.

Thanks for the review,
Mathieu 
 
> >       if (!write_kvmstate_to_list(cpu)) {
> >           fprintf(stderr, "write_kvmstate_to_list failed\n");
> >           abort();
> 
> Thanks,
> Gavin
>