[RFC PATCH 0/4] use MemTxAttrs to signal cpu index

[RFC PATCH 0/4] use MemTxAttrs to signal cpu index

[Alex Bennée]

Hi,

This ostensibly fixes a bug with gdbstub when accessing the GIC state
(via system registers). It also proposes a pattern for removing
current_cpu/qtest_enabled() hacks in hw/ by passing the cpu index via
MemTxAttrs. For the ARM timer code we also assert that those accesses
do come from a CPU (as opposed to HW DMA attempting to overwrite
stuff).

We do need to audit helpers in target/ which use the address_space API
to read and write data and ensure they properly form their MxTxAttrs
so this works cleanly.

What do people think?

Alex Bennée (4):
  hw: encode accessing CPU index in MemTxAttrs
  qtest: make read/write operation appear to be from CPU
  hw/intc/gic: use MxTxAttrs to divine accessing CPU
  hw/timer: convert mptimer access to attrs to derive cpu index

 include/exec/memattrs.h |  4 +++-
 accel/tcg/cputlb.c      | 22 ++++++++++++++++------
 hw/core/cpu-sysemu.c    | 17 +++++++++++++----
 hw/intc/arm_gic.c       | 39 ++++++++++++++++++++++-----------------
 hw/timer/arm_mptimer.c  | 25 ++++++++++++++-----------
 softmmu/qtest.c         | 31 +++++++++++++++++++------------
 6 files changed, 87 insertions(+), 51 deletions(-)

-- 
2.34.1

[RFC PATCH 1/4] hw: encode accessing CPU index in MemTxAttrs

[Alex Bennée]

We currently have hacks across the hw/ to reference current_cpu to
work out what the current accessing CPU is. This breaks in some cases
including using gdbstub to access HW state. As we have MemTxAttrs to
describe details about the access lets extend it to mention if this is
a CPU access and which one it is.

There are a number of places we need to fix up including:

  CPU helpers directly calling address_space_*() fns
  models in hw/ fishing the data out of current_cpu

I'll start addressing some of these in following patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/memattrs.h |  4 +++-
 accel/tcg/cputlb.c      | 22 ++++++++++++++++------
 hw/core/cpu-sysemu.c    | 17 +++++++++++++----
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..3bccd5d291 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -43,7 +43,9 @@ typedef struct MemTxAttrs {
      * (see MEMTX_ACCESS_ERROR).
      */
     unsigned int memory:1;
-    /* Requester ID (for MSI for example) */
+    /* Requester is CPU (or as CPU, e.g. debug) */
+    unsigned int requester_cpu:1;
+    /* Requester ID (for MSI for example) or cpu_index */
     unsigned int requester_id:16;
     /* Invert endianness for this page */
     unsigned int byte_swap:1;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 8fad2d9b83..68dc7dc646 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1340,8 +1340,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     uint64_t val;
     bool locked = false;
     MemTxResult r;
+    MemTxAttrs attrs = iotlbentry->attrs;
 
-    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+    /* encode the accessing CPU */
+    attrs.requester_cpu = 1;
+    attrs.requester_id = cpu->cpu_index;
+
+    section = iotlb_to_section(cpu, iotlbentry->addr, attrs);
     mr = section->mr;
     mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
     cpu->mem_io_pc = retaddr;
@@ -1353,14 +1358,14 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         qemu_mutex_lock_iothread();
         locked = true;
     }
-    r = memory_region_dispatch_read(mr, mr_offset, &val, op, iotlbentry->attrs);
+    r = memory_region_dispatch_read(mr, mr_offset, &val, op, attrs);
     if (r != MEMTX_OK) {
         hwaddr physaddr = mr_offset +
             section->offset_within_address_space -
             section->offset_within_region;
 
         cpu_transaction_failed(cpu, physaddr, addr, memop_size(op), access_type,
-                               mmu_idx, iotlbentry->attrs, r, retaddr);
+                               mmu_idx, attrs, r, retaddr);
     }
     if (locked) {
         qemu_mutex_unlock_iothread();
@@ -1395,8 +1400,13 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     MemoryRegion *mr;
     bool locked = false;
     MemTxResult r;
+    MemTxAttrs attrs = iotlbentry->attrs;
+
+    /* encode the accessing CPU */
+    attrs.requester_cpu = 1;
+    attrs.requester_id = cpu->cpu_index;
 
-    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+    section = iotlb_to_section(cpu, iotlbentry->addr, attrs);
     mr = section->mr;
     mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
     if (!cpu->can_do_io) {
@@ -1414,14 +1424,14 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         qemu_mutex_lock_iothread();
         locked = true;
     }
-    r = memory_region_dispatch_write(mr, mr_offset, val, op, iotlbentry->attrs);
+    r = memory_region_dispatch_write(mr, mr_offset, val, op, attrs);
     if (r != MEMTX_OK) {
         hwaddr physaddr = mr_offset +
             section->offset_within_address_space -
             section->offset_within_region;
 
         cpu_transaction_failed(cpu, physaddr, addr, memop_size(op),
-                               MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
+                               MMU_DATA_STORE, mmu_idx, attrs, r,
                                retaddr);
     }
     if (locked) {
diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
index 00253f8929..bd7ae983ed 100644
--- a/hw/core/cpu-sysemu.c
+++ b/hw/core/cpu-sysemu.c
@@ -51,13 +51,22 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
                                      MemTxAttrs *attrs)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    MemTxAttrs local = { };
+    hwaddr res;
 
     if (cc->sysemu_ops->get_phys_page_attrs_debug) {
-        return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
+        res = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, &local);
+    } else {
+        /* Fallback for CPUs which don't implement the _attrs_ hook */
+        local = MEMTXATTRS_UNSPECIFIED;
+        res = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
     }
-    /* Fallback for CPUs which don't implement the _attrs_ hook */
-    *attrs = MEMTXATTRS_UNSPECIFIED;
-    return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
+
+    /* debug access is treated as though it came from the CPU */
+    local.requester_cpu = 1;
+    local.requester_id = cpu->cpu_index;
+    *attrs = local;
+    return res;
 }
 
 hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
-- 
2.34.1

Re: [RFC PATCH 1/4] hw: encode accessing CPU index in MemTxAttrs

[Richard Henderson]

On 9/14/22 17:09, Alex Bennée wrote:
> @@ -1340,8 +1340,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>       uint64_t val;
>       bool locked = false;
>       MemTxResult r;
> +    MemTxAttrs attrs = iotlbentry->attrs;
>   
> -    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
> +    /* encode the accessing CPU */
> +    attrs.requester_cpu = 1;
> +    attrs.requester_id = cpu->cpu_index;
> +
> +    section = iotlb_to_section(cpu, iotlbentry->addr, attrs);
>       mr = section->mr;
>       mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
>       cpu->mem_io_pc = retaddr;

At first I was going to suggest that this be done in tlb_set_page_with_attrs, so that it 
could be done once and not duplicate code across read/write.

