[PATCH] target/riscv: Fix PMU node property for virt machine

[PATCH] target/riscv: Fix PMU node property for virt machine

[Yu Chien Peter Lin]

The length of fdt_event_ctr_map[20] will add 5 dummy cells in
"riscv,event-to-mhpmcounters" property, so directly initialize
the array without an explicit size.

This patch also fixes the typo of PMU cache operation result ID
of MISS (0x1) in the comments, and renames event idx 0x10021 to
RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.

Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
---

  $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
  [...]
    riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9 
                                   0x02 0x02 0x7fffc
                                   0x10019 0x10019 0x7fff8
                                   0x1001b 0x1001b 0x7fff8
                                   0x10021 0x10021 0x7fff8
               dummy cells --->    0x00 0x00 0x00 0x00 0x00>;

This won't break the OpenSBI, but will cause it to incorrectly increment
num_hw_events [1] to 6, and DT validation failure in kernel [2].

  $ dt-validate -p Documentation/devicetree/bindings/processed-schema.json virt.dtb
  [...]
  virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}
          From schema: /home/peterlin/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
  virt.dtb: pmu: riscv,event-to-mhpmcounters:6: [0, 0] is too short
  [...]

[1] https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_pmu.c#L255
[2] https://github.com/torvalds/linux/blob/v6.3-rc7/Documentation/devicetree/bindings/perf/riscv%2Cpmu.yaml#L54-L66
---
 target/riscv/cpu.h        |  2 +-
 target/riscv/cpu_helper.c |  2 +-
 target/riscv/pmu.c        | 88 +++++++++++++++++++--------------------
 3 files changed, 45 insertions(+), 47 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..eab518542c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -812,7 +812,7 @@ enum riscv_pmu_event_idx {
     RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
     RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
     RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
-    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
+    RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS = 0x10021,
 };
 
 /* CSR function table */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..5d3e032ec9 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1210,7 +1210,7 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
 
     switch (access_type) {
     case MMU_INST_FETCH:
-        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
+        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS;
         break;
     case MMU_DATA_LOAD:
         pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index b8e56d2b7b..0b21c3fa38 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -35,51 +35,49 @@
  */
 void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
 {
-    uint32_t fdt_event_ctr_map[20] = {};
-    uint32_t cmask;
-
     /* All the programmable counters can map to any event */
-    cmask = MAKE_32BIT_MASK(3, num_ctrs);
-
-   /*
-    * The event encoding is specified in the SBI specification
-    * Event idx is a 20bits wide number encoded as follows:
-    * event_idx[19:16] = type
-    * event_idx[15:0] = code
-    * The code field in cache events are encoded as follows:
-    * event_idx.code[15:3] = cache_id
-    * event_idx.code[2:1] = op_id
-    * event_idx.code[0:0] = result_id
-    */
-
-   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
-   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
-   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
-   fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
-
-   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
-   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
-   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
-   fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
-
-   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
-   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
-   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
-   fdt_event_ctr_map[8] = cpu_to_be32(cmask);
-
-   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
-   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
-   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
-   fdt_event_ctr_map[11] = cpu_to_be32(cmask);
-
-   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
-   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
-   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
-   fdt_event_ctr_map[14] = cpu_to_be32(cmask);
-
-   /* This a OpenSBI specific DT property documented in OpenSBI docs */
-   qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
-                    fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
+    uint32_t cmask = MAKE_32BIT_MASK(3, num_ctrs);
+
+    /*
+     * The event encoding is specified in the SBI specification
+     * Event idx is a 20bits wide number encoded as follows:
+     * event_idx[19:16] = type
+     * event_idx[15:0] = code
+     * The code field in cache events are encoded as follows:
+     * event_idx.code[15:3] = cache_id
+     * event_idx.code[2:1] = op_id
+     * event_idx.code[0:0] = result_id
+     */
+    const uint32_t fdt_event_ctr_map[] = {
+        /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
+        cpu_to_be32(RISCV_PMU_EVENT_HW_CPU_CYCLES),
+        cpu_to_be32(RISCV_PMU_EVENT_HW_CPU_CYCLES),
+        cpu_to_be32(cmask | 1 << 0),
+
+        /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
+        cpu_to_be32(RISCV_PMU_EVENT_HW_INSTRUCTIONS),
+        cpu_to_be32(RISCV_PMU_EVENT_HW_INSTRUCTIONS),
+        cpu_to_be32(cmask | 1 << 2),
+
+        /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x01 type(0x01) */
+        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS),
+        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS),
+        cpu_to_be32(cmask),
+
+        /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x01 type(0x01) */
+        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS),
+        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS),
+        cpu_to_be32(cmask),
+
+        /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x01 type(0x01) */
+        cpu_to_be32(RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS),
+        cpu_to_be32(RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS),
+        cpu_to_be32(cmask),
+    };
+
+    /* This a OpenSBI specific DT property documented in OpenSBI docs */
+    qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
+                     fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
 }
 
 static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
