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

Commit Message

Alex Bennée Sept. 14, 2022, 4:09 p.m. UTC

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 <[e-mail]>
---
 include/exec/memattrs.h |  4 +++-
 accel/tcg/cputlb.c      | 22 ++++++++++++++++------
 hw/core/cpu-sysemu.c    | 17 +++++++++++++----
 3 files changed, 32 insertions(+), 11 deletions(-)

Comments

Richard Henderson Sept. 15, 2022, 8:05 a.m. UTC | #1

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~

Peter Maydell Sept. 16, 2022, 3:19 p.m. UTC | #2

On Wed, 14 Sept 2022 at 17:50, Alex Bennée <[e-mail]> 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

Patch

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)