But then I got to thinking how this ought to interact with MEMTXATTRS_UNSPECIFIED, and now 
I think that we simply have to leave this to the cpu's tlb_fill routine, where it fills in 
(or doesn't) all of the other transaction attributes.


r~

Re: [RFC PATCH 1/4] hw: encode accessing CPU index in MemTxAttrs

[Peter Maydell]

On Wed, 14 Sept 2022 at 17:50, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We currently have hacks across the hw/ to reference current_cpu to
> work out what the current accessing CPU is. This breaks in some cases
> including using gdbstub to access HW state. As we have MemTxAttrs to
> describe details about the access lets extend it to mention if this is
> a CPU access and which one it is.
>
> There are a number of places we need to fix up including:
>
>   CPU helpers directly calling address_space_*() fns
>   models in hw/ fishing the data out of current_cpu
>
> I'll start addressing some of these in following patches.

> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..3bccd5d291 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -43,7 +43,9 @@ typedef struct MemTxAttrs {
>       * (see MEMTX_ACCESS_ERROR).
>       */
>      unsigned int memory:1;
> -    /* Requester ID (for MSI for example) */
> +    /* Requester is CPU (or as CPU, e.g. debug) */
> +    unsigned int requester_cpu:1;
> +    /* Requester ID (for MSI for example) or cpu_index */
>      unsigned int requester_id:16;

This defines effectively two uses for requester_id, with a
bool field differentiating between them, but the patch doesn't
change any of the places that are currently using requester_id
on the assumption that it's the MSI PCI ID to check that
it's not actually a CPU ID instead. (Generally you don't want
the guest CPU to be able to masquerade as a PCI device...)

Also, I think we should look at how this is usually done in
hardware. I'm pretty sure that in AXI, for instance, CPUs
are not special -- every device that can generate memory
transactions can specify an ID (and it's up to the SoC/system
config to assign them sensibly.)

-- PMM

[RFC PATCH 2/4] qtest: make read/write operation appear to be from CPU

[Alex Bennée]

The point of qtest is to simulate how running code might interact with
the system. However because it's not a real system we have places in
the code which especially handle check qtest_enabled() before
referencing current_cpu. Now we can encode these details in the
MemTxAttrs lets do that so we can start removing them.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 softmmu/qtest.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8acef2628..c086bd34b7 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -362,6 +362,13 @@ static void qtest_clock_warp(int64_t dest)
     qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
 }
 
+/*
+ * QTest memory accesses are treated as though they come from the
+ * first (non-existent) CPU. We need to expose this via MemTxAttrs for
+ * those bits of HW which care which core is accessing them.
+ */
+#define MEMTXATTRS_QTEST ((MemTxAttrs) { .requester_cpu = 1 })
+
 static void qtest_process_command(CharBackend *chr, gchar **words)
 {
     const gchar *command;
@@ -525,17 +532,17 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         } else if (words[0][5] == 'w') {
             uint16_t data = value;
             tswap16s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_write(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                 &data, 2);
         } else if (words[0][5] == 'l') {
             uint32_t data = value;
             tswap32s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_write(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                 &data, 4);
         } else if (words[0][5] == 'q') {
             uint64_t data = value;
             tswap64s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_write(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                 &data, 8);
         }
         qtest_send_prefix(chr);
@@ -554,21 +561,21 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
 
         if (words[0][4] == 'b') {
             uint8_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_read(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                &data, 1);
             value = data;
         } else if (words[0][4] == 'w') {
             uint16_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_read(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                &data, 2);
             value = tswap16(data);
         } else if (words[0][4] == 'l') {
             uint32_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_read(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                &data, 4);
             value = tswap32(data);
         } else if (words[0][4] == 'q') {
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_read(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                &value, 8);
             tswap64s(&value);
         }
@@ -589,7 +596,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(len);
 
         data = g_malloc(len);
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+        address_space_read(first_cpu->as, addr, MEMTXATTRS_QTEST, data,
                            len);
 
         enc = g_malloc(2 * len + 1);
@@ -615,7 +622,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(ret == 0);
 
         data = g_malloc(len);
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+        address_space_read(first_cpu->as, addr, MEMTXATTRS_QTEST, data,
                            len);
         b64_data = g_base64_encode(data, len);
         qtest_send_prefix(chr);
@@ -650,7 +657,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                 data[i] = 0;
             }
         }
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+        address_space_write(first_cpu->as, addr, MEMTXATTRS_QTEST, data,
                             len);
         g_free(data);
 
@@ -673,7 +680,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         if (len) {
             data = g_malloc(len);
             memset(data, pattern, len);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_write(first_cpu->as, addr, MEMTXATTRS_QTEST,
                                 data, len);
             g_free(data);
         }
@@ -707,7 +714,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
             out_len = MIN(out_len, len);
         }
 
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
+        address_space_write(first_cpu->as, addr, MEMTXATTRS_QTEST, data,
                             len);
 
         qtest_send_prefix(chr);
-- 
2.34.1

Re: [RFC PATCH 2/4] qtest: make read/write operation appear to be from CPU

[Richard Henderson]

On 9/14/22 17:09, Alex Bennée wrote:
> The point of qtest is to simulate how running code might interact with
> the system. However because it's not a real system we have places in
> the code which especially handle check qtest_enabled() before
> referencing current_cpu. Now we can encode these details in the
> MemTxAttrs lets do that so we can start removing them.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   softmmu/qtest.c | 31 +++++++++++++++++++------------
>   1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
> index f8acef2628..c086bd34b7 100644
> --- a/softmmu/qtest.c
> +++ b/softmmu/qtest.c
> @@ -362,6 +362,13 @@ static void qtest_clock_warp(int64_t dest)
>       qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>   }
>   
> +/*
> + * QTest memory accesses are treated as though they come from the
> + * first (non-existent) CPU. We need to expose this via MemTxAttrs for
> + * those bits of HW which care which core is accessing them.
> + */
> +#define MEMTXATTRS_QTEST ((MemTxAttrs) { .requester_cpu = 1 })

Maybe clearer as { .requester_cpu = true, .requester_id = 0 }?
Otherwise it kinda looks like we're setting the second cpu (index 1).

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [RFC PATCH 2/4] qtest: make read/write operation appear to be from CPU

[Philippe Mathieu-Daudé via]

On 15/9/22 10:08, Richard Henderson wrote:
> On 9/14/22 17:09, Alex Bennée wrote:
>> The point of qtest is to simulate how running code might interact with
>> the system. However because it's not a real system we have places in
>> the code which especially handle check qtest_enabled() before
>> referencing current_cpu. Now we can encode these details in the
>> MemTxAttrs lets do that so we can start removing them.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   softmmu/qtest.c | 31 +++++++++++++++++++------------
>>   1 file changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
>> index f8acef2628..c086bd34b7 100644
>> --- a/softmmu/qtest.c
>> +++ b/softmmu/qtest.c
>> @@ -362,6 +362,13 @@ static void qtest_clock_warp(int64_t dest)
>>       qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>>   }
>> +/*
>> + * QTest memory accesses are treated as though they come from the
>> + * first (non-existent) CPU. We need to expose this via MemTxAttrs for
>> + * those bits of HW which care which core is accessing them.
>> + */
>> +#define MEMTXATTRS_QTEST ((MemTxAttrs) { .requester_cpu = 1 })
> 
> Maybe clearer as { .requester_cpu = true, .requester_id = 0 }?
> Otherwise it kinda looks like we're setting the second cpu (index 1).

We could rename the field as 'requester_is_cpu'.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
> 

[RFC PATCH 3/4] hw/intc/gic: use MxTxAttrs to divine accessing CPU

[Alex Bennée]

Now that MxTxAttrs encodes a CPU we should use that to figure it out.
This solves edge cases like accessing via gdbstub or qtest.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
---
 hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 492b2421ab..7feedac735 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
     0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
 };
 
-static inline int gic_get_current_cpu(GICState *s)
+static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
 {
-    if (!qtest_enabled() && s->num_cpu > 1) {
-        return current_cpu->cpu_index;
-    }
-    return 0;
+    /*
+     * Something other than a CPU accessing the GIC would be a bug as
+     * would a CPU index higher than the GICState expects to be
+     * handling
+     */
+    g_assert(attrs.requester_cpu == 1);
+    g_assert(attrs.requester_id < s->num_cpu);
+
+    return attrs.requester_id;
 }
 
-static inline int gic_get_current_vcpu(GICState *s)
+static inline int gic_get_current_vcpu(GICState *s, MemTxAttrs attrs)
 {
-    return gic_get_current_cpu(s) + GIC_NCPU;
+    return gic_get_current_cpu(s, attrs) + GIC_NCPU;
 }
 
 /* Return true if this GIC config has interrupt groups, which is
@@ -951,7 +956,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
     int cm;
     int mask;
 
-    cpu = gic_get_current_cpu(s);
+    cpu = gic_get_current_cpu(s, attrs);
     cm = 1 << cpu;
     if (offset < 0x100) {
         if (offset == 0) {      /* GICD_CTLR */
@@ -1182,7 +1187,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
     int i;
     int cpu;
 
-    cpu = gic_get_current_cpu(s);
+    cpu = gic_get_current_cpu(s, attrs);
     if (offset < 0x100) {
         if (offset == 0) {
             if (s->security_extn && !attrs.secure) {
@@ -1476,7 +1481,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
         int mask;
         int target_cpu;
 
-        cpu = gic_get_current_cpu(s);
+        cpu = gic_get_current_cpu(s, attrs);
         irq = value & 0xf;
         switch ((value >> 24) & 3) {
         case 0:
@@ -1780,7 +1785,7 @@ static MemTxResult gic_thiscpu_read(void *opaque, hwaddr addr, uint64_t *data,
                                     unsigned size, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-    return gic_cpu_read(s, gic_get_current_cpu(s), addr, data, attrs);
+    return gic_cpu_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
 }
 
 static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
@@ -1788,7 +1793,7 @@ static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
                                      MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-    return gic_cpu_write(s, gic_get_current_cpu(s), addr, value, attrs);
+    return gic_cpu_write(s, gic_get_current_cpu(s, attrs), addr, value, attrs);
 }
 
 /* Wrappers to read/write the GIC CPU interface for a specific CPU.
@@ -1818,7 +1823,7 @@ static MemTxResult gic_thisvcpu_read(void *opaque, hwaddr addr, uint64_t *data,
 {
     GICState *s = (GICState *)opaque;
 
-    return gic_cpu_read(s, gic_get_current_vcpu(s), addr, data, attrs);
+    return gic_cpu_read(s, gic_get_current_vcpu(s, attrs), addr, data, attrs);
 }
 
 static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
@@ -1827,7 +1832,7 @@ static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
 {
     GICState *s = (GICState *)opaque;
 
-    return gic_cpu_write(s, gic_get_current_vcpu(s), addr, value, attrs);
+    return gic_cpu_write(s, gic_get_current_vcpu(s, attrs), addr, value, attrs);
 }
 
 static uint32_t gic_compute_eisr(GICState *s, int cpu, int lr_start)
@@ -1860,7 +1865,7 @@ static uint32_t gic_compute_elrsr(GICState *s, int cpu, int lr_start)
 
 static void gic_vmcr_write(GICState *s, uint32_t value, MemTxAttrs attrs)
 {
-    int vcpu = gic_get_current_vcpu(s);
+    int vcpu = gic_get_current_vcpu(s, attrs);
     uint32_t ctlr;
     uint32_t abpr;
     uint32_t bpr;
@@ -1995,7 +2000,7 @@ static MemTxResult gic_thiscpu_hyp_read(void *opaque, hwaddr addr, uint64_t *dat
 {
     GICState *s = (GICState *)opaque;
 
-    return gic_hyp_read(s, gic_get_current_cpu(s), addr, data, attrs);
+    return gic_hyp_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
 }
 
 static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
@@ -2004,7 +2009,7 @@ static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
 {
     GICState *s = (GICState *)opaque;
 
-    return gic_hyp_write(s, gic_get_current_cpu(s), addr, value, attrs);
+    return gic_hyp_write(s, gic_get_current_cpu(s, attrs), addr, value, attrs);
 }
 
 static MemTxResult gic_do_hyp_read(void *opaque, hwaddr addr, uint64_t *data,
-- 
2.34.1

Re: [RFC PATCH 3/4] hw/intc/gic: use MxTxAttrs to divine accessing CPU

[Richard Henderson]

On 9/14/22 17:09, Alex Bennée wrote:
> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
> This solves edge cases like accessing via gdbstub or qtest.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
> ---
>   hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
>   1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 492b2421ab..7feedac735 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
>       0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>   };
>   
> -static inline int gic_get_current_cpu(GICState *s)
> +static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
>   {
> -    if (!qtest_enabled() && s->num_cpu > 1) {
> -        return current_cpu->cpu_index;
> -    }
> -    return 0;
> +    /*
> +     * Something other than a CPU accessing the GIC would be a bug as
> +     * would a CPU index higher than the GICState expects to be
> +     * handling
> +     */
> +    g_assert(attrs.requester_cpu == 1);

Better without "== 1" -- this field ought to be boolean.

> +    g_assert(attrs.requester_id < s->num_cpu);

Do we still need the qtest_enabled test to short circuit the num_cpu test within the 
assert?  I guess if tests aren't failing then perhaps we don't...

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [RFC PATCH 3/4] hw/intc/gic: use MxTxAttrs to divine accessing CPU

[Philippe Mathieu-Daudé via]

On 15/9/22 10:16, Richard Henderson wrote:
> On 9/14/22 17:09, Alex Bennée wrote:
>> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
>> This solves edge cases like accessing via gdbstub or qtest.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
>> ---
>>   hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
>>   1 file changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 492b2421ab..7feedac735 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
>>       0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 
>> 0x05, 0xb1
>>   };
>> -static inline int gic_get_current_cpu(GICState *s)
>> +static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
>>   {
>> -    if (!qtest_enabled() && s->num_cpu > 1) {
>> -        return current_cpu->cpu_index;
>> -    }
>> -    return 0;
>> +    /*
>> +     * Something other than a CPU accessing the GIC would be a bug as
>> +     * would a CPU index higher than the GICState expects to be
>> +     * handling
>> +     */
>> +    g_assert(attrs.requester_cpu == 1);
> 
> Better without "== 1" -- this field ought to be boolean.

Boolean so far, but this could get more types (such DMA...).
Maybe we could already add an enum definitions, i.e.:

typedef enum MemTxRequesterType {
   MEMTXATTRS_CPU,
   MEMTXATTRS_MSI,
} MemTxRequesterType;

and name the field MemTxAttrs::requester_type.

Re: [RFC PATCH 3/4] hw/intc/gic: use MxTxAttrs to divine accessing CPU

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 15/9/22 10:16, Richard Henderson wrote:
>> On 9/14/22 17:09, Alex Bennée wrote:
>>> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
>>> This solves edge cases like accessing via gdbstub or qtest.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
>>> ---
>>>   hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
>>>   1 file changed, 22 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index 492b2421ab..7feedac735 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
>>>       0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0,
>>> 0x05, 0xb1
>>>   };
>>> -static inline int gic_get_current_cpu(GICState *s)
>>> +static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
>>>   {
>>> -    if (!qtest_enabled() && s->num_cpu > 1) {
>>> -        return current_cpu->cpu_index;
>>> -    }
>>> -    return 0;
>>> +    /*
>>> +     * Something other than a CPU accessing the GIC would be a bug as
>>> +     * would a CPU index higher than the GICState expects to be
>>> +     * handling
>>> +     */
>>> +    g_assert(attrs.requester_cpu == 1);
>> Better without "== 1" -- this field ought to be boolean.
>
> Boolean so far, but this could get more types (such DMA...).
> Maybe we could already add an enum definitions, i.e.:
>
> typedef enum MemTxRequesterType {
>   MEMTXATTRS_CPU,
>   MEMTXATTRS_MSI,
> } MemTxRequesterType;
>
> and name the field MemTxAttrs::requester_type.

I pondered boolean but wasn't sure if that would blow up the size of
MemTxAttrs so went for the bitfield. However I can certainly rename to
requester_is_cpu and make a boolean assertion. I'd hold off adding an
enum until we actually have more than two requester types.

-- 
Alex Bennée

Re: [RFC PATCH 3/4] hw/intc/gic: use MxTxAttrs to divine accessing CPU

[Richard Henderson]

On 9/16/22 17:28, Alex Bennée wrote:
> I pondered boolean but wasn't sure if that would blow up the size of
> MemTxAttrs so went for the bitfield.

You can use bool with a bitfield.  Though Phil's suggestion of .requestor_type has merit. 
  That depends on what comes of Peter's hw modeling suggestion that might integrate the 
types on any given bus.


r~

[RFC PATCH 4/4] hw/timer: convert mptimer access to attrs to derive cpu index

[Alex Bennée]

This removes the hacks to deal with empty current_cpu.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/timer/arm_mptimer.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index cdfca3000b..a7fe6ddc0d 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -41,9 +41,10 @@
  * which is used in both the ARM11MPCore and Cortex-A9MP.
  */
 
-static inline int get_current_cpu(ARMMPTimerState *s)
+static inline int get_current_cpu(ARMMPTimerState *s, MemTxAttrs attrs)
 {
-    int cpu_id = current_cpu ? current_cpu->cpu_index : 0;
+    int cpu_id = attrs.requester_id;
+    g_assert(attrs.requester_cpu == 1);
 
     if (cpu_id >= s->num_cpu) {
         hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
@@ -178,25 +179,27 @@ static void timerblock_write(void *opaque, hwaddr addr,
 /* Wrapper functions to implement the "read timer/watchdog for
  * the current CPU" memory regions.
  */
-static uint64_t arm_thistimer_read(void *opaque, hwaddr addr,
-                                   unsigned size)
+static MemTxResult arm_thistimer_read(void *opaque, hwaddr addr, uint64_t *data,
+                                      unsigned size, MemTxAttrs attrs)
 {
     ARMMPTimerState *s = (ARMMPTimerState *)opaque;
-    int id = get_current_cpu(s);
-    return timerblock_read(&s->timerblock[id], addr, size);
+    int id = get_current_cpu(s, attrs);
+    *data = timerblock_read(&s->timerblock[id], addr, size);
+    return MEMTX_OK;
 }
 
-static void arm_thistimer_write(void *opaque, hwaddr addr,
-                                uint64_t value, unsigned size)
+static MemTxResult arm_thistimer_write(void *opaque, hwaddr addr,
+                                uint64_t value, unsigned size, MemTxAttrs attrs)
 {
     ARMMPTimerState *s = (ARMMPTimerState *)opaque;
-    int id = get_current_cpu(s);
+    int id = get_current_cpu(s, attrs);
     timerblock_write(&s->timerblock[id], addr, value, size);
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps arm_thistimer_ops = {
-    .read = arm_thistimer_read,
-    .write = arm_thistimer_write,
+    .read_with_attrs = arm_thistimer_read,
+    .write_with_attrs = arm_thistimer_write,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 4,
-- 
2.34.1

Re: [RFC PATCH 4/4] hw/timer: convert mptimer access to attrs to derive cpu index

[Richard Henderson]

On 9/14/22 17:09, Alex Bennée wrote:
> This removes the hacks to deal with empty current_cpu.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/timer/arm_mptimer.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index cdfca3000b..a7fe6ddc0d 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -41,9 +41,10 @@
>    * which is used in both the ARM11MPCore and Cortex-A9MP.
>    */
>   
> -static inline int get_current_cpu(ARMMPTimerState *s)
> +static inline int get_current_cpu(ARMMPTimerState *s, MemTxAttrs attrs)
>   {
> -    int cpu_id = current_cpu ? current_cpu->cpu_index : 0;
> +    int cpu_id = attrs.requester_id;
> +    g_assert(attrs.requester_cpu == 1);

Again, drop the "== 1".

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~