diff mbox series

[v5,3/3] ppc: Enable 2nd DAWR support on p10

Message ID 20210412114433.129702-4-ravi.bangoria@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series ppc: Enable 2nd DAWR support on Power10 | expand

Commit Message

Ravi Bangoria April 12, 2021, 11:44 a.m. UTC
As per the PAPR, bit 0 of byte 64 in pa-features property indicates
availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
find whether kvm supports 2nd DAWR or not. If it's supported, allow
user to set the pa-feature bit in guest DT using cap-dawr1 machine
capability. Though, watchpoint on powerpc TCG guest is not supported
and thus 2nd DAWR is not enabled for TCG mode.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c                  |  7 ++++++-
 hw/ppc/spapr_caps.c             | 32 ++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h          |  6 +++++-
 target/ppc/cpu.h                |  2 ++
 target/ppc/kvm.c                | 12 ++++++++++++
 target/ppc/kvm_ppc.h            | 12 ++++++++++++
 target/ppc/translate_init.c.inc | 15 +++++++++++++++
 7 files changed, 84 insertions(+), 2 deletions(-)

Comments

Cédric Le Goater April 12, 2021, 1:10 p.m. UTC | #1
On 4/12/21 1:44 PM, Ravi Bangoria wrote:
> As per the PAPR, bit 0 of byte 64 in pa-features property indicates
> availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
> find whether kvm supports 2nd DAWR or not. If it's supported, allow
> user to set the pa-feature bit in guest DT using cap-dawr1 machine
> capability. Though, watchpoint on powerpc TCG guest is not supported
> and thus 2nd DAWR is not enabled for TCG mode.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/ppc/spapr.c                  |  7 ++++++-
>  hw/ppc/spapr_caps.c             | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |  6 +++++-
>  target/ppc/cpu.h                |  2 ++
>  target/ppc/kvm.c                | 12 ++++++++++++
>  target/ppc/kvm_ppc.h            | 12 ++++++++++++
>  target/ppc/translate_init.c.inc | 15 +++++++++++++++
>  7 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 73a06df3b1..6317fad973 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -238,7 +238,7 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>          0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
>          /* 54: DecFP, 56: DecI, 58: SHA */
>          0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
> -        /* 60: NM atomic, 62: RNG */
> +        /* 60: NM atomic, 62: RNG, 64: DAWR1 (ISA 3.1) */
>          0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
>      };
>      uint8_t *pa_features = NULL;
> @@ -279,6 +279,9 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>           * in pa-features. So hide it from them. */
>          pa_features[40 + 2] &= ~0x80; /* Radix MMU */
>      }
> +    if (spapr_get_cap(spapr, SPAPR_CAP_DAWR1)) {
> +        pa_features[66] |= 0x80;
> +    }
>  
>      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
>  }
> @@ -2003,6 +2006,7 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_cap_ccf_assist,
>          &vmstate_spapr_cap_fwnmi,
>          &vmstate_spapr_fwnmi,
> +        &vmstate_spapr_cap_dawr1,
>          NULL
>      }
>  };
> @@ -4542,6 +4546,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_DAWR1] = SPAPR_CAP_OFF;
>      spapr_caps_add_properties(smc);
>      smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9ea7ddd1e9..98a6b15f29 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -523,6 +523,28 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>      }
>  }
>  
> +static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
> +                               Error **errp)
> +{
> +    ERRP_GUARD();
> +    if (!val) {
> +        return; /* Disable by default */
> +    }
> +
> +    if (tcg_enabled()) {
> +        error_setg(errp, "DAWR1 not supported in TCG.");
> +        error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
> +    } else if (kvm_enabled()) {
> +        if (!kvmppc_has_cap_dawr1()) {
> +            error_setg(errp, "DAWR1 not supported by KVM.");
> +            error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
> +        } else if (kvmppc_set_cap_dawr1(val) < 0) {
> +            error_setg(errp, "Error enabling cap-dawr1 with KVM.");
> +            error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
> +        }
> +    }
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -631,6 +653,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_fwnmi_apply,
>      },
> +    [SPAPR_CAP_DAWR1] = {
> +        .name = "dawr1",
> +        .description = "Allow 2nd Data Address Watchpoint Register (DAWR1)",
> +        .index = SPAPR_CAP_DAWR1,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_dawr1_apply,
> +    },
>  };
>  
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -771,6 +802,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> +SPAPR_CAP_MIG_STATE(dawr1, SPAPR_CAP_DAWR1);
>  
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5f90bb26d5..51202b7c90 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -74,8 +74,10 @@ typedef enum {
>  #define SPAPR_CAP_CCF_ASSIST            0x09
>  /* Implements PAPR FWNMI option */
>  #define SPAPR_CAP_FWNMI                 0x0A
> +/* DAWR1 */
> +#define SPAPR_CAP_DAWR1                 0x0B
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_DAWR1 + 1)
>  
>  /*
>   * Capability Values
> @@ -366,6 +368,7 @@ struct SpaprMachineState {
>  #define H_SET_MODE_RESOURCE_SET_DAWR0           2
>  #define H_SET_MODE_RESOURCE_ADDR_TRANS_MODE     3
>  #define H_SET_MODE_RESOURCE_LE                  4
> +#define H_SET_MODE_RESOURCE_SET_DAWR1           5
>  
>  /* Flags for H_SET_MODE_RESOURCE_LE */
>  #define H_SET_MODE_ENDIAN_BIG    0
> @@ -921,6 +924,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> +extern const VMStateDescription vmstate_spapr_cap_dawr1;
>  
>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>  {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index cd02d65303..6a60416559 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1460,9 +1460,11 @@ typedef PowerPCCPU ArchCPU;
>  #define SPR_PSPB              (0x09F)
>  #define SPR_DPDES             (0x0B0)
>  #define SPR_DAWR0             (0x0B4)
> +#define SPR_DAWR1             (0x0B5)
>  #define SPR_RPR               (0x0BA)
>  #define SPR_CIABR             (0x0BB)
>  #define SPR_DAWRX0            (0x0BC)
> +#define SPR_DAWRX1            (0x0BD)
>  #define SPR_HFSCR             (0x0BE)
>  #define SPR_VRSAVE            (0x100)
>  #define SPR_USPRG0            (0x100)
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..fe3e8a13bb 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -89,6 +89,7 @@ static int cap_ppc_count_cache_flush_assist;
>  static int cap_ppc_nested_kvm_hv;
>  static int cap_large_decr;
>  static int cap_fwnmi;
> +static int cap_dawr1;
>  
>  static uint32_t debug_inst_opcode;
>  
> @@ -138,6 +139,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
>      cap_large_decr = kvmppc_get_dec_bits();
>      cap_fwnmi = kvm_vm_check_extension(s, KVM_CAP_PPC_FWNMI);
> +    cap_dawr1 = kvm_vm_check_extension(s, KVM_CAP_PPC_DAWR1);
>      /*
>       * Note: setting it to false because there is not such capability
>       * in KVM at this moment.
> @@ -2091,6 +2093,16 @@ int kvmppc_set_fwnmi(PowerPCCPU *cpu)
>      return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
>  }
>  
> +bool kvmppc_has_cap_dawr1(void)
> +{
> +    return !!cap_dawr1;
> +}
> +
> +int kvmppc_set_cap_dawr1(int enable)
> +{
> +    return kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_DAWR1, 0, enable);
> +}
> +
>  int kvmppc_smt_threads(void)
>  {
>      return cap_ppc_smt ? cap_ppc_smt : 1;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 989f61ace0..47248fbbfd 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -63,6 +63,8 @@ bool kvmppc_has_cap_htm(void);
>  bool kvmppc_has_cap_mmu_radix(void);
>  bool kvmppc_has_cap_mmu_hash_v3(void);
>  bool kvmppc_has_cap_xive(void);
> +bool kvmppc_has_cap_dawr1(void);
> +int kvmppc_set_cap_dawr1(int enable);
>  int kvmppc_get_cap_safe_cache(void);
>  int kvmppc_get_cap_safe_bounds_check(void);
>  int kvmppc_get_cap_safe_indirect_branch(void);
> @@ -341,6 +343,16 @@ static inline bool kvmppc_has_cap_xive(void)
>      return false;
>  }
>  
> +static inline bool kvmppc_has_cap_dawr1(void)
> +{
> +    return false;
> +}
> +
> +static inline int kvmppc_set_cap_dawr1(int enable)
> +{
> +    abort();
> +}
> +
>  static inline int kvmppc_get_cap_safe_cache(void)
>  {
>      return 0;
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index 879e6df217..8b76e191f1 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -7765,6 +7765,20 @@ static void gen_spr_book3s_207_dbg(CPUPPCState *env)
>                          KVM_REG_PPC_CIABR, 0x00000000);
>  }
>  
> +static void gen_spr_book3s_310_dbg(CPUPPCState *env)
> +{
> +    spr_register_kvm_hv(env, SPR_DAWR1, "DAWR1",
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        &spr_read_generic, &spr_write_generic,
> +                        KVM_REG_PPC_DAWR1, 0x00000000);
> +    spr_register_kvm_hv(env, SPR_DAWRX1, "DAWRX1",
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        &spr_read_generic, &spr_write_generic,
> +                        KVM_REG_PPC_DAWRX1, 0x00000000);
> +}
> +
>  static void gen_spr_970_dbg(CPUPPCState *env)
>  {
>      /* Breakpoints */
> @@ -9142,6 +9156,7 @@ static void init_proc_POWER10(CPUPPCState *env)
>      /* Common Registers */
>      init_proc_book3s_common(env);
>      gen_spr_book3s_207_dbg(env);
> +    gen_spr_book3s_310_dbg(env);
>  
>      /* POWER8 Specific Registers */
>      gen_spr_book3s_ids(env);
>
David Gibson April 19, 2021, 4:53 a.m. UTC | #2
On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
> As per the PAPR, bit 0 of byte 64 in pa-features property indicates
> availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
> find whether kvm supports 2nd DAWR or not. If it's supported, allow
> user to set the pa-feature bit in guest DT using cap-dawr1 machine
> capability. Though, watchpoint on powerpc TCG guest is not supported
> and thus 2nd DAWR is not enabled for TCG mode.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>

So, I'm actually not sure if using an spapr capability is what we want
to do here.  The problem is that presumably the idea is to at some
point make the DAWR1 capability default to on (on POWER10, at least).
But at that point you'll no longer to be able to start TCG guests
without explicitly disabling it.  That's technically correct, since we
don't implement DAWR1 in TCG, but then we also don't implement DAWR0
and we let that slide... which I think is probably going to cause less
irritation on balance.

I'm wondering if we're actually just better off setting the pa feature
just based on the guest CPU model.  TCG will be broken if you try to
use it, but then, it already is.  AFAIK there's no inherent reason we
couldn't implement DAWR support in TCG, it's just never been worth the
trouble.

> ---
>  hw/ppc/spapr.c                  |  7 ++++++-
>  hw/ppc/spapr_caps.c             | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h          |  6 +++++-
>  target/ppc/cpu.h                |  2 ++
>  target/ppc/kvm.c                | 12 ++++++++++++
>  target/ppc/kvm_ppc.h            | 12 ++++++++++++
>  target/ppc/translate_init.c.inc | 15 +++++++++++++++
>  7 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 73a06df3b1..6317fad973 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -238,7 +238,7 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>          0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
>          /* 54: DecFP, 56: DecI, 58: SHA */
>          0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
> -        /* 60: NM atomic, 62: RNG */
> +        /* 60: NM atomic, 62: RNG, 64: DAWR1 (ISA 3.1) */
>          0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
>      };
>      uint8_t *pa_features = NULL;
> @@ -279,6 +279,9 @@ static void spapr_dt_pa_features(SpaprMachineState *spapr,
>           * in pa-features. So hide it from them. */
>          pa_features[40 + 2] &= ~0x80; /* Radix MMU */
>      }
> +    if (spapr_get_cap(spapr, SPAPR_CAP_DAWR1)) {
> +        pa_features[66] |= 0x80;
> +    }
>  
>      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
>  }
> @@ -2003,6 +2006,7 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_cap_ccf_assist,
>          &vmstate_spapr_cap_fwnmi,
>          &vmstate_spapr_fwnmi,
> +        &vmstate_spapr_cap_dawr1,
>          NULL
>      }
>  };
> @@ -4542,6 +4546,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_DAWR1] = SPAPR_CAP_OFF;
>      spapr_caps_add_properties(smc);
>      smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9ea7ddd1e9..98a6b15f29 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -523,6 +523,28 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
>      }
>  }
>  
> +static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
> +                               Error **errp)
> +{
> +    ERRP_GUARD();
> +    if (!val) {
> +        return; /* Disable by default */
> +    }
> +
> +    if (tcg_enabled()) {
> +        error_setg(errp, "DAWR1 not supported in TCG.");
> +        error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
> +    } else if (kvm_enabled()) {
> +        if (!kvmppc_has_cap_dawr1()) {
> +            error_setg(errp, "DAWR1 not supported by KVM.");
> +            error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
> +        } else if (kvmppc_set_cap_dawr1(val) < 0) {
> +            error_setg(errp, "Error enabling cap-dawr1 with KVM.");
> +            error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
> +        }
> +    }
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -631,6 +653,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_fwnmi_apply,
>      },
> +    [SPAPR_CAP_DAWR1] = {
> +        .name = "dawr1",
> +        .description = "Allow 2nd Data Address Watchpoint Register (DAWR1)",
> +        .index = SPAPR_CAP_DAWR1,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_dawr1_apply,
> +    },
>  };
>  
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -771,6 +802,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> +SPAPR_CAP_MIG_STATE(dawr1, SPAPR_CAP_DAWR1);
>  
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5f90bb26d5..51202b7c90 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -74,8 +74,10 @@ typedef enum {
>  #define SPAPR_CAP_CCF_ASSIST            0x09
>  /* Implements PAPR FWNMI option */
>  #define SPAPR_CAP_FWNMI                 0x0A
> +/* DAWR1 */
> +#define SPAPR_CAP_DAWR1                 0x0B
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_DAWR1 + 1)
>  
>  /*
>   * Capability Values
> @@ -366,6 +368,7 @@ struct SpaprMachineState {
>  #define H_SET_MODE_RESOURCE_SET_DAWR0           2
>  #define H_SET_MODE_RESOURCE_ADDR_TRANS_MODE     3
>  #define H_SET_MODE_RESOURCE_LE                  4
> +#define H_SET_MODE_RESOURCE_SET_DAWR1           5
>  
>  /* Flags for H_SET_MODE_RESOURCE_LE */
>  #define H_SET_MODE_ENDIAN_BIG    0
> @@ -921,6 +924,7 @@ extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> +extern const VMStateDescription vmstate_spapr_cap_dawr1;
>  
>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>  {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index cd02d65303..6a60416559 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1460,9 +1460,11 @@ typedef PowerPCCPU ArchCPU;
>  #define SPR_PSPB              (0x09F)
>  #define SPR_DPDES             (0x0B0)
>  #define SPR_DAWR0             (0x0B4)
> +#define SPR_DAWR1             (0x0B5)
>  #define SPR_RPR               (0x0BA)
>  #define SPR_CIABR             (0x0BB)
>  #define SPR_DAWRX0            (0x0BC)
> +#define SPR_DAWRX1            (0x0BD)
>  #define SPR_HFSCR             (0x0BE)
>  #define SPR_VRSAVE            (0x100)
>  #define SPR_USPRG0            (0x100)
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..fe3e8a13bb 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -89,6 +89,7 @@ static int cap_ppc_count_cache_flush_assist;
>  static int cap_ppc_nested_kvm_hv;
>  static int cap_large_decr;
>  static int cap_fwnmi;
> +static int cap_dawr1;
>  
>  static uint32_t debug_inst_opcode;
>  
> @@ -138,6 +139,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
>      cap_large_decr = kvmppc_get_dec_bits();
>      cap_fwnmi = kvm_vm_check_extension(s, KVM_CAP_PPC_FWNMI);
> +    cap_dawr1 = kvm_vm_check_extension(s, KVM_CAP_PPC_DAWR1);
>      /*
>       * Note: setting it to false because there is not such capability
>       * in KVM at this moment.
> @@ -2091,6 +2093,16 @@ int kvmppc_set_fwnmi(PowerPCCPU *cpu)
>      return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
>  }
>  
> +bool kvmppc_has_cap_dawr1(void)
> +{
> +    return !!cap_dawr1;
> +}
> +
> +int kvmppc_set_cap_dawr1(int enable)
> +{
> +    return kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_DAWR1, 0, enable);
> +}
> +
>  int kvmppc_smt_threads(void)
>  {
>      return cap_ppc_smt ? cap_ppc_smt : 1;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 989f61ace0..47248fbbfd 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -63,6 +63,8 @@ bool kvmppc_has_cap_htm(void);
>  bool kvmppc_has_cap_mmu_radix(void);
>  bool kvmppc_has_cap_mmu_hash_v3(void);
>  bool kvmppc_has_cap_xive(void);
> +bool kvmppc_has_cap_dawr1(void);
> +int kvmppc_set_cap_dawr1(int enable);
>  int kvmppc_get_cap_safe_cache(void);
>  int kvmppc_get_cap_safe_bounds_check(void);
>  int kvmppc_get_cap_safe_indirect_branch(void);
> @@ -341,6 +343,16 @@ static inline bool kvmppc_has_cap_xive(void)
>      return false;
>  }
>  
> +static inline bool kvmppc_has_cap_dawr1(void)
> +{
> +    return false;
> +}
> +
> +static inline int kvmppc_set_cap_dawr1(int enable)
> +{
> +    abort();
> +}
> +
>  static inline int kvmppc_get_cap_safe_cache(void)
>  {
>      return 0;
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index 879e6df217..8b76e191f1 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -7765,6 +7765,20 @@ static void gen_spr_book3s_207_dbg(CPUPPCState *env)
>                          KVM_REG_PPC_CIABR, 0x00000000);
>  }
>  
> +static void gen_spr_book3s_310_dbg(CPUPPCState *env)
> +{
> +    spr_register_kvm_hv(env, SPR_DAWR1, "DAWR1",
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        &spr_read_generic, &spr_write_generic,
> +                        KVM_REG_PPC_DAWR1, 0x00000000);
> +    spr_register_kvm_hv(env, SPR_DAWRX1, "DAWRX1",
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        SPR_NOACCESS, SPR_NOACCESS,
> +                        &spr_read_generic, &spr_write_generic,
> +                        KVM_REG_PPC_DAWRX1, 0x00000000);
> +}
> +
>  static void gen_spr_970_dbg(CPUPPCState *env)
>  {
>      /* Breakpoints */
> @@ -9142,6 +9156,7 @@ static void init_proc_POWER10(CPUPPCState *env)
>      /* Common Registers */
>      init_proc_book3s_common(env);
>      gen_spr_book3s_207_dbg(env);
> +    gen_spr_book3s_310_dbg(env);
>  
>      /* POWER8 Specific Registers */
>      gen_spr_book3s_ids(env);
Ravi Bangoria April 21, 2021, 6:20 a.m. UTC | #3
Hi David,

On 4/19/21 10:23 AM, David Gibson wrote:
> On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
>> As per the PAPR, bit 0 of byte 64 in pa-features property indicates
>> availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
>> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
>> find whether kvm supports 2nd DAWR or not. If it's supported, allow
>> user to set the pa-feature bit in guest DT using cap-dawr1 machine
>> capability. Though, watchpoint on powerpc TCG guest is not supported
>> and thus 2nd DAWR is not enabled for TCG mode.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> So, I'm actually not sure if using an spapr capability is what we want
> to do here.  The problem is that presumably the idea is to at some
> point make the DAWR1 capability default to on (on POWER10, at least).
> But at that point you'll no longer to be able to start TCG guests
> without explicitly disabling it.  That's technically correct, since we
> don't implement DAWR1 in TCG, but then we also don't implement DAWR0
> and we let that slide... which I think is probably going to cause less
> irritation on balance.

Ok. Probably something like this is what you want?

Power10 behavior:
   - KVM does not support DAWR1: Boot the guest without DAWR1
     support (No warnings). Error out only if user tries with
     cap-dawr1=on.
   - KVM supports DAWR1: Boot the guest with DAWR1 support, unless
     user specifies cap-dawr1=off.
   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
     DAWR0 (Should be fixed in future while adding PowerPC watch-
     point support in TCG mode)

Power10 predecessor behavior:
   - KVM guest: Boot the guest without DAWR1 support. Error out
     if user tries with cap-dawr1=on.
   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
     DAWR0 (Should be fixed in future while adding PowerPC watch-
     point support in TCG mode)

> I'm wondering if we're actually just better off setting the pa feature
> just based on the guest CPU model.  TCG will be broken if you try to
> use it, but then, it already is.  AFAIK there's no inherent reason we
> couldn't implement DAWR support in TCG, it's just never been worth the
> trouble.

Correct. Probably there is no practical usecase for DAWR in TCG mode.

Thanks,
Ravi
Cédric Le Goater April 21, 2021, 6:31 a.m. UTC | #4
On 4/21/21 8:20 AM, Ravi Bangoria wrote:
> Hi David,
> 
> On 4/19/21 10:23 AM, David Gibson wrote:
>> On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
>>> As per the PAPR, bit 0 of byte 64 in pa-features property indicates
>>> availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
>>> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
>>> find whether kvm supports 2nd DAWR or not. If it's supported, allow
>>> user to set the pa-feature bit in guest DT using cap-dawr1 machine
>>> capability. Though, watchpoint on powerpc TCG guest is not supported
>>> and thus 2nd DAWR is not enabled for TCG mode.
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>> So, I'm actually not sure if using an spapr capability is what we want
>> to do here.  The problem is that presumably the idea is to at some
>> point make the DAWR1 capability default to on (on POWER10, at least).
>> But at that point you'll no longer to be able to start TCG guests
>> without explicitly disabling it.  That's technically correct, since we
>> don't implement DAWR1 in TCG, but then we also don't implement DAWR0
>> and we let that slide... which I think is probably going to cause less
>> irritation on balance.
> 
> Ok. Probably something like this is what you want?
> 
> Power10 behavior:
>   - KVM does not support DAWR1: Boot the guest without DAWR1
>     support (No warnings). Error out only if user tries with
>     cap-dawr1=on.
>   - KVM supports DAWR1: Boot the guest with DAWR1 support, unless
>     user specifies cap-dawr1=off.
>   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
>     DAWR0 (Should be fixed in future while adding PowerPC watch-
>     point support in TCG mode)
> 
> Power10 predecessor behavior:
>   - KVM guest: Boot the guest without DAWR1 support. Error out
>     if user tries with cap-dawr1=on.
>   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
>     DAWR0 (Should be fixed in future while adding PowerPC watch-
>     point support in TCG mode)
> 
>> I'm wondering if we're actually just better off setting the pa feature
>> just based on the guest CPU model.  TCG will be broken if you try to
>> use it, but then, it already is.  AFAIK there's no inherent reason we
>> couldn't implement DAWR support in TCG, it's just never been worth the
>> trouble.
> 
> Correct. Probably there is no practical usecase for DAWR in TCG mode.

What's the expected behavior ? Is it to generate a DSI if we have a DAWR
match ? 

C.
Ravi Bangoria April 21, 2021, 6:54 a.m. UTC | #5
Hi Cedric,

On 4/21/21 12:01 PM, Cédric Le Goater wrote:
> On 4/21/21 8:20 AM, Ravi Bangoria wrote:
>> Hi David,
>>
>> On 4/19/21 10:23 AM, David Gibson wrote:
>>> On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
>>>> As per the PAPR, bit 0 of byte 64 in pa-features property indicates
>>>> availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
>>>> DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
>>>> find whether kvm supports 2nd DAWR or not. If it's supported, allow
>>>> user to set the pa-feature bit in guest DT using cap-dawr1 machine
>>>> capability. Though, watchpoint on powerpc TCG guest is not supported
>>>> and thus 2nd DAWR is not enabled for TCG mode.
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>
>>> So, I'm actually not sure if using an spapr capability is what we want
>>> to do here.  The problem is that presumably the idea is to at some
>>> point make the DAWR1 capability default to on (on POWER10, at least).
>>> But at that point you'll no longer to be able to start TCG guests
>>> without explicitly disabling it.  That's technically correct, since we
>>> don't implement DAWR1 in TCG, but then we also don't implement DAWR0
>>> and we let that slide... which I think is probably going to cause less
>>> irritation on balance.
>>
>> Ok. Probably something like this is what you want?
>>
>> Power10 behavior:
>>    - KVM does not support DAWR1: Boot the guest without DAWR1
>>      support (No warnings). Error out only if user tries with
>>      cap-dawr1=on.
>>    - KVM supports DAWR1: Boot the guest with DAWR1 support, unless
>>      user specifies cap-dawr1=off.
>>    - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
>>      DAWR0 (Should be fixed in future while adding PowerPC watch-
>>      point support in TCG mode)
>>
>> Power10 predecessor behavior:
>>    - KVM guest: Boot the guest without DAWR1 support. Error out
>>      if user tries with cap-dawr1=on.
>>    - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
>>      DAWR0 (Should be fixed in future while adding PowerPC watch-
>>      point support in TCG mode)
>>
>>> I'm wondering if we're actually just better off setting the pa feature
>>> just based on the guest CPU model.  TCG will be broken if you try to
>>> use it, but then, it already is.  AFAIK there's no inherent reason we
>>> couldn't implement DAWR support in TCG, it's just never been worth the
>>> trouble.
>>
>> Correct. Probably there is no practical usecase for DAWR in TCG mode.
> 
> What's the expected behavior ? Is it to generate a DSI if we have a DAWR
> match ?

Yes. DSI is the main thing. But many auxiliary stuff, off the top of my
head:
  - DAR needs to be set. Now, DAR value is set differently on p8 vs p10
    (not sure about p9 because there was hw bug and thus we needed to
    fully disable DAWR on p9).
  - DAWR matching criteria for quadword instruction are different for
    p8/p9 vs p10.
  - P10 supports 512 byte unaligned watchpoints but p8/p9 does not.

Kernel is aware of these differences and thus handles these scenarios,
sometimes as special case. i.e. Qemu will need to mimic the exact hw
behavior for the specific revision of processor.

Thanks,
Ravi
David Gibson April 22, 2021, 1:56 a.m. UTC | #6
On Wed, Apr 21, 2021 at 12:24:22PM +0530, Ravi Bangoria wrote:
> Hi Cedric,
> 
> On 4/21/21 12:01 PM, Cédric Le Goater wrote:
> > On 4/21/21 8:20 AM, Ravi Bangoria wrote:
> > > Hi David,
> > > 
> > > On 4/19/21 10:23 AM, David Gibson wrote:
> > > > On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
> > > > > As per the PAPR, bit 0 of byte 64 in pa-features property indicates
> > > > > availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> > > > > DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
> > > > > find whether kvm supports 2nd DAWR or not. If it's supported, allow
> > > > > user to set the pa-feature bit in guest DT using cap-dawr1 machine
> > > > > capability. Though, watchpoint on powerpc TCG guest is not supported
> > > > > and thus 2nd DAWR is not enabled for TCG mode.
> > > > > 
> > > > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > > 
> > > > So, I'm actually not sure if using an spapr capability is what we want
> > > > to do here.  The problem is that presumably the idea is to at some
> > > > point make the DAWR1 capability default to on (on POWER10, at least).
> > > > But at that point you'll no longer to be able to start TCG guests
> > > > without explicitly disabling it.  That's technically correct, since we
> > > > don't implement DAWR1 in TCG, but then we also don't implement DAWR0
> > > > and we let that slide... which I think is probably going to cause less
> > > > irritation on balance.
> > > 
> > > Ok. Probably something like this is what you want?
> > > 
> > > Power10 behavior:
> > >    - KVM does not support DAWR1: Boot the guest without DAWR1
> > >      support (No warnings). Error out only if user tries with
> > >      cap-dawr1=on.
> > >    - KVM supports DAWR1: Boot the guest with DAWR1 support, unless
> > >      user specifies cap-dawr1=off.
> > >    - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
> > >      DAWR0 (Should be fixed in future while adding PowerPC watch-
> > >      point support in TCG mode)
> > > 
> > > Power10 predecessor behavior:
> > >    - KVM guest: Boot the guest without DAWR1 support. Error out
> > >      if user tries with cap-dawr1=on.
> > >    - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
> > >      DAWR0 (Should be fixed in future while adding PowerPC watch-
> > >      point support in TCG mode)
> > > 
> > > > I'm wondering if we're actually just better off setting the pa feature
> > > > just based on the guest CPU model.  TCG will be broken if you try to
> > > > use it, but then, it already is.  AFAIK there's no inherent reason we
> > > > couldn't implement DAWR support in TCG, it's just never been worth the
> > > > trouble.
> > > 
> > > Correct. Probably there is no practical usecase for DAWR in TCG mode.
> > 
> > What's the expected behavior ? Is it to generate a DSI if we have a DAWR
> > match ?
> 
> Yes. DSI is the main thing. But many auxiliary stuff, off the top of my
> head:
>  - DAR needs to be set. Now, DAR value is set differently on p8 vs p10
>    (not sure about p9 because there was hw bug and thus we needed to
>    fully disable DAWR on p9).
>  - DAWR matching criteria for quadword instruction are different for
>    p8/p9 vs p10.
>  - P10 supports 512 byte unaligned watchpoints but p8/p9 does not.
> 
> Kernel is aware of these differences and thus handles these scenarios,
> sometimes as special case. i.e. Qemu will need to mimic the exact hw
> behavior for the specific revision of processor.

I don't actually know if qemu has TCG watchpoint support on any
hardware.  Presumably it would mean instrumenting all the tcg loads
and stores.
Richard Henderson April 22, 2021, 4:45 a.m. UTC | #7
On 4/21/21 6:56 PM, David Gibson wrote:
> I don't actually know if qemu has TCG watchpoint support on any
> hardware.  Presumably it would mean instrumenting all the tcg loads
> and stores.

We tag the soft tlb for pages that contain watchpoints.

See include/hw/core/cpu.h:
   cpu_watchpoint_insert
   cpu_watchpoint_remove


r~
David Gibson May 5, 2021, 5:50 a.m. UTC | #8
On Wed, Apr 21, 2021 at 11:50:40AM +0530, Ravi Bangoria wrote:
> Hi David,
> 
> On 4/19/21 10:23 AM, David Gibson wrote:
> > On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
> > > As per the PAPR, bit 0 of byte 64 in pa-features property indicates
> > > availability of 2nd DAWR registers. i.e. If this bit is set, 2nd
> > > DAWR is present, otherwise not. Use KVM_CAP_PPC_DAWR1 capability to
> > > find whether kvm supports 2nd DAWR or not. If it's supported, allow
> > > user to set the pa-feature bit in guest DT using cap-dawr1 machine
> > > capability. Though, watchpoint on powerpc TCG guest is not supported
> > > and thus 2nd DAWR is not enabled for TCG mode.
> > > 
> > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > So, I'm actually not sure if using an spapr capability is what we want
> > to do here.  The problem is that presumably the idea is to at some
> > point make the DAWR1 capability default to on (on POWER10, at least).
> > But at that point you'll no longer to be able to start TCG guests
> > without explicitly disabling it.  That's technically correct, since we
> > don't implement DAWR1 in TCG, but then we also don't implement DAWR0
> > and we let that slide... which I think is probably going to cause less
> > irritation on balance.
> 
> Ok. Probably something like this is what you want?
> 
> Power10 behavior:
>   - KVM does not support DAWR1: Boot the guest without DAWR1
>     support (No warnings). Error out only if user tries with
>     cap-dawr1=on.
>   - KVM supports DAWR1: Boot the guest with DAWR1 support, unless
>     user specifies cap-dawr1=off.
>   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
>     DAWR0 (Should be fixed in future while adding PowerPC watch-
>     point support in TCG mode)
> 
> Power10 predecessor behavior:
>   - KVM guest: Boot the guest without DAWR1 support. Error out
>     if user tries with cap-dawr1=on.
>   - TCG guest: Ignore cap-dawr1 i.e. boot as if there is only
>     DAWR0 (Should be fixed in future while adding PowerPC watch-
>     point support in TCG mode)

Sorry I've neglected this thread so long.  I'm afraid the logic above
won't work.  As a general rule we never want to change guest-visible
details of the platform based on KVM capabilities, because it makes a
total mess of migration across clusters.

So, if we'd introduced this along with initial POWER10 support, I
don't think we'd need a capability at all: we just present DAWR1 on
POWER10, don't otherwise.  The fact it doesn't work in TCG we just
treat as a TCG bug we'll probably never get around to fixing, just
like TCG not supporting DAWR0 is a TCG bug right now.

Since we have released versions with POWER10 support, but no DAWR1, in
theory we need a capability so new qemu with old machine types don't
gain guest visible features that the same machine types on older qemus
had.

Except.. there's a loophole we might use to sidestep that.  The
current POWER10 CPU modelled in qemu is a DD1 - which I strongly
suspect will never appear outside of IBM.  I'm pretty sure we want to
replace that with a DD2.

While the modelled CPU is DD1, I think it's pretty reasonable to say
our POWER10 support hasn't yet stabilized, and it would therefore be
ok to simply add DAWR1 on POWER10 unconditionally, as long as we do it
before we switch over to DD2.

> > I'm wondering if we're actually just better off setting the pa feature
> > just based on the guest CPU model.  TCG will be broken if you try to
> > use it, but then, it already is.  AFAIK there's no inherent reason we
> > couldn't implement DAWR support in TCG, it's just never been worth the
> > trouble.
> 
> Correct. Probably there is no practical usecase for DAWR in TCG mode.
> 
> Thanks,
> Ravi
>
Shivaprasad G Bhat July 7, 2023, 8:42 a.m. UTC | #9
Hi David, All,

I am revisiting/reviving this patch.

On 5/5/21 11:20, David Gibson wrote:
> On Wed, Apr 21, 2021 at 11:50:40AM +0530, Ravi Bangoria wrote:
>> Hi David,
>>
>> On 4/19/21 10:23 AM, David Gibson wrote:
>>> On Mon, Apr 12, 2021 at 05:14:33PM +0530, Ravi Bangoria wrote:
>>>
<snip>
> Since we have released versions with POWER10 support, but no DAWR1, in
> theory we need a capability so new qemu with old machine types don't
> gain guest visible features that the same machine types on older qemus
> had.
>
> Except.. there's a loophole we might use to sidestep that.  The
> current POWER10 CPU modelled in qemu is a DD1 - which I strongly
> suspect will never appear outside of IBM.  I'm pretty sure we want to
> replace that with a DD2.
>
> While the modelled CPU is DD1, I think it's pretty reasonable to say
> our POWER10 support hasn't yet stabilized, and it would therefore be
> ok to simply add DAWR1 on POWER10 unconditionally, as long as we do it
> before we switch over to DD2.

As POWER10 DD2 switch over has already happened, the need for

new/separate capability for dawr1 still holds. So, I am keeping it as is.


Posting the next version after rebase.


Thanks,

Shivaprasad

>>> I'm wondering if we're actually just better off setting the pa feature
>>> just based on the guest CPU model.  TCG will be broken if you try to
>>> use it, but then, it already is.  AFAIK there's no inherent reason we
>>> couldn't implement DAWR support in TCG, it's just never been worth the
>>> trouble.
>> Correct. Probably there is no practical usecase for DAWR in TCG mode.
>>
>> Thanks,
>> Ravi
>>
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 73a06df3b1..6317fad973 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -238,7 +238,7 @@  static void spapr_dt_pa_features(SpaprMachineState *spapr,
         0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 48 - 53 */
         /* 54: DecFP, 56: DecI, 58: SHA */
         0x80, 0x00, 0x80, 0x00, 0x80, 0x00, /* 54 - 59 */
-        /* 60: NM atomic, 62: RNG */
+        /* 60: NM atomic, 62: RNG, 64: DAWR1 (ISA 3.1) */
         0x80, 0x00, 0x80, 0x00, 0x00, 0x00, /* 60 - 65 */
     };
     uint8_t *pa_features = NULL;
@@ -279,6 +279,9 @@  static void spapr_dt_pa_features(SpaprMachineState *spapr,
          * in pa-features. So hide it from them. */
         pa_features[40 + 2] &= ~0x80; /* Radix MMU */
     }
+    if (spapr_get_cap(spapr, SPAPR_CAP_DAWR1)) {
+        pa_features[66] |= 0x80;
+    }
 
     _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
 }
@@ -2003,6 +2006,7 @@  static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_cap_ccf_assist,
         &vmstate_spapr_cap_fwnmi,
         &vmstate_spapr_fwnmi,
+        &vmstate_spapr_cap_dawr1,
         NULL
     }
 };
@@ -4542,6 +4546,7 @@  static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
+    smc->default_caps.caps[SPAPR_CAP_DAWR1] = SPAPR_CAP_OFF;
     spapr_caps_add_properties(smc);
     smc->irq = &spapr_irq_dual;
     smc->dr_phb_enabled = true;
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 9ea7ddd1e9..98a6b15f29 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -523,6 +523,28 @@  static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val,
     }
 }
 
+static void cap_dawr1_apply(SpaprMachineState *spapr, uint8_t val,
+                               Error **errp)
+{
+    ERRP_GUARD();
+    if (!val) {
+        return; /* Disable by default */
+    }
+
+    if (tcg_enabled()) {
+        error_setg(errp, "DAWR1 not supported in TCG.");
+        error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
+    } else if (kvm_enabled()) {
+        if (!kvmppc_has_cap_dawr1()) {
+            error_setg(errp, "DAWR1 not supported by KVM.");
+            error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
+        } else if (kvmppc_set_cap_dawr1(val) < 0) {
+            error_setg(errp, "Error enabling cap-dawr1 with KVM.");
+            error_append_hint(errp, "Try appending -machine cap-dawr1=off\n");
+        }
+    }
+}
+
 SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
         .name = "htm",
@@ -631,6 +653,15 @@  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .type = "bool",
         .apply = cap_fwnmi_apply,
     },
+    [SPAPR_CAP_DAWR1] = {
+        .name = "dawr1",
+        .description = "Allow 2nd Data Address Watchpoint Register (DAWR1)",
+        .index = SPAPR_CAP_DAWR1,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
+        .apply = cap_dawr1_apply,
+    },
 };
 
 static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
@@ -771,6 +802,7 @@  SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
 SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
 SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
 SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
+SPAPR_CAP_MIG_STATE(dawr1, SPAPR_CAP_DAWR1);
 
 void spapr_caps_init(SpaprMachineState *spapr)
 {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5f90bb26d5..51202b7c90 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -74,8 +74,10 @@  typedef enum {
 #define SPAPR_CAP_CCF_ASSIST            0x09
 /* Implements PAPR FWNMI option */
 #define SPAPR_CAP_FWNMI                 0x0A
+/* DAWR1 */
+#define SPAPR_CAP_DAWR1                 0x0B
 /* Num Caps */
-#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
+#define SPAPR_CAP_NUM                   (SPAPR_CAP_DAWR1 + 1)
 
 /*
  * Capability Values
@@ -366,6 +368,7 @@  struct SpaprMachineState {
 #define H_SET_MODE_RESOURCE_SET_DAWR0           2
 #define H_SET_MODE_RESOURCE_ADDR_TRANS_MODE     3
 #define H_SET_MODE_RESOURCE_LE                  4
+#define H_SET_MODE_RESOURCE_SET_DAWR1           5
 
 /* Flags for H_SET_MODE_RESOURCE_LE */
 #define H_SET_MODE_ENDIAN_BIG    0
@@ -921,6 +924,7 @@  extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv;
 extern const VMStateDescription vmstate_spapr_cap_large_decr;
 extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
 extern const VMStateDescription vmstate_spapr_cap_fwnmi;
+extern const VMStateDescription vmstate_spapr_cap_dawr1;
 
 static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
 {
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index cd02d65303..6a60416559 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1460,9 +1460,11 @@  typedef PowerPCCPU ArchCPU;
 #define SPR_PSPB              (0x09F)
 #define SPR_DPDES             (0x0B0)
 #define SPR_DAWR0             (0x0B4)
+#define SPR_DAWR1             (0x0B5)
 #define SPR_RPR               (0x0BA)
 #define SPR_CIABR             (0x0BB)
 #define SPR_DAWRX0            (0x0BC)
+#define SPR_DAWRX1            (0x0BD)
 #define SPR_HFSCR             (0x0BE)
 #define SPR_VRSAVE            (0x100)
 #define SPR_USPRG0            (0x100)
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..fe3e8a13bb 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -89,6 +89,7 @@  static int cap_ppc_count_cache_flush_assist;
 static int cap_ppc_nested_kvm_hv;
 static int cap_large_decr;
 static int cap_fwnmi;
+static int cap_dawr1;
 
 static uint32_t debug_inst_opcode;
 
@@ -138,6 +139,7 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_ppc_nested_kvm_hv = kvm_vm_check_extension(s, KVM_CAP_PPC_NESTED_HV);
     cap_large_decr = kvmppc_get_dec_bits();
     cap_fwnmi = kvm_vm_check_extension(s, KVM_CAP_PPC_FWNMI);
+    cap_dawr1 = kvm_vm_check_extension(s, KVM_CAP_PPC_DAWR1);
     /*
      * Note: setting it to false because there is not such capability
      * in KVM at this moment.
@@ -2091,6 +2093,16 @@  int kvmppc_set_fwnmi(PowerPCCPU *cpu)
     return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
 }
 
+bool kvmppc_has_cap_dawr1(void)
+{
+    return !!cap_dawr1;
+}
+
+int kvmppc_set_cap_dawr1(int enable)
+{
+    return kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_DAWR1, 0, enable);
+}
+
 int kvmppc_smt_threads(void)
 {
     return cap_ppc_smt ? cap_ppc_smt : 1;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 989f61ace0..47248fbbfd 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -63,6 +63,8 @@  bool kvmppc_has_cap_htm(void);
 bool kvmppc_has_cap_mmu_radix(void);
 bool kvmppc_has_cap_mmu_hash_v3(void);
 bool kvmppc_has_cap_xive(void);
+bool kvmppc_has_cap_dawr1(void);
+int kvmppc_set_cap_dawr1(int enable);
 int kvmppc_get_cap_safe_cache(void);
 int kvmppc_get_cap_safe_bounds_check(void);
 int kvmppc_get_cap_safe_indirect_branch(void);
@@ -341,6 +343,16 @@  static inline bool kvmppc_has_cap_xive(void)
     return false;
 }
 
+static inline bool kvmppc_has_cap_dawr1(void)
+{
+    return false;
+}
+
+static inline int kvmppc_set_cap_dawr1(int enable)
+{
+    abort();
+}
+
 static inline int kvmppc_get_cap_safe_cache(void)
 {
     return 0;
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index 879e6df217..8b76e191f1 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -7765,6 +7765,20 @@  static void gen_spr_book3s_207_dbg(CPUPPCState *env)
                         KVM_REG_PPC_CIABR, 0x00000000);
 }
 
+static void gen_spr_book3s_310_dbg(CPUPPCState *env)
+{
+    spr_register_kvm_hv(env, SPR_DAWR1, "DAWR1",
+                        SPR_NOACCESS, SPR_NOACCESS,
+                        SPR_NOACCESS, SPR_NOACCESS,
+                        &spr_read_generic, &spr_write_generic,
+                        KVM_REG_PPC_DAWR1, 0x00000000);
+    spr_register_kvm_hv(env, SPR_DAWRX1, "DAWRX1",
+                        SPR_NOACCESS, SPR_NOACCESS,
+                        SPR_NOACCESS, SPR_NOACCESS,
+                        &spr_read_generic, &spr_write_generic,
+                        KVM_REG_PPC_DAWRX1, 0x00000000);
+}
+
 static void gen_spr_970_dbg(CPUPPCState *env)
 {
     /* Breakpoints */
@@ -9142,6 +9156,7 @@  static void init_proc_POWER10(CPUPPCState *env)
     /* Common Registers */
     init_proc_book3s_common(env);
     gen_spr_book3s_207_dbg(env);
+    gen_spr_book3s_310_dbg(env);
 
     /* POWER8 Specific Registers */
     gen_spr_book3s_ids(env);