diff mbox series

[v3,1/2] hw/intc: Fix LoongArch extioi function

Message ID 20220930071045.848674-2-yangxiaojuan@loongson.cn (mailing list archive)
State New, archived
Headers show
Series Add memmap and fix bugs for LoongArch | expand

Commit Message

Xiaojuan Yang Sept. 30, 2022, 7:10 a.m. UTC 
1.When cpu read or write extioi COREISR reg, it should access
the reg belonged to itself, so the index of 's->coreisr' is
current cpu number. Using MemTxAttrs' requester_type and id
to get the cpu index.
2.Remove the unused extioi system memory region and we only
support the extioi iocsr memory region now.

Based-on: <20220927141504.3886314-1-alex.bennee@linaro.org>
Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 hw/intc/loongarch_extioi.c      | 51 +++++++++++++++++++--------------
 hw/intc/trace-events            |  2 +-
 target/loongarch/iocsr_helper.c | 16 +++++------
 3 files changed, 38 insertions(+), 31 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 3, 2022, 12:54 p.m. UTC | #1 
Hi,

On 30/9/22 09:10, Xiaojuan Yang wrote:
> 1.When cpu read or write extioi COREISR reg, it should access
> the reg belonged to itself, so the index of 's->coreisr' is
> current cpu number. Using MemTxAttrs' requester_type and id
> to get the cpu index.
> 2.Remove the unused extioi system memory region and we only
> support the extioi iocsr memory region now.
> 
> Based-on: <20220927141504.3886314-1-alex.bennee@linaro.org>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   hw/intc/loongarch_extioi.c      | 51 +++++++++++++++++++--------------
>   hw/intc/trace-events            |  2 +-
>   target/loongarch/iocsr_helper.c | 16 +++++------
>   3 files changed, 38 insertions(+), 31 deletions(-)

> -static uint64_t extioi_readw(void *opaque, hwaddr addr, unsigned size)
> +static MemTxResult extioi_readw(void *opaque, hwaddr addr, uint64_t *data,
> +                                unsigned size, MemTxAttrs attrs)
>   {
>       LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
>       unsigned long offset = addr & 0xffff;
> -    uint32_t index, cpu, ret = 0;
> +    uint32_t index, cpu;
>   
>       switch (offset) {
>       case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
>           index = (offset - EXTIOI_NODETYPE_START) >> 2;
> -        ret = s->nodetype[index];
> +        *data = s->nodetype[index];
>           break;
>       case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
>           index = (offset - EXTIOI_IPMAP_START) >> 2;
> -        ret = s->ipmap[index];
> +        *data = s->ipmap[index];
>           break;
>       case EXTIOI_ENABLE_START ... EXTIOI_ENABLE_END - 1:
>           index = (offset - EXTIOI_ENABLE_START) >> 2;
> -        ret = s->enable[index];
> +        *data = s->enable[index];
>           break;
>       case EXTIOI_BOUNCE_START ... EXTIOI_BOUNCE_END - 1:
>           index = (offset - EXTIOI_BOUNCE_START) >> 2;
> -        ret = s->bounce[index];
> +        *data = s->bounce[index];
>           break;
>       case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
> -        index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
> -        cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
> -        ret = s->coreisr[cpu][index];
> +        index = (offset - EXTIOI_COREISR_START) >> 2;
> +        /* using attrs to get current cpu index */
> +        if (attrs.requester_type != MTRT_CPU) {

We now miss the trace event. Should we add another one for errors?

> +            return MEMTX_ACCESS_ERROR;
> +        }
> +        cpu = attrs.requester_id;
> +        *data = s->coreisr[cpu][index];
>           break;
>       case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1:
>           index = (offset - EXTIOI_COREMAP_START) >> 2;
> -        ret = s->coremap[index];
> +        *data = s->coremap[index];
>           break;
>       default:
>           break;
>       }
>   
> -    trace_loongarch_extioi_readw(addr, ret);
> -    return ret;
> +    trace_loongarch_extioi_readw(addr, *data);
> +    return MEMTX_OK;
>   }
>   
>   static inline void extioi_enable_irq(LoongArchExtIOI *s, int index,\
> @@ -127,8 +131,9 @@ static inline void extioi_enable_irq(LoongArchExtIOI *s, int index,\
>       }
>   }
>   
> -static void extioi_writew(void *opaque, hwaddr addr,
> -                          uint64_t val, unsigned size)
> +static MemTxResult extioi_writew(void *opaque, hwaddr addr,
> +                          uint64_t val, unsigned size,
> +                          MemTxAttrs attrs)
>   {
>       LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
>       int i, cpu, index, old_data, irq;
> @@ -183,8 +188,12 @@ static void extioi_writew(void *opaque, hwaddr addr,
>           s->bounce[index] = val;
>           break;
>       case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
> -        index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
> -        cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
> +        index = (offset - EXTIOI_COREISR_START) >> 2;
> +        /* using attrs to get current cpu index */
> +        if (attrs.requester_type != MTRT_CPU) {
> +            return MEMTX_ACCESS_ERROR;

Ditto trace event.

> +        }
> +        cpu = attrs.requester_id;
>           old_data = s->coreisr[cpu][index];
>           s->coreisr[cpu][index] = old_data & ~val;
>           /* write 1 to clear interrrupt */
> @@ -231,11 +240,12 @@ static void extioi_writew(void *opaque, hwaddr addr,
>       default:
>           break;
>       }
> +    return MEMTX_OK;
>   }
>   
>   static const MemoryRegionOps extioi_ops = {
> -    .read = extioi_readw,
> -    .write = extioi_writew,
> +    .read_with_attrs = extioi_readw,
> +    .write_with_attrs = extioi_writew,
>       .impl.min_access_size = 4,
>       .impl.max_access_size = 4,
>       .valid.min_access_size = 4,
> @@ -284,9 +294,6 @@ static void loongarch_extioi_instance_init(Object *obj)
>               qdev_init_gpio_out(DEVICE(obj), &s->parent_irq[cpu][pin], 1);
>           }
>       }
> -    memory_region_init_io(&s->extioi_system_mem, OBJECT(s), &extioi_ops,
> -                          s, "extioi_system_mem", 0x900);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->extioi_system_mem);
>   }

I am confused, isn't this used in loongarch_irq_init()?

510         /* extioi iocsr memory region */
511         memory_region_add_subregion(&env->system_iocsr, APIC_BASE,
512 
sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi),
513                                 cpu));

Anyhow this is a separate logical change, so must go in a separate
patch/commit.

Regards,

Phil.
diff mbox series

Patch

diff --git a/hw/intc/loongarch_extioi.c b/hw/intc/loongarch_extioi.c
index 22803969bc..e94bc91565 100644
--- a/hw/intc/loongarch_extioi.c
+++ b/hw/intc/loongarch_extioi.c
@@ -17,7 +17,6 @@ 
 #include "migration/vmstate.h"
 #include "trace.h"
 
-
 static void extioi_update_irq(LoongArchExtIOI *s, int irq, int level)
 {
     int ipnum, cpu, found, irq_index, irq_mask;
@@ -68,44 +67,49 @@  static void extioi_setirq(void *opaque, int irq, int level)
     extioi_update_irq(s, irq, level);
 }
 
-static uint64_t extioi_readw(void *opaque, hwaddr addr, unsigned size)
+static MemTxResult extioi_readw(void *opaque, hwaddr addr, uint64_t *data,
+                                unsigned size, MemTxAttrs attrs)
 {
     LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
     unsigned long offset = addr & 0xffff;
-    uint32_t index, cpu, ret = 0;
+    uint32_t index, cpu;
 
     switch (offset) {
     case EXTIOI_NODETYPE_START ... EXTIOI_NODETYPE_END - 1:
         index = (offset - EXTIOI_NODETYPE_START) >> 2;
-        ret = s->nodetype[index];
+        *data = s->nodetype[index];
         break;
     case EXTIOI_IPMAP_START ... EXTIOI_IPMAP_END - 1:
         index = (offset - EXTIOI_IPMAP_START) >> 2;
-        ret = s->ipmap[index];
+        *data = s->ipmap[index];
         break;
     case EXTIOI_ENABLE_START ... EXTIOI_ENABLE_END - 1:
         index = (offset - EXTIOI_ENABLE_START) >> 2;
-        ret = s->enable[index];
+        *data = s->enable[index];
         break;
     case EXTIOI_BOUNCE_START ... EXTIOI_BOUNCE_END - 1:
         index = (offset - EXTIOI_BOUNCE_START) >> 2;
-        ret = s->bounce[index];
+        *data = s->bounce[index];
         break;
     case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
-        index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
-        cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
-        ret = s->coreisr[cpu][index];
+        index = (offset - EXTIOI_COREISR_START) >> 2;
+        /* using attrs to get current cpu index */
+        if (attrs.requester_type != MTRT_CPU) {
+            return MEMTX_ACCESS_ERROR;
+        }
+        cpu = attrs.requester_id;
+        *data = s->coreisr[cpu][index];
         break;
     case EXTIOI_COREMAP_START ... EXTIOI_COREMAP_END - 1:
         index = (offset - EXTIOI_COREMAP_START) >> 2;
-        ret = s->coremap[index];
+        *data = s->coremap[index];
         break;
     default:
         break;
     }
 
-    trace_loongarch_extioi_readw(addr, ret);
-    return ret;
+    trace_loongarch_extioi_readw(addr, *data);
+    return MEMTX_OK;
 }
 
 static inline void extioi_enable_irq(LoongArchExtIOI *s, int index,\
@@ -127,8 +131,9 @@  static inline void extioi_enable_irq(LoongArchExtIOI *s, int index,\
     }
 }
 
-static void extioi_writew(void *opaque, hwaddr addr,
-                          uint64_t val, unsigned size)
+static MemTxResult extioi_writew(void *opaque, hwaddr addr,
+                          uint64_t val, unsigned size,
+                          MemTxAttrs attrs)
 {
     LoongArchExtIOI *s = LOONGARCH_EXTIOI(opaque);
     int i, cpu, index, old_data, irq;
@@ -183,8 +188,12 @@  static void extioi_writew(void *opaque, hwaddr addr,
         s->bounce[index] = val;
         break;
     case EXTIOI_COREISR_START ... EXTIOI_COREISR_END - 1:
-        index = ((offset - EXTIOI_COREISR_START) & 0x1f) >> 2;
-        cpu = ((offset - EXTIOI_COREISR_START) >> 8) & 0x3;
+        index = (offset - EXTIOI_COREISR_START) >> 2;
+        /* using attrs to get current cpu index */
+        if (attrs.requester_type != MTRT_CPU) {
+            return MEMTX_ACCESS_ERROR;
+        }
+        cpu = attrs.requester_id;
         old_data = s->coreisr[cpu][index];
         s->coreisr[cpu][index] = old_data & ~val;
         /* write 1 to clear interrrupt */
@@ -231,11 +240,12 @@  static void extioi_writew(void *opaque, hwaddr addr,
     default:
         break;
     }
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps extioi_ops = {
-    .read = extioi_readw,
-    .write = extioi_writew,
+    .read_with_attrs = extioi_readw,
+    .write_with_attrs = extioi_writew,
     .impl.min_access_size = 4,
     .impl.max_access_size = 4,
     .valid.min_access_size = 4,
@@ -284,9 +294,6 @@  static void loongarch_extioi_instance_init(Object *obj)
             qdev_init_gpio_out(DEVICE(obj), &s->parent_irq[cpu][pin], 1);
         }
     }
-    memory_region_init_io(&s->extioi_system_mem, OBJECT(s), &extioi_ops,
-                          s, "extioi_system_mem", 0x900);
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->extioi_system_mem);
 }
 
 static void loongarch_extioi_class_init(ObjectClass *klass, void *data)
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 0a90c1cdec..e505d8a2b8 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -306,6 +306,6 @@  loongarch_msi_set_irq(int irq_num) "set msi irq %d"
 
 # loongarch_extioi.c
 loongarch_extioi_setirq(int irq, int level) "set extirq irq %d level %d"
-loongarch_extioi_readw(uint64_t addr, uint32_t val) "addr: 0x%"PRIx64 "val: 0x%x"
+loongarch_extioi_readw(uint64_t addr, uint64_t val) "addr: 0x%"PRIx64 "val: 0x%" PRIx64
 loongarch_extioi_writew(uint64_t addr, uint64_t val) "addr: 0x%"PRIx64 "val: 0x%" PRIx64
 
diff --git a/target/loongarch/iocsr_helper.c b/target/loongarch/iocsr_helper.c
index 0e9c537dc7..d3aac362ff 100644
--- a/target/loongarch/iocsr_helper.c
+++ b/target/loongarch/iocsr_helper.c
@@ -17,51 +17,51 @@ 
 uint64_t helper_iocsrrd_b(CPULoongArchState *env, target_ulong r_addr)
 {
     return address_space_ldub(&env->address_space_iocsr, r_addr,
-                              MEMTXATTRS_UNSPECIFIED, NULL);
+                              MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 uint64_t helper_iocsrrd_h(CPULoongArchState *env, target_ulong r_addr)
 {
     return address_space_lduw(&env->address_space_iocsr, r_addr,
-                              MEMTXATTRS_UNSPECIFIED, NULL);
+                              MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 uint64_t helper_iocsrrd_w(CPULoongArchState *env, target_ulong r_addr)
 {
     return address_space_ldl(&env->address_space_iocsr, r_addr,
-                             MEMTXATTRS_UNSPECIFIED, NULL);
+                             MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 uint64_t helper_iocsrrd_d(CPULoongArchState *env, target_ulong r_addr)
 {
     return address_space_ldq(&env->address_space_iocsr, r_addr,
-                             MEMTXATTRS_UNSPECIFIED, NULL);
+                             MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 void helper_iocsrwr_b(CPULoongArchState *env, target_ulong w_addr,
                       target_ulong val)
 {
     address_space_stb(&env->address_space_iocsr, w_addr,
-                      val, MEMTXATTRS_UNSPECIFIED, NULL);
+                      val, MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 void helper_iocsrwr_h(CPULoongArchState *env, target_ulong w_addr,
                       target_ulong val)
 {
     address_space_stw(&env->address_space_iocsr, w_addr,
-                      val, MEMTXATTRS_UNSPECIFIED, NULL);
+                      val, MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 void helper_iocsrwr_w(CPULoongArchState *env, target_ulong w_addr,
                       target_ulong val)
 {
     address_space_stl(&env->address_space_iocsr, w_addr,
-                      val, MEMTXATTRS_UNSPECIFIED, NULL);
+                      val, MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }
 
 void helper_iocsrwr_d(CPULoongArchState *env, target_ulong w_addr,
                       target_ulong val)
 {
     address_space_stq(&env->address_space_iocsr, w_addr,
-                      val, MEMTXATTRS_UNSPECIFIED, NULL);
+                      val, MEMTXATTRS_CPU(env_cpu(env)), NULL);
 }