qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v3 1/2] hw/intc: Fix LoongArch extioi function
Date: Mon, 3 Oct 2022 14:54:06 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.13.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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]