@@ -317,7 +315,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
     case RISCV_PMU_EVENT_HW_INSTRUCTIONS:
     case RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS:
     case RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
-    case RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
+    case RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS:
         break;
     default:
         /* We don't support any raw events right now */
-- 
2.34.1

Re: [PATCH] target/riscv: Fix PMU node property for virt machine

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 8689 bytes --]

On Fri, Apr 21, 2023 at 09:14:37PM +0800, Yu Chien Peter Lin wrote:
> The length of fdt_event_ctr_map[20] will add 5 dummy cells in
> "riscv,event-to-mhpmcounters" property, so directly initialize
> the array without an explicit size.
> 
> This patch also fixes the typo of PMU cache operation result ID
> of MISS (0x1) in the comments, and renames event idx 0x10021 to
> RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.
> 
> Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
> ---
> 
>   $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
>   [...]
>     riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9 
>                                    0x02 0x02 0x7fffc
>                                    0x10019 0x10019 0x7fff8
>                                    0x1001b 0x1001b 0x7fff8
>                                    0x10021 0x10021 0x7fff8
>                dummy cells --->    0x00 0x00 0x00 0x00 0x00>;
> 
> This won't break the OpenSBI, but will cause it to incorrectly increment
> num_hw_events [1] to 6, and DT validation failure in kernel [2].
> 
>   $ dt-validate -p Documentation/devicetree/bindings/processed-schema.json virt.dtb
>   [...]
>   virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}

I would note that this warning here does not go away with this patch ^^
It's still on my todo list, unless you want to fix it!

>           From schema: /home/peterlin/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
>   virt.dtb: pmu: riscv,event-to-mhpmcounters:6: [0, 0] is too short
>   [...]

And with the binding below there is a 3rd warning, but I think it is
incorrect and I submitted a patch for the binding to fix it.

That said, this doesn't seem to have been submitted on top of my naive
s/20/15/ patch that Alistair picked up. Is this intended to replace, or
for another branch? Replace works fine for me, don't have sentimental
feelings for that patch!

I think your approach here was better than my s/20/15/, but seems like a
bit of a fix and a bit of cleanup all-in-one.

Either way, warnings gone so WFM :) :)

Thanks,
Conor.

> [1] https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_pmu.c#L255
> [2] https://github.com/torvalds/linux/blob/v6.3-rc7/Documentation/devicetree/bindings/perf/riscv%2Cpmu.yaml#L54-L66
> ---
>  target/riscv/cpu.h        |  2 +-
>  target/riscv/cpu_helper.c |  2 +-
>  target/riscv/pmu.c        | 88 +++++++++++++++++++--------------------
>  3 files changed, 45 insertions(+), 47 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..eab518542c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -812,7 +812,7 @@ enum riscv_pmu_event_idx {
>      RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
>      RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
>      RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> -    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> +    RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS = 0x10021,
>  };
>  
>  /* CSR function table */
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..5d3e032ec9 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1210,7 +1210,7 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
>  
>      switch (access_type) {
>      case MMU_INST_FETCH:
> -        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> +        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS;
>          break;
>      case MMU_DATA_LOAD:
>          pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index b8e56d2b7b..0b21c3fa38 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -35,51 +35,49 @@
>   */
>  void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
>  {
> -    uint32_t fdt_event_ctr_map[20] = {};
> -    uint32_t cmask;
> -
>      /* All the programmable counters can map to any event */
> -    cmask = MAKE_32BIT_MASK(3, num_ctrs);
> -
> -   /*
> -    * The event encoding is specified in the SBI specification
> -    * Event idx is a 20bits wide number encoded as follows:
> -    * event_idx[19:16] = type
> -    * event_idx[15:0] = code
> -    * The code field in cache events are encoded as follows:
> -    * event_idx.code[15:3] = cache_id
> -    * event_idx.code[2:1] = op_id
> -    * event_idx.code[0:0] = result_id
> -    */
> -
> -   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> -   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
> -   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
> -   fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
> -
> -   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> -   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
> -   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
> -   fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
> -
> -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> -   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
> -   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
> -   fdt_event_ctr_map[8] = cpu_to_be32(cmask);
> -
> -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> -   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
> -   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
> -   fdt_event_ctr_map[11] = cpu_to_be32(cmask);
> -
> -   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> -   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
> -   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
> -   fdt_event_ctr_map[14] = cpu_to_be32(cmask);
> -
> -   /* This a OpenSBI specific DT property documented in OpenSBI docs */
> -   qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
> -                    fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
> +    uint32_t cmask = MAKE_32BIT_MASK(3, num_ctrs);
> +
> +    /*
> +     * The event encoding is specified in the SBI specification
> +     * Event idx is a 20bits wide number encoded as follows:
> +     * event_idx[19:16] = type
> +     * event_idx[15:0] = code
> +     * The code field in cache events are encoded as follows:
> +     * event_idx.code[15:3] = cache_id
> +     * event_idx.code[2:1] = op_id
> +     * event_idx.code[0:0] = result_id
> +     */
> +    const uint32_t fdt_event_ctr_map[] = {
> +        /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> +        cpu_to_be32(RISCV_PMU_EVENT_HW_CPU_CYCLES),
> +        cpu_to_be32(RISCV_PMU_EVENT_HW_CPU_CYCLES),
> +        cpu_to_be32(cmask | 1 << 0),
> +
> +        /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> +        cpu_to_be32(RISCV_PMU_EVENT_HW_INSTRUCTIONS),
> +        cpu_to_be32(RISCV_PMU_EVENT_HW_INSTRUCTIONS),
> +        cpu_to_be32(cmask | 1 << 2),
> +
> +        /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x01 type(0x01) */
> +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS),
> +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS),
> +        cpu_to_be32(cmask),
> +
> +        /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x01 type(0x01) */
> +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS),
> +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS),
> +        cpu_to_be32(cmask),
> +
> +        /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x01 type(0x01) */
> +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS),
> +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS),
> +        cpu_to_be32(cmask),
> +    };
> +
> +    /* This a OpenSBI specific DT property documented in OpenSBI docs */
> +    qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
> +                     fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
>  }
>  
>  static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
> @@ -317,7 +315,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>      case RISCV_PMU_EVENT_HW_INSTRUCTIONS:
>      case RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS:
>      case RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
> -    case RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
> +    case RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS:
>          break;
>      default:
>          /* We don't support any raw events right now */
> -- 
> 2.34.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] target/riscv: Fix PMU node property for virt machine

[Yu-Chien Peter Lin]

Hi Conor,

Thank you for your prompt response.

On Fri, Apr 21, 2023 at 06:59:40PM +0100, Conor Dooley wrote:
> On Fri, Apr 21, 2023 at 09:14:37PM +0800, Yu Chien Peter Lin wrote:
> > The length of fdt_event_ctr_map[20] will add 5 dummy cells in
> > "riscv,event-to-mhpmcounters" property, so directly initialize
> > the array without an explicit size.
> > 
> > This patch also fixes the typo of PMU cache operation result ID
> > of MISS (0x1) in the comments, and renames event idx 0x10021 to
> > RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.
> > 
> > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
> > ---
> > 
> >   $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
> >   [...]
> >     riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9 
> >                                    0x02 0x02 0x7fffc
> >                                    0x10019 0x10019 0x7fff8
> >                                    0x1001b 0x1001b 0x7fff8
> >                                    0x10021 0x10021 0x7fff8
> >                dummy cells --->    0x00 0x00 0x00 0x00 0x00>;
> > 
> > This won't break the OpenSBI, but will cause it to incorrectly increment
> > num_hw_events [1] to 6, and DT validation failure in kernel [2].
> > 
> >   $ dt-validate -p Documentation/devicetree/bindings/processed-schema.json virt.dtb
> >   [...]
> >   virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}
> 
> I would note that this warning here does not go away with this patch ^^
> It's still on my todo list, unless you want to fix it!

I don't fully understand the warning raised by simple-bus.yaml
is it reasonable to move pmu out of soc node?

> 
> >           From schema: /home/peterlin/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
> >   virt.dtb: pmu: riscv,event-to-mhpmcounters:6: [0, 0] is too short
> >   [...]
> 
> And with the binding below there is a 3rd warning, but I think it is
> incorrect and I submitted a patch for the binding to fix it.
> 
> That said, this doesn't seem to have been submitted on top of my naive
> s/20/15/ patch that Alistair picked up. Is this intended to replace, or
> for another branch? Replace works fine for me, don't have sentimental
> feelings for that patch!
> 
> I think your approach here was better than my s/20/15/, but seems like a
> bit of a fix and a bit of cleanup all-in-one.

I just find your patch [1], this solves my problem, thanks!
I will resend this patch based on yours, to simply fix some typos.

[1] https://patchwork.ozlabs.org/project/qemu-devel/patch/20230404173333.35179-1-conor@kernel.org/

Best regards,
Peter Lin

> Either way, warnings gone so WFM :) :)
> 
> Thanks,
> Conor.
> 
> > [1] https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_pmu.c#L255
> > [2] https://github.com/torvalds/linux/blob/v6.3-rc7/Documentation/devicetree/bindings/perf/riscv%2Cpmu.yaml#L54-L66
> > ---
> >  target/riscv/cpu.h        |  2 +-
> >  target/riscv/cpu_helper.c |  2 +-
> >  target/riscv/pmu.c        | 88 +++++++++++++++++++--------------------
> >  3 files changed, 45 insertions(+), 47 deletions(-)
> > 
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 638e47c75a..eab518542c 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -812,7 +812,7 @@ enum riscv_pmu_event_idx {
> >      RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> >      RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> >      RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > -    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > +    RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS = 0x10021,
> >  };
> >  
> >  /* CSR function table */
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index f88c503cf4..5d3e032ec9 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1210,7 +1210,7 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
> >  
> >      switch (access_type) {
> >      case MMU_INST_FETCH:
> > -        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> > +        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS;
> >          break;
> >      case MMU_DATA_LOAD:
> >          pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index b8e56d2b7b..0b21c3fa38 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -35,51 +35,49 @@
> >   */
> >  void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> >  {
> > -    uint32_t fdt_event_ctr_map[20] = {};
> > -    uint32_t cmask;
> > -
> >      /* All the programmable counters can map to any event */
> > -    cmask = MAKE_32BIT_MASK(3, num_ctrs);
> > -
> > -   /*
> > -    * The event encoding is specified in the SBI specification
> > -    * Event idx is a 20bits wide number encoded as follows:
> > -    * event_idx[19:16] = type
> > -    * event_idx[15:0] = code
> > -    * The code field in cache events are encoded as follows:
> > -    * event_idx.code[15:3] = cache_id
> > -    * event_idx.code[2:1] = op_id
> > -    * event_idx.code[0:0] = result_id
> > -    */
> > -
> > -   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> > -   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
> > -   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
> > -   fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
> > -
> > -   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> > -   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
> > -   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
> > -   fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
> > -
> > -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> > -   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
> > -   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
> > -   fdt_event_ctr_map[8] = cpu_to_be32(cmask);
> > -
> > -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> > -   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
> > -   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
> > -   fdt_event_ctr_map[11] = cpu_to_be32(cmask);
> > -
> > -   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> > -   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
> > -   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
> > -   fdt_event_ctr_map[14] = cpu_to_be32(cmask);
> > -
> > -   /* This a OpenSBI specific DT property documented in OpenSBI docs */
> > -   qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
> > -                    fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
> > +    uint32_t cmask = MAKE_32BIT_MASK(3, num_ctrs);
> > +
> > +    /*
> > +     * The event encoding is specified in the SBI specification
> > +     * Event idx is a 20bits wide number encoded as follows:
> > +     * event_idx[19:16] = type
> > +     * event_idx[15:0] = code
> > +     * The code field in cache events are encoded as follows:
> > +     * event_idx.code[15:3] = cache_id
> > +     * event_idx.code[2:1] = op_id
> > +     * event_idx.code[0:0] = result_id
> > +     */
> > +    const uint32_t fdt_event_ctr_map[] = {
> > +        /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> > +        cpu_to_be32(RISCV_PMU_EVENT_HW_CPU_CYCLES),
> > +        cpu_to_be32(RISCV_PMU_EVENT_HW_CPU_CYCLES),
> > +        cpu_to_be32(cmask | 1 << 0),
> > +
> > +        /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> > +        cpu_to_be32(RISCV_PMU_EVENT_HW_INSTRUCTIONS),
> > +        cpu_to_be32(RISCV_PMU_EVENT_HW_INSTRUCTIONS),
> > +        cpu_to_be32(cmask | 1 << 2),
> > +
> > +        /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x01 type(0x01) */
> > +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS),
> > +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS),
> > +        cpu_to_be32(cmask),
> > +
> > +        /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x01 type(0x01) */
> > +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS),
> > +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS),
> > +        cpu_to_be32(cmask),
> > +
> > +        /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x01 type(0x01) */
> > +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS),
> > +        cpu_to_be32(RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS),
> > +        cpu_to_be32(cmask),
> > +    };
> > +
> > +    /* This a OpenSBI specific DT property documented in OpenSBI docs */
> > +    qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
> > +                     fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
> >  }
> >  
> >  static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
> > @@ -317,7 +315,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> >      case RISCV_PMU_EVENT_HW_INSTRUCTIONS:
> >      case RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS:
> >      case RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
> > -    case RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
> > +    case RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS:
> >          break;
> >      default:
> >          /* We don't support any raw events right now */
> > -- 
> > 2.34.1
> > 
> > 

Re: [PATCH] target/riscv: Fix PMU node property for virt machine

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Thu, Apr 27, 2023 at 01:28:18PM +0800, Yu-Chien Peter Lin wrote:
> Hi Conor,
> 
> Thank you for your prompt response.
> 
> On Fri, Apr 21, 2023 at 06:59:40PM +0100, Conor Dooley wrote:
> > On Fri, Apr 21, 2023 at 09:14:37PM +0800, Yu Chien Peter Lin wrote:
> > > The length of fdt_event_ctr_map[20] will add 5 dummy cells in
> > > "riscv,event-to-mhpmcounters" property, so directly initialize
> > > the array without an explicit size.
> > > 
> > > This patch also fixes the typo of PMU cache operation result ID
> > > of MISS (0x1) in the comments, and renames event idx 0x10021 to
> > > RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS.
> > > 
> > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
> > > ---
> > > 
> > >   $ ./build/qemu-system-riscv64 -M virt,dumpdtb=/tmp/virt.dtb -cpu rv64,sscofpmf=on && dtc /tmp/virt.dtb | grep mhpmcounters
> > >   [...]
> > >     riscv,event-to-mhpmcounters = <0x01 0x01 0x7fff9 
> > >                                    0x02 0x02 0x7fffc
> > >                                    0x10019 0x10019 0x7fff8
> > >                                    0x1001b 0x1001b 0x7fff8
> > >                                    0x10021 0x10021 0x7fff8
> > >                dummy cells --->    0x00 0x00 0x00 0x00 0x00>;
> > > 
> > > This won't break the OpenSBI, but will cause it to incorrectly increment
> > > num_hw_events [1] to 6, and DT validation failure in kernel [2].
> > > 
> > >   $ dt-validate -p Documentation/devicetree/bindings/processed-schema.json virt.dtb
> > >   [...]
> > >   virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280], [0, 0, 0], [0, 0]], 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}
> > 
> > I would note that this warning here does not go away with this patch ^^
> > It's still on my todo list, unless you want to fix it!
> 
> I don't fully understand the warning raised by simple-bus.yaml
> is it reasonable to move pmu out of soc node?

If it has no reg properties, it should not be on the soc bus.
I previously made similar changes to other nodes, see commit
ae29379998 ("hw/riscv: virt: fix syscon subnode paths"), as I think it
is indeed the same change here.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]