[PATCH for 8.0 v5 00/20] use MemTxAttrs to avoid current_cpu in hw/

[PATCH for 8.0 v5 00/20] use MemTxAttrs to avoid current_cpu in hw/

[Alex Bennée]

Hi,

This series attempts to improve the modelling of non-CPU writes to
peripherals by expanding the MemTxAttrs to carry more details about
the requester. There are only 3 requester types, the CPU, the PCI bus
and the MACHINE. The last is intended for machine specific buses and
leaves the details of how to decode that information to machine
specific code.

I've extended this beyond just being an Arm only experiment and into
some other machine types. Perhaps the most complicated bit was
tweaking the modelling of the IOAPIC/APIC which gave me the first use
of MTRT_MACHINE (although we don't fully validate the source we do now
correctly drop CPU accesses to the APIC MSI region).

The longer term goal will be to eliminate all the legacy mem
read/write functions and use MemTxAttrs everywhere.

The final patch deprecates the use of current_cpu in hw/ for new code
as a comment. What do people think?

Based-on: 20221111145529.4020801-1-alex.bennee@linaro.org

Alex Bennée (20):
  hw: encode accessing CPU index in MemTxAttrs
  target/arm: ensure TCG IO accesses set appropriate MemTxAttrs
  target/arm: ensure HVF traps set appropriate MemTxAttrs
  target/arm: ensure KVM traps set appropriate MemTxAttrs
  target/arm: ensure m-profile helpers set appropriate 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
  hw/arm: remove current_cpu hack from pxa2xx access
  target/microblaze: initialise MemTxAttrs for CPU access
  target/sparc: initialise MemTxAttrs for CPU access
  target/riscv: initialise MemTxAttrs for CPU access
  target/i386: add explicit initialisation for MexTxAttrs
  hw/audio: explicitly set .requester_type for intel-hda
  hw/i386: update vapic_write to use MemTxAttrs
  include: add MEMTXATTRS_MACHINE helper
  hw/intc: properly model IOAPIC MSI messages
  hw/i386: convert apic access to use MemTxAttrs
  hw/isa: derive CPUState from MemTxAttrs in apm_ioport_writeb
  include/hw: add commentary to current_cpu export

 include/exec/memattrs.h           |  76 +++++++++++---
 include/hw/core/cpu.h             |  14 +++
 include/hw/i386/apic.h            |   2 +-
 include/hw/i386/ioapic_internal.h |   2 +
 include/hw/isa/apm.h              |   2 +-
 target/i386/cpu.h                 |   4 +-
 hw/acpi/ich9.c                    |   1 -
 hw/acpi/piix4.c                   |   2 +-
 hw/arm/pxa2xx.c                   |   2 +-
 hw/audio/intel-hda.c              |   2 +-
 hw/i386/amd_iommu.c               |   6 +-
 hw/i386/intel_iommu.c             |   2 +-
 hw/i386/kvmvapic.c                |  19 ++--
 hw/i386/x86.c                     |  11 +--
 hw/intc/apic.c                    |  62 ++++++++----
 hw/intc/arm_gic.c                 | 159 +++++++++++++++++++-----------
 hw/intc/ioapic.c                  |  35 +++++--
 hw/isa/apm.c                      |  21 +++-
 hw/isa/lpc_ich9.c                 |   5 +-
 hw/misc/tz-mpc.c                  |   2 +-
 hw/misc/tz-msc.c                  |   6 +-
 hw/pci/pci.c                      |   4 +-
 hw/timer/arm_mptimer.c            |  49 ++++++---
 softmmu/qtest.c                   |  26 ++---
 target/arm/hvf/hvf.c              |   4 +-
 target/arm/kvm.c                  |   9 +-
 target/arm/m_helper.c             |  12 +--
 target/arm/ptw.c                  |   3 +-
 target/arm/tlb_helper.c           |   2 +-
 target/i386/hax/hax-all.c         |   2 +-
 target/i386/nvmm/nvmm-all.c       |   2 +-
 target/i386/sev.c                 |   2 +-
 target/i386/whpx/whpx-all.c       |   2 +-
 target/microblaze/helper.c        |   4 +-
 target/riscv/cpu_helper.c         |   2 +-
 target/sparc/mmu_helper.c         |   6 +-
 36 files changed, 370 insertions(+), 194 deletions(-)

-- 
2.34.1

[PATCH v5 01/20] 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 so CPU accesses can
be explicitly marked.

To achieve this we create a new requester_type which indicates to
consumers how requester_id it to be consumed. We absorb the existing
unspecified:1 bitfield into this type and also document a potential
machine specific encoding which will be useful to (currently)
out-of-tree extensions.

Places that checked to see if things where unspecified now instead
check the source if what they expected.

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
  hypervisors offloading device emulation to QEMU

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

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use separate field cpu_index
  - bool for requester_is_cpu
v3
  - switch to enum MemTxRequesterType
  - move helper #define to patch
  - revert to overloading requester_id
  - mention hypervisors in commit message
  - drop cputlb tweaks, they will move to target specific code
v4
  - merge unspecified:1 into MTRT_UNSPECIFIED
  - document a MTRT_MACHINE for more complex encoding
  - ensure existing users of requester_id set MTRT_PCI
  - ensure existing consumers of requester_id check type is MTRT_PCI
  - have MEMTXATTRS_CPU take CPUState * directly
v5
  - re-order so MTRT_UNSPECIFIED is zero
  - fix up comments referring to the difference between empty and unspecified:1
  - kernel-doc annotations for typedefs
  - don't impose source type tz-msc during transformation
  - re-order bitfields so requester_type/id at top
  - add helper for MEMTXATTRS_PCI
---
 include/exec/memattrs.h | 68 ++++++++++++++++++++++++++++++++---------
 hw/i386/amd_iommu.c     |  6 ++--
 hw/i386/intel_iommu.c   |  2 +-
 hw/misc/tz-mpc.c        |  2 +-
 hw/misc/tz-msc.c        |  6 ++--
 hw/pci/pci.c            |  4 +--
 6 files changed, 60 insertions(+), 28 deletions(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..8359fc448b 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -14,7 +14,32 @@
 #ifndef MEMATTRS_H
 #define MEMATTRS_H
 
-/* Every memory transaction has associated with it a set of
+/**
+ * typedef MemTxRequesterType - source of memory transaction
+ *
+ * Every memory transaction comes from a specific place which defines
+ * how requester_id should be handled if at all.
+ *
+ * UNSPECIFIED: the default for otherwise undefined MemTxAttrs
+ * CPU: requester_id is the global cpu_index
+ *      This needs further processing if you need to work out which
+ *      socket or complex it comes from
+ * PCI: indicates the requester_id is a PCI id
+ * MACHINE: indicates a machine specific encoding
+ *          This will require further processing to decode into its
+ *          constituent parts.
+ */
+typedef enum MemTxRequesterType {
+    MTRT_UNSPECIFIED = 0,
+    MTRT_CPU,
+    MTRT_PCI,
+    MTRT_MACHINE
+} MemTxRequesterType;
+
+/**
+ * typedef MemTxAttrs - attributes of a memory transaction
+ *
+ * Every memory transaction has associated with it a set of
  * attributes. Some of these are generic (such as the ID of
  * the bus master); some are specific to a particular kind of
  * bus (such as the ARM Secure/NonSecure bit). We define them
@@ -23,13 +48,12 @@
  * different semantics.
  */
 typedef struct MemTxAttrs {
-    /* Bus masters which don't specify any attributes will get this
-     * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
-     * distinguish "all attributes deliberately clear" from
-     * "didn't specify" if necessary.
-     */
-    unsigned int unspecified:1;
-    /* ARM/AMBA: TrustZone Secure access
+    /* Requester type (e.g. CPU or PCI MSI) */
+    MemTxRequesterType requester_type:2;
+    /* Requester ID */
+    unsigned int requester_id:16;
+    /*
+     * ARM/AMBA: TrustZone Secure access
      * x86: System Management Mode access
      */
     unsigned int secure:1;
@@ -43,8 +67,6 @@ typedef struct MemTxAttrs {
      * (see MEMTX_ACCESS_ERROR).
      */
     unsigned int memory:1;
-    /* Requester ID (for MSI for example) */
-    unsigned int requester_id:16;
     /* Invert endianness for this page */
     unsigned int byte_swap:1;
     /*
@@ -59,12 +81,28 @@ typedef struct MemTxAttrs {
     unsigned int target_tlb_bit2 : 1;
 } MemTxAttrs;
 
-/* Bus masters which don't specify any attributes will get this,
- * which has all attribute bits clear except the topmost one
- * (so that we can distinguish "all attributes deliberately clear"
- * from "didn't specify" if necessary).
+/*
+ * Bus masters which don't specify any attributes will get this which
+ * indicates none of the attributes can be used.
+ */
+#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) \
+                                { .requester_type = MTRT_UNSPECIFIED })
+
+/*
+ * Helper for setting a basic CPU sourced transaction, it expects a
+ * CPUState *
+ */
+#define MEMTXATTRS_CPU(cs) ((MemTxAttrs) \
+                            {.requester_type = MTRT_CPU, \
+                             .requester_id = cs->cpu_index})
+
+/*
+ * Helper for setting a basic PCI sourced transaction, it expects a
+ * PCIDevice *
  */
-#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
+#define MEMTXATTRS_PCI(dev) ((MemTxAttrs) \
+                             {.requester_type = MTRT_PCI,   \
+                             .requester_id = pci_requester_id(dev)})
 
 /* New-style MMIO accessors can indicate that the transaction failed.
  * A zero (MEMTX_OK) response means success; anything else is a failure
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 725f69095b..284359c16e 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -153,9 +153,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
 static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
     MSIMessage msg = {};
-    MemTxAttrs attrs = {
-        .requester_id = pci_requester_id(&s->pci.dev)
-    };
+    MemTxAttrs attrs = MEMTXATTRS_PCI(&s->pci.dev);
 
     if (msi_enabled(&s->pci.dev)) {
         msg = msi_get_message(&s->pci.dev, 0);
@@ -1356,7 +1354,7 @@ static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
 
     trace_amdvi_mem_ir_write_req(addr, value, size);
 
-    if (!attrs.unspecified) {
+    if (attrs.requester_type == MTRT_PCI) {
         /* We have explicit Source ID */
         sid = attrs.requester_id;
     }
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a08ee85edf..12752413eb 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3517,7 +3517,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
     from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
     from.data = (uint32_t) value;
 
-    if (!attrs.unspecified) {
+    if (attrs.requester_type == MTRT_PCI) {
         /* We have explicit Source ID */
         sid = attrs.requester_id;
     }
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index 30481e1c90..4beb5daa1a 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -461,7 +461,7 @@ static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
      * All the real during-emulation transactions from the CPU will
      * specify attributes.
      */
-    return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
+    return ((attrs.requester_type == MTRT_UNSPECIFIED) || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
 }
 
 static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
diff --git a/hw/misc/tz-msc.c b/hw/misc/tz-msc.c
index acbe94400b..e93bfc7083 100644
--- a/hw/misc/tz-msc.c
+++ b/hw/misc/tz-msc.c
@@ -137,11 +137,9 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
         return MEMTX_OK;
     case MSCAllowSecure:
         attrs.secure = 1;
-        attrs.unspecified = 0;
         break;
     case MSCAllowNonSecure:
         attrs.secure = 0;
-        attrs.unspecified = 0;
         break;
     }
 
@@ -179,11 +177,11 @@ static MemTxResult tz_msc_write(void *opaque, hwaddr addr, uint64_t val,
         return MEMTX_OK;
     case MSCAllowSecure:
         attrs.secure = 1;
-        attrs.unspecified = 0;
+        attrs.requester_type = MTRT_CPU;
         break;
     case MSCAllowNonSecure:
         attrs.secure = 0;
-        attrs.unspecified = 0;
+        attrs.requester_type = MTRT_CPU;
         break;
     }
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2f450f6a72..1d0d8d866f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -319,9 +319,7 @@ void pci_device_deassert_intx(PCIDevice *dev)
 
 static void pci_msi_trigger(PCIDevice *dev, MSIMessage msg)
 {
-    MemTxAttrs attrs = {};
-
-    attrs.requester_id = pci_requester_id(dev);
+    MemTxAttrs attrs = MEMTXATTRS_PCI(dev);
     address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
                          attrs, NULL);
 }
-- 
2.34.1

[PATCH v5 02/20] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs

[Alex Bennée]

Both arm_cpu_tlb_fill (for normal IO) and
arm_cpu_get_phys_page_attrs_debug (for debug access) come through
get_phys_addr which is setting the other memory attributes for the
transaction. As these are all by definition CPU accesses we can also
set the requested_type/index as appropriate.

We also have to handle where the attributes are totally reset if we
call into get_phys_addr_twostage.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - reword commit summary
v5
  - fix for new *result ABI
  - use MEMTXATTRS_CPU to fill in the initial values
  - also reset attrs in get_phys_addr_twostage
---
 target/arm/ptw.c        | 3 ++-
 target/arm/tlb_helper.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 3745ac9723..4b6683f90d 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2634,6 +2634,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
     s1_lgpgsz = result->f.lg_page_size;
     cacheattrs1 = result->cacheattrs;
     memset(result, 0, sizeof(*result));
+    result->f.attrs = MEMTXATTRS_CPU(env_cpu(env));
 
     ret = get_phys_addr_lpae(env, ptw, ipa, access_type, is_el0, result, fi);
     fi->s2addr = ipa;
@@ -2872,7 +2873,7 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
         .in_secure = arm_is_secure(env),
         .in_debug = true,
     };
-    GetPhysAddrResult res = {};
+    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
     ARMMMUFaultInfo fi = {};
     bool ret;
 
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 0f4f4fc809..5960269421 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -208,7 +208,7 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                       bool probe, uintptr_t retaddr)
 {
     ARMCPU *cpu = ARM_CPU(cs);
-    GetPhysAddrResult res = {};
+    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
     ARMMMUFaultInfo local_fi, *fi;
     int ret;
 
-- 
2.34.1

[PATCH v5 03/20] target/arm: ensure HVF traps set appropriate MemTxAttrs

[Alex Bennée]

As most HVF devices are done purely in software we need to make sure
we properly encode the source CPU in MemTxAttrs. This will allow the
device emulations to use those attributes rather than relying on
current_cpu (although current_cpu will still be correct in this case).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Mads Ynddal <mads@ynddal.dk>
Acked-by: Alexander Graf <agraf@csgraf.de>

---
v2
  - update MEMTXATTRS macro
v5
  - more tags
---
 target/arm/hvf/hvf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 060aa0ccf4..d81fbbb2df 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1233,11 +1233,11 @@ int hvf_vcpu_exec(CPUState *cpu)
             val = hvf_get_reg(cpu, srt);
             address_space_write(&address_space_memory,
                                 hvf_exit->exception.physical_address,
-                                MEMTXATTRS_UNSPECIFIED, &val, len);
+                                MEMTXATTRS_CPU(cpu), &val, len);
         } else {
             address_space_read(&address_space_memory,
                                hvf_exit->exception.physical_address,
-                               MEMTXATTRS_UNSPECIFIED, &val, len);
+                               MEMTXATTRS_CPU(cpu), &val, len);
             hvf_set_reg(cpu, srt, val);
         }
 
-- 
2.34.1

[PATCH v5 04/20] target/arm: ensure KVM traps set appropriate MemTxAttrs

[Alex Bennée]

Although most KVM users will use the in-kernel GIC emulation it is
perfectly possible not to. In this case we need to ensure the
MemTxAttrs are correctly populated so the GIC can divine the source
CPU of the operation.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - new for v3
v5
  - tags
  - use MEMTXATTRS_PCI
---
 target/arm/kvm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index f022c644d2..bb4cdbfbd5 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -803,13 +803,14 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
 {
     ARMCPU *cpu;
     uint32_t switched_level;
+    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
 
     if (kvm_irqchip_in_kernel()) {
         /*
          * We only need to sync timer states with user-space interrupt
          * controllers, so return early and save cycles if we don't.
          */
-        return MEMTXATTRS_UNSPECIFIED;
+        return attrs;
     }
 
     cpu = ARM_CPU(cs);
@@ -850,7 +851,7 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
         qemu_mutex_unlock_iothread();
     }
 
-    return MEMTXATTRS_UNSPECIFIED;
+    return attrs;
 }
 
 void kvm_arm_vm_state_change(void *opaque, bool running, RunState state)
@@ -1005,6 +1006,7 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
     hwaddr xlat, len, doorbell_gpa;
     MemoryRegionSection mrs;
     MemoryRegion *mr;
+    MemTxAttrs attrs = MEMTXATTRS_PCI(dev);
 
     if (as == &address_space_memory) {
         return 0;
@@ -1014,8 +1016,7 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
 
     RCU_READ_LOCK_GUARD();
 
-    mr = address_space_translate(as, address, &xlat, &len, true,
-                                 MEMTXATTRS_UNSPECIFIED);
+    mr = address_space_translate(as, address, &xlat, &len, true, attrs);
 
     if (!mr) {
         return 1;
-- 
2.34.1

[PATCH v5 04/20] target/arm: ensure KVM traps set appropriate MemTxAttrs

[Alex Bennée]

Although most KVM users will use the in-kernel GIC emulation it is
perfectly possible not to. In this case we need to ensure the
MemTxAttrs are correctly populated so the GIC can divine the source
CPU of the operation.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v3
  - new for v3
v5
  - tags
  - use MEMTXATTRS_PCI
---
 target/arm/kvm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index f022c644d2..bb4cdbfbd5 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -803,13 +803,14 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
 {
     ARMCPU *cpu;
     uint32_t switched_level;
+    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
 
     if (kvm_irqchip_in_kernel()) {
         /*
          * We only need to sync timer states with user-space interrupt
          * controllers, so return early and save cycles if we don't.
          */
-        return MEMTXATTRS_UNSPECIFIED;
+        return attrs;
     }
 
     cpu = ARM_CPU(cs);
@@ -850,7 +851,7 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
         qemu_mutex_unlock_iothread();
     }
 
-    return MEMTXATTRS_UNSPECIFIED;
+    return attrs;
 }
 
 void kvm_arm_vm_state_change(void *opaque, bool running, RunState state)
@@ -1005,6 +1006,7 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
     hwaddr xlat, len, doorbell_gpa;
     MemoryRegionSection mrs;
     MemoryRegion *mr;
+    MemTxAttrs attrs = MEMTXATTRS_PCI(dev);
 
     if (as == &address_space_memory) {
         return 0;
@@ -1014,8 +1016,7 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
 
     RCU_READ_LOCK_GUARD();
 
-    mr = address_space_translate(as, address, &xlat, &len, true,
-                                 MEMTXATTRS_UNSPECIFIED);
+    mr = address_space_translate(as, address, &xlat, &len, true, attrs);
 
     if (!mr) {
         return 1;
-- 
2.34.1

[PATCH v5 05/20] target/arm: ensure m-profile helpers set appropriate MemTxAttrs

[Alex Bennée]

There are a number of helpers for M-profile that deal with CPU
initiated access to the vector and stack areas. While it is unlikely
these coincided with memory mapped IO devices it is not inconceivable.
Embedded targets tend to attract all sorts of interesting code and for
completeness we should tag the transaction appropriately.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v5
  - rebase fixes for refactoring
---
 target/arm/m_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 355cd4d60a..2fb1ef95cd 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -184,7 +184,7 @@ static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     MemTxResult txres;
-    GetPhysAddrResult res = {};
+    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
     ARMMMUFaultInfo fi = {};
     bool secure = mmu_idx & ARM_MMU_IDX_M_S;
     int exc;
@@ -272,7 +272,7 @@ static bool v7m_stack_read(ARMCPU *cpu, uint32_t *dest, uint32_t addr,
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     MemTxResult txres;
-    GetPhysAddrResult res = {};
+    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
     ARMMMUFaultInfo fi = {};
     bool secure = mmu_idx & ARM_MMU_IDX_M_S;
     int exc;
@@ -665,7 +665,7 @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,
     MemTxResult result;
     uint32_t addr = env->v7m.vecbase[targets_secure] + exc * 4;
     uint32_t vector_entry;
-    MemTxAttrs attrs = {};
+    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
     ARMMMUIdx mmu_idx;
     bool exc_secure;
 
@@ -1999,7 +1999,7 @@ static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx, bool secure,
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     V8M_SAttributes sattrs = {};
-    GetPhysAddrResult res = {};
+    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
     ARMMMUFaultInfo fi = {};
     MemTxResult txres;
 
@@ -2047,7 +2047,7 @@ static bool v7m_read_sg_stack_word(ARMCPU *cpu, ARMMMUIdx mmu_idx,
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     MemTxResult txres;
-    GetPhysAddrResult res = {};
+    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
     ARMMMUFaultInfo fi = {};
     uint32_t value;
 
@@ -2805,7 +2805,7 @@ uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t addr, uint32_t op)
      * inspecting the other MPU state.
      */
     if (arm_current_el(env) != 0 || alt) {
-        GetPhysAddrResult res = {};
+        GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(env_cpu(env)) };
         ARMMMUFaultInfo fi = {};
 
         /* We can ignore the return value as prot is always set */
-- 
2.34.1

[PATCH v5 06/20] 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.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use a common macro instead of specific MEMTXATTRS_QTEST
v3
  - macro moved to earlier patch
---
 softmmu/qtest.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index d3e0ab4eda..5e9ac234ce 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -520,22 +520,22 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
 
         if (words[0][5] == 'b') {
             uint8_t data = value;
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_write(first_cpu->as, addr, MEMTXATTRS_CPU(first_cpu),
                                 &data, 1);
         } 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_CPU(first_cpu),
                                 &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_CPU(first_cpu),
                                 &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_CPU(first_cpu),
                                 &data, 8);
         }
         qtest_send_prefix(chr);
@@ -554,21 +554,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_CPU(first_cpu),
                                &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_CPU(first_cpu),
                                &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_CPU(first_cpu),
                                &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_CPU(first_cpu),
                                &value, 8);
             tswap64s(&value);
         }
@@ -589,7 +589,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_CPU(first_cpu), data,
                            len);
 
         enc = g_malloc(2 * len + 1);
@@ -615,7 +615,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_CPU(first_cpu), data,
                            len);
         b64_data = g_base64_encode(data, len);
         qtest_send_prefix(chr);
@@ -650,7 +650,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_CPU(first_cpu), data,
                             len);
         g_free(data);
 
@@ -673,7 +673,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_CPU(first_cpu),
                                 data, len);
             g_free(data);
         }
@@ -707,7 +707,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_CPU(first_cpu), data,
                             len);
 
         qtest_send_prefix(chr);
-- 
2.34.1

[PATCH v5 07/20] 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. As we
should only be processing accesses from CPU cores we can push the CPU
extraction logic out to the main access functions. If the access does
not come from a CPU we log it and fail the transaction with
MEMTX_ACCESS_ERROR.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124

---
v2
  - update for new field
  - bool asserts
v3
  - fail non-CPU transactions
v5
  - split gic_valid_cpu from gic_get_current_cpu and use this
  - fix dud return false from gic_valid_cpu()
---
 hw/intc/arm_gic.c | 159 +++++++++++++++++++++++++++++-----------------
 1 file changed, 102 insertions(+), 57 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 65b1ef7151..62f36b247f 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -56,17 +56,38 @@ 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)
+
+/*
+ * The GIC should only be accessed by the CPU so if it is not we
+ * should fail the transaction (it would either be a bug in how we've
+ * wired stuff up, a limitation of the translator or the guest doing
+ * something weird like programming a DMA master to write to the MMIO
+ * region).
+ *
+ * Note the cpu_index is global and we currently don't have any models
+ * with multiple SoC's with different CPUs. However if we did we would
+ * need to transform the cpu_index into the socket core.
+ */
+
+static bool gic_valid_cpu(MemTxAttrs attrs)
 {
-    if (!qtest_enabled() && s->num_cpu > 1) {
-        return current_cpu->cpu_index;
+    if (attrs.requester_type != MTRT_CPU) {
+        qemu_log_mask(LOG_UNIMP | LOG_GUEST_ERROR,
+                      "%s: saw non-CPU transaction", __func__);
+        return false;
     }
-    return 0;
+    return true;
+}
+
+static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
+{
+    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
@@ -945,17 +966,14 @@ static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
  * Although this is named a byte read we don't always return bytes and
  * rely on the calling function oring bits together.
  */
-static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
+static uint32_t gic_dist_readb(GICState *s, int cpu, hwaddr offset, MemTxAttrs attrs)
 {
-    GICState *s = (GICState *)opaque;
     uint32_t res;
     int irq;
     int i;
-    int cpu;
     int cm;
     int mask;
 
-    cpu = gic_get_current_cpu(s);
     cm = 1 << cpu;
     if (offset < 0x100) {
         if (offset < 0xc) {
@@ -1168,19 +1186,27 @@ bad_reg:
 static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
                                  unsigned size, MemTxAttrs attrs)
 {
+    GICState *s = (GICState *)opaque;
+    int cpu;
+
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    cpu = gic_get_current_cpu(s, attrs);
+
     switch (size) {
     case 1:
-        *data = gic_dist_readb(opaque, offset, attrs);
+        *data = gic_dist_readb(s, cpu, offset, attrs);
         break;
     case 2:
-        *data = gic_dist_readb(opaque, offset, attrs);
-        *data |= gic_dist_readb(opaque, offset + 1, attrs) << 8;
+        *data = gic_dist_readb(s, cpu, offset, attrs);
+        *data |= gic_dist_readb(s, cpu, offset + 1, attrs) << 8;
         break;
     case 4:
-        *data = gic_dist_readb(opaque, offset, attrs);
-        *data |= gic_dist_readb(opaque, offset + 1, attrs) << 8;
-        *data |= gic_dist_readb(opaque, offset + 2, attrs) << 16;
-        *data |= gic_dist_readb(opaque, offset + 3, attrs) << 24;
+        *data = gic_dist_readb(s, cpu, offset, attrs);
+        *data |= gic_dist_readb(s, cpu, offset + 1, attrs) << 8;
+        *data |= gic_dist_readb(s, cpu, offset + 2, attrs) << 16;
+        *data |= gic_dist_readb(s, cpu, offset + 3, attrs) << 24;
         break;
     default:
         return MEMTX_ERROR;
@@ -1190,15 +1216,12 @@ static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
     return MEMTX_OK;
 }
 
-static void gic_dist_writeb(void *opaque, hwaddr offset,
+static void gic_dist_writeb(GICState *s, int cpu, hwaddr offset,
                             uint32_t value, MemTxAttrs attrs)
 {
-    GICState *s = (GICState *)opaque;
     int irq;
     int i;
-    int cpu;
 
-    cpu = gic_get_current_cpu(s);
     if (offset < 0x100) {
         if (offset == 0) {
             if (s->security_extn && !attrs.secure) {
@@ -1475,24 +1498,21 @@ bad_reg:
                   "gic_dist_writeb: Bad offset %x\n", (int)offset);
 }
 
-static void gic_dist_writew(void *opaque, hwaddr offset,
+static void gic_dist_writew(GICState *s, int cpu, hwaddr offset,
                             uint32_t value, MemTxAttrs attrs)
 {
-    gic_dist_writeb(opaque, offset, value & 0xff, attrs);
-    gic_dist_writeb(opaque, offset + 1, value >> 8, attrs);
+    gic_dist_writeb(s, cpu, offset, value & 0xff, attrs);
+    gic_dist_writeb(s, cpu, offset + 1, value >> 8, attrs);
 }
 
-static void gic_dist_writel(void *opaque, hwaddr offset,
+static void gic_dist_writel(GICState *s, int cpu, hwaddr offset,
                             uint32_t value, MemTxAttrs attrs)
 {
-    GICState *s = (GICState *)opaque;
     if (offset == 0xf00) {
-        int cpu;
         int irq;
         int mask;
         int target_cpu;
 
-        cpu = gic_get_current_cpu(s);
         irq = value & 0xf;
         switch ((value >> 24) & 3) {
         case 0:
@@ -1519,24 +1539,32 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
         gic_update(s);
         return;
     }
-    gic_dist_writew(opaque, offset, value & 0xffff, attrs);
-    gic_dist_writew(opaque, offset + 2, value >> 16, attrs);
+    gic_dist_writew(s, cpu, offset, value & 0xffff, attrs);
+    gic_dist_writew(s, cpu, offset + 2, value >> 16, attrs);
 }
 
 static MemTxResult gic_dist_write(void *opaque, hwaddr offset, uint64_t data,
                                   unsigned size, MemTxAttrs attrs)
 {
+    GICState *s = (GICState *)opaque;
+    int cpu;
+
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    cpu = gic_get_current_cpu(s, attrs);
+
     trace_gic_dist_write(offset, size, data);
 
     switch (size) {
     case 1:
-        gic_dist_writeb(opaque, offset, data, attrs);
+        gic_dist_writeb(s, cpu, offset, data, attrs);
         return MEMTX_OK;
     case 2:
-        gic_dist_writew(opaque, offset, data, attrs);
+        gic_dist_writew(s, cpu, offset, data, attrs);
         return MEMTX_OK;
     case 4:
-        gic_dist_writel(opaque, offset, data, attrs);
+        gic_dist_writel(s, cpu, offset, data, attrs);
         return MEMTX_OK;
     default:
         return MEMTX_ERROR;
@@ -1796,7 +1824,10 @@ 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);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    return gic_cpu_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
 }
 
 static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
@@ -1804,7 +1835,10 @@ 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);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    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.
@@ -1833,8 +1867,10 @@ static MemTxResult gic_thisvcpu_read(void *opaque, hwaddr addr, uint64_t *data,
                                     unsigned size, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-
-    return gic_cpu_read(s, gic_get_current_vcpu(s), addr, data, attrs);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    return gic_cpu_read(s, gic_get_current_vcpu(s, attrs), addr, data, attrs);
 }
 
 static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
@@ -1842,8 +1878,10 @@ static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
                                      MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-
-    return gic_cpu_write(s, gic_get_current_vcpu(s), addr, value, attrs);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    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)
@@ -1874,9 +1912,8 @@ static uint32_t gic_compute_elrsr(GICState *s, int cpu, int lr_start)
     return ret;
 }
 
-static void gic_vmcr_write(GICState *s, uint32_t value, MemTxAttrs attrs)
+static void gic_vmcr_write(GICState *s, int vcpu, uint32_t value, MemTxAttrs attrs)
 {
-    int vcpu = gic_get_current_vcpu(s);
     uint32_t ctlr;
     uint32_t abpr;
     uint32_t bpr;
@@ -1893,11 +1930,10 @@ static void gic_vmcr_write(GICState *s, uint32_t value, MemTxAttrs attrs)
     gic_set_priority_mask(s, vcpu, prio_mask, attrs);
 }
 
-static MemTxResult gic_hyp_read(void *opaque, int cpu, hwaddr addr,
+static MemTxResult gic_hyp_read(GICState *s, int cpu, hwaddr addr,
                                 uint64_t *data, MemTxAttrs attrs)
 {
-    GICState *s = ARM_GIC(opaque);
-    int vcpu = cpu + GIC_NCPU;
+    int vcpu = gic_get_current_vcpu(s, attrs);
 
     switch (addr) {
     case A_GICH_HCR: /* Hypervisor Control */
@@ -1961,11 +1997,10 @@ static MemTxResult gic_hyp_read(void *opaque, int cpu, hwaddr addr,
     return MEMTX_OK;
 }
 
-static MemTxResult gic_hyp_write(void *opaque, int cpu, hwaddr addr,
+static MemTxResult gic_hyp_write(GICState *s, int cpu, hwaddr addr,
                                  uint64_t value, MemTxAttrs attrs)
 {
-    GICState *s = ARM_GIC(opaque);
-    int vcpu = cpu + GIC_NCPU;
+    int vcpu = gic_get_current_vcpu(s, attrs);
 
     trace_gic_hyp_write(addr, value);
 
@@ -1975,12 +2010,13 @@ static MemTxResult gic_hyp_write(void *opaque, int cpu, hwaddr addr,
         break;
 
     case A_GICH_VMCR: /* Virtual Machine Control */
-        gic_vmcr_write(s, value, attrs);
+        gic_vmcr_write(s, vcpu, value, attrs);
         break;
 
     case A_GICH_APR: /* Active Priorities */
         s->h_apr[cpu] = value;
-        s->running_priority[vcpu] = gic_get_prio_from_apr_bits(s, vcpu);
+        s->running_priority[vcpu] =
+            gic_get_prio_from_apr_bits(s, vcpu);
         break;
 
     case A_GICH_LR0 ... A_GICH_LR63: /* List Registers */
@@ -2007,20 +2043,24 @@ static MemTxResult gic_hyp_write(void *opaque, int cpu, hwaddr addr,
 }
 
 static MemTxResult gic_thiscpu_hyp_read(void *opaque, hwaddr addr, uint64_t *data,
-                                    unsigned size, MemTxAttrs attrs)
+                                        unsigned size, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-
-    return gic_hyp_read(s, gic_get_current_cpu(s), addr, data, attrs);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    return gic_hyp_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
 }
 
 static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
-                                     uint64_t value, unsigned size,
-                                     MemTxAttrs attrs)
+                                         uint64_t value, unsigned size,
+                                         MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-
-    return gic_hyp_write(s, gic_get_current_cpu(s), addr, value, attrs);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
+    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,
@@ -2029,6 +2069,9 @@ static MemTxResult gic_do_hyp_read(void *opaque, hwaddr addr, uint64_t *data,
     GICState **backref = (GICState **)opaque;
     GICState *s = *backref;
     int id = (backref - s->backref);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
 
     return gic_hyp_read(s, id, addr, data, attrs);
 }
@@ -2040,9 +2083,11 @@ static MemTxResult gic_do_hyp_write(void *opaque, hwaddr addr,
     GICState **backref = (GICState **)opaque;
     GICState *s = *backref;
     int id = (backref - s->backref);
+    if (!gic_valid_cpu(attrs)) {
+        return MEMTX_ACCESS_ERROR;
+    }
 
     return gic_hyp_write(s, id + GIC_NCPU, addr, value, attrs);
-
 }
 
 static const MemoryRegionOps gic_ops[2] = {
-- 
2.34.1

[PATCH v5 08/20] hw/timer: convert mptimer access to attrs to derive cpu index

[Alex Bennée]

This removes the hacks to deal with empty current_cpu.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - update for new fields
  - bool asserts
v3
  - properly fail memory transactions from non-CPU sources
---
 hw/timer/arm_mptimer.c | 49 +++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index cdfca3000b..4618779ade 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -28,6 +28,7 @@
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 #include "hw/core/cpu.h"
 
 #define PTIMER_POLICY                       \
@@ -41,15 +42,23 @@
  * which is used in both the ARM11MPCore and Cortex-A9MP.
  */
 
-static inline int get_current_cpu(ARMMPTimerState *s)
+static bool is_from_cpu(MemTxAttrs attrs)
 {
-    int cpu_id = current_cpu ? current_cpu->cpu_index : 0;
+    if (attrs.requester_type != MTRT_CPU) {
+        qemu_log_mask(LOG_UNIMP | LOG_GUEST_ERROR,
+                      "%s: saw non-CPU transaction", __func__);
+        return false;
+    }
+    return true;
+}
 
+static int get_current_cpu(ARMMPTimerState *s, MemTxAttrs attrs)
+{
+    int cpu_id = attrs.requester_id;
     if (cpu_id >= s->num_cpu) {
         hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
                  s->num_cpu, cpu_id);
     }
-
     return cpu_id;
 }
 
@@ -178,25 +187,35 @@ 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);
+    if (is_from_cpu(attrs)) {
+        ARMMPTimerState *s = (ARMMPTimerState *)opaque;
+        int id = get_current_cpu(s, attrs);
+        *data = timerblock_read(&s->timerblock[id], addr, size);
+        return MEMTX_OK;
+    } else {
+        return MEMTX_ACCESS_ERROR;
+    }
 }
 
-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);
-    timerblock_write(&s->timerblock[id], addr, value, size);
+    if (is_from_cpu(attrs)) {
+        ARMMPTimerState *s = (ARMMPTimerState *)opaque;
+        int id = get_current_cpu(s, attrs);
+        timerblock_write(&s->timerblock[id], addr, value, size);
+        return MEMTX_OK;
+    } else {
+        return MEMTX_ACCESS_ERROR;
+    }
 }
 
 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

[PATCH v5 09/20] hw/arm: remove current_cpu hack from pxa2xx access

[Alex Bennée]

We can derive the correct CPU from CPUARMState so lets not rely on
current_cpu.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/arm/pxa2xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 93dda83d7a..065392a8bc 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -319,7 +319,7 @@ static void pxa2xx_pwrmode_write(CPUARMState *env, const ARMCPRegInfo *ri,
 #endif
 
         /* Suspend */
-        cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
+        cpu_interrupt(env_cpu(env), CPU_INTERRUPT_HALT);
 
         goto message;
 
-- 
2.34.1

[PATCH v5 10/20] target/microblaze: initialise MemTxAttrs for CPU access

[Alex Bennée]

Both of these functions deal with CPU based access (as is evidenced by
the secure check straight after). Use the new MEMTXATTRS_CPU
constructor to ensure the correct CPU id is filled in should it ever
be needed by any devices later.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/microblaze/helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/microblaze/helper.c b/target/microblaze/helper.c
index 98bdb82de8..655be3b320 100644
--- a/target/microblaze/helper.c
+++ b/target/microblaze/helper.c
@@ -44,7 +44,7 @@ bool mb_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     MicroBlazeMMULookup lu;
     unsigned int hit;
     int prot;
-    MemTxAttrs attrs = {};
+    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
 
     attrs.secure = mb_cpu_access_is_secure(cpu, access_type);
 
@@ -235,7 +235,7 @@ hwaddr mb_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
     unsigned int hit;
 
     /* Caller doesn't initialize */
-    *attrs = (MemTxAttrs) {};
+    *attrs = MEMTXATTRS_CPU(cs);
     attrs->secure = mb_cpu_access_is_secure(cpu, MMU_DATA_LOAD);
 
     if (mmu_idx != MMU_NOMMU_IDX) {
-- 
2.34.1

[PATCH v5 11/20] target/sparc: initialise MemTxAttrs for CPU access

[Alex Bennée]

Both of the TLB fill functions and the cpu_sparc_get_phys_page deal
with CPU based access. Use the new MEMTXATTRS_CPU constructor to
ensure the correct CPU id is filled in should it ever be needed by any
devices later.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/sparc/mmu_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 919448a494..eeb52b5ee6 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -212,7 +212,7 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     target_ulong vaddr;
     target_ulong page_size;
     int error_code = 0, prot, access_index;
-    MemTxAttrs attrs = {};
+    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
 
     /*
      * TODO: If we ever need tlb_vaddr_to_host for this target,
@@ -771,7 +771,7 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     target_ulong vaddr;
     hwaddr paddr;
     target_ulong page_size;
-    MemTxAttrs attrs = {};
+    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
     int error_code = 0, prot, access_index;
 
     address &= TARGET_PAGE_MASK;
@@ -890,7 +890,7 @@ static int cpu_sparc_get_phys_page(CPUSPARCState *env, hwaddr *phys,
 {
     target_ulong page_size;
     int prot, access_index;
-    MemTxAttrs attrs = {};
+    MemTxAttrs attrs = MEMTXATTRS_CPU(env_cpu(env));
 
     return get_physical_address(env, phys, &prot, &access_index, &attrs, addr,
                                 rw, mmu_idx, &page_size);
-- 
2.34.1

[PATCH v5 12/20] target/riscv: initialise MemTxAttrs for CPU access

[Alex Bennée]

get_physical_address works in the CPU context. Use the new
MEMTXATTRS_CPU constructor to ensure the correct CPU id is filled in
should it ever be needed by any devices later.

Currently the tlb_fill function isn't using the set with attributes
function so IO accesses from the softmmu slow-path will not be tagged
as coming from the CPU.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/riscv/cpu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 278d163803..e661f9e68a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -761,7 +761,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
      * correct, but the value visible to the exception handler
      * (riscv_cpu_do_interrupt) is correct */
     MemTxResult res;
-    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+    MemTxAttrs attrs = MEMTXATTRS_CPU(env_cpu(env));
     int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK;
     bool use_background = false;
     hwaddr ppn;
-- 
2.34.1

[PATCH v5 13/20] target/i386: add explicit initialisation for MexTxAttrs

[Alex Bennée]

Where appropriate initialise with MEMTXATTRS_CPU otherwise use
MEMTXATTRS_UNSPECIFIED instead of the null initialiser.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/i386/cpu.h           | 4 +++-
 target/i386/hax/hax-all.c   | 2 +-
 target/i386/nvmm/nvmm-all.c | 2 +-
 target/i386/sev.c           | 2 +-
 target/i386/whpx/whpx-all.c | 2 +-
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d4bc19577a..04ab96b076 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2246,7 +2246,9 @@ static inline uint32_t cpu_compute_eflags(CPUX86State *env)
 
 static inline MemTxAttrs cpu_get_mem_attrs(CPUX86State *env)
 {
-    return ((MemTxAttrs) { .secure = (env->hflags & HF_SMM_MASK) != 0 });
+    MemTxAttrs attrs = MEMTXATTRS_CPU(env_cpu(env));
+    attrs.secure = (env->hflags & HF_SMM_MASK) != 0;
+    return attrs;
 }
 
 static inline int32_t x86_get_a20_mask(CPUX86State *env)
diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c
index b185ee8de4..337090e16f 100644
--- a/target/i386/hax/hax-all.c
+++ b/target/i386/hax/hax-all.c
@@ -385,7 +385,7 @@ static int hax_handle_io(CPUArchState *env, uint32_t df, uint16_t port,
 {
     uint8_t *ptr;
     int i;
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
 
     if (!df) {
         ptr = (uint8_t *) buffer;
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index b75738ee9c..cb0720a6fa 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -502,7 +502,7 @@ nvmm_vcpu_post_run(CPUState *cpu, struct nvmm_vcpu_exit *exit)
 static void
 nvmm_io_callback(struct nvmm_io *io)
 {
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     int ret;
 
     ret = address_space_rw(&address_space_io, io->port, attrs, io->data,
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 32f7dbac4e..292cbcdd92 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1274,7 +1274,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     uint8_t *hashp;
     size_t hash_len = HASH_SIZE;
     hwaddr mapped_len = sizeof(*padded_ht);
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     bool ret = true;
 
     /*
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index e738d83e81..42846144dd 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -791,7 +791,7 @@ static HRESULT CALLBACK whpx_emu_ioport_callback(
     void *ctx,
     WHV_EMULATOR_IO_ACCESS_INFO *IoAccess)
 {
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     address_space_rw(&address_space_io, IoAccess->Port, attrs,
                      &IoAccess->Data, IoAccess->AccessSize,
                      IoAccess->Direction);
-- 
2.34.1

[PATCH v5 13/20] target/i386: add explicit initialisation for MexTxAttrs

[Alex Bennée]

Where appropriate initialise with MEMTXATTRS_CPU otherwise use
MEMTXATTRS_UNSPECIFIED instead of the null initialiser.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/i386/cpu.h           | 4 +++-
 target/i386/hax/hax-all.c   | 2 +-
 target/i386/nvmm/nvmm-all.c | 2 +-
 target/i386/sev.c           | 2 +-
 target/i386/whpx/whpx-all.c | 2 +-
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d4bc19577a..04ab96b076 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2246,7 +2246,9 @@ static inline uint32_t cpu_compute_eflags(CPUX86State *env)
 
 static inline MemTxAttrs cpu_get_mem_attrs(CPUX86State *env)
 {
-    return ((MemTxAttrs) { .secure = (env->hflags & HF_SMM_MASK) != 0 });
+    MemTxAttrs attrs = MEMTXATTRS_CPU(env_cpu(env));
+    attrs.secure = (env->hflags & HF_SMM_MASK) != 0;
+    return attrs;
 }
 
 static inline int32_t x86_get_a20_mask(CPUX86State *env)
diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c
index b185ee8de4..337090e16f 100644
--- a/target/i386/hax/hax-all.c
+++ b/target/i386/hax/hax-all.c
@@ -385,7 +385,7 @@ static int hax_handle_io(CPUArchState *env, uint32_t df, uint16_t port,
 {
     uint8_t *ptr;
     int i;
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
 
     if (!df) {
         ptr = (uint8_t *) buffer;
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index b75738ee9c..cb0720a6fa 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -502,7 +502,7 @@ nvmm_vcpu_post_run(CPUState *cpu, struct nvmm_vcpu_exit *exit)
 static void
 nvmm_io_callback(struct nvmm_io *io)
 {
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     int ret;
 
     ret = address_space_rw(&address_space_io, io->port, attrs, io->data,
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 32f7dbac4e..292cbcdd92 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1274,7 +1274,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     uint8_t *hashp;
     size_t hash_len = HASH_SIZE;
     hwaddr mapped_len = sizeof(*padded_ht);
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     bool ret = true;
 
     /*
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index e738d83e81..42846144dd 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -791,7 +791,7 @@ static HRESULT CALLBACK whpx_emu_ioport_callback(
     void *ctx,
     WHV_EMULATOR_IO_ACCESS_INFO *IoAccess)
 {
-    MemTxAttrs attrs = { 0 };
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
     address_space_rw(&address_space_io, IoAccess->Port, attrs,
                      &IoAccess->Data, IoAccess->AccessSize,
                      IoAccess->Direction);
-- 
2.34.1

[PATCH v5 14/20] hw/audio: explicitly set .requester_type for intel-hda

[Alex Bennée]

This is simulating a bus master writing data back into system memory.
Mark it as such.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/audio/intel-hda.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index f38117057b..95c28b315c 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -345,7 +345,7 @@ static void intel_hda_corb_run(IntelHDAState *d)
 
 static void intel_hda_response(HDACodecDevice *dev, bool solicited, uint32_t response)
 {
-    const MemTxAttrs attrs = { .memory = true };
+    const MemTxAttrs attrs = { .requester_type = MTRT_PCI, .memory = true };
     HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
     IntelHDAState *d = container_of(bus, IntelHDAState, codecs);
     hwaddr addr;
-- 
2.34.1

[PATCH v5 15/20] hw/i386: update vapic_write to use MemTxAttrs

[Alex Bennée]

This allows us to drop the current_cpu hack and properly model an
invalid access to the vapic.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/i386/kvmvapic.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 43f8a8f679..a76ed07199 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -635,20 +635,21 @@ static int vapic_prepare(VAPICROMState *s)
     return 0;
 }
 
-static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
-                        unsigned int size)
+static MemTxResult vapic_write(void *opaque, hwaddr addr, uint64_t data,
+                               unsigned int size, MemTxAttrs attrs)
 {
     VAPICROMState *s = opaque;
+    CPUState *cs;
     X86CPU *cpu;
     CPUX86State *env;
     hwaddr rom_paddr;
 
-    if (!current_cpu) {
-        return;
+    if (attrs.requester_type != MTRT_CPU) {
+        return MEMTX_ACCESS_ERROR;
     }
-
-    cpu_synchronize_state(current_cpu);
-    cpu = X86_CPU(current_cpu);
+    cs = qemu_get_cpu(attrs.requester_id);
+    cpu_synchronize_state(cs);
+    cpu = X86_CPU(cs);
     env = &cpu->env;
 
     /*
@@ -708,6 +709,8 @@ static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
         }
         break;
     }
+
+    return MEMTX_OK;
 }
 
 static uint64_t vapic_read(void *opaque, hwaddr addr, unsigned size)
@@ -716,7 +719,7 @@ static uint64_t vapic_read(void *opaque, hwaddr addr, unsigned size)
 }
 
 static const MemoryRegionOps vapic_ops = {
-    .write = vapic_write,
+    .write_with_attrs = vapic_write,
     .read = vapic_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
-- 
2.34.1

[PATCH v5 16/20] include: add MEMTXATTRS_MACHINE helper

[Alex Bennée]

We will need this shortly for machine specific transactions for the PC
IOAPIC.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/memattrs.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 8359fc448b..b92f11aaa4 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -104,6 +104,14 @@ typedef struct MemTxAttrs {
                              {.requester_type = MTRT_PCI,   \
                              .requester_id = pci_requester_id(dev)})
 
+/*
+ * Helper for setting a machine specific sourced transaction. The
+ * details of how to decode the requester_id are machine specific.
+ */
+#define MEMTXATTRS_MACHINE(id) ((MemTxAttrs) \
+                                {.requester_type = MTRT_MACHINE, \
+                                 .requester_id = id })
+
 /* New-style MMIO accessors can indicate that the transaction failed.
  * A zero (MEMTX_OK) response means success; anything else is a failure
  * of some kind. The memory subsystem will bitwise-OR together results
-- 
2.34.1

[PATCH v5 17/20] hw/intc: properly model IOAPIC MSI messages

[Alex Bennée]

On the real HW the IOAPIC is wired directly to the APIC and doesn't
really generate memory accesses on the main bus of the system. To
model this we can use the MTRT_MACHINE requester type and set the id
as a magic number to represent the IOAPIC as the source.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
---
 include/hw/i386/ioapic_internal.h |  2 ++
 hw/intc/ioapic.c                  | 35 ++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index 9880443cc7..a8c7a1418a 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -82,6 +82,8 @@
 
 #define IOAPIC_VER_ENTRIES_SHIFT        16
 
+/* Magic number to identify IOAPIC memory transactions */
+#define MEMTX_IOAPIC                    0xA71C
 
 #define TYPE_IOAPIC_COMMON "ioapic-common"
 OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, IOAPIC_COMMON)
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 264262959d..8a5418002b 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -21,6 +21,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qapi/error.h"
 #include "monitor/monitor.h"
 #include "hw/i386/apic.h"
@@ -88,9 +89,33 @@ static void ioapic_entry_parse(uint64_t entry, struct ioapic_entry_info *info)
         (info->delivery_mode << MSI_DATA_DELIVERY_MODE_SHIFT);
 }
 
-static void ioapic_service(IOAPICCommonState *s)
+/*
+ * No matter whether IR is enabled, we translate the IOAPIC message
+ * into a MSI one, and its address space will decide whether we need a
+ * translation.
+ *
+ * As the IOPIC is directly wired to the APIC writes to it are not the
+ * same as writes coming from the main bus of the machine. To model
+ * this we set its source as machine specific with the MEMTX_IOPIC
+ * id.
+ */
+static void send_ioapic_msi(struct ioapic_entry_info info)
 {
     AddressSpace *ioapic_as = X86_MACHINE(qdev_get_machine())->ioapic_as;
+    MemTxAttrs attrs = MEMTXATTRS_MACHINE(MEMTX_IOAPIC);
+    MemTxResult res;
+
+    address_space_stl_le(ioapic_as, info.addr, info.data,
+                                         attrs, &res);
+    if (res != MEMTX_OK) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: couldn't write to %"PRIx32"\n", __func__, info.addr);
+    }
+}
+
+
+static void ioapic_service(IOAPICCommonState *s)
+{
     struct ioapic_entry_info info;
     uint8_t i;
     uint32_t mask;
@@ -130,12 +155,8 @@ static void ioapic_service(IOAPICCommonState *s)
                     continue;
                 }
 #endif
-
-                /* No matter whether IR is enabled, we translate
-                 * the IOAPIC message into a MSI one, and its
-                 * address space will decide whether we need a
-                 * translation. */
-                stl_le_phys(ioapic_as, info.addr, info.data);
+                /* If not handled by KVM we now send it ourselves */
+                send_ioapic_msi(info);
             }
         }
     }
-- 
2.34.1

[PATCH v5 18/20] hw/i386: convert apic access to use MemTxAttrs

[Alex Bennée]

This allows us to correctly model invalid accesses to the interrupt
controller as well as avoiding the use of current_cpu hacks to find
the APIC structure. We have to ensure we check for MSI signals first
which shouldn't arrive from the CPU but are either triggered by PCI or
internal IOAPIC writes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>

---
v1
  - don't validate requester_id for MTRT_MACHINE, just assume IOPIC
---
 include/hw/i386/apic.h |  2 +-
 hw/i386/x86.c          | 11 +++-----
 hw/intc/apic.c         | 62 ++++++++++++++++++++++++++++--------------
 3 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
index da1d2fe155..064ea5ac1b 100644
--- a/include/hw/i386/apic.h
+++ b/include/hw/i386/apic.h
@@ -22,6 +22,6 @@ void apic_designate_bsp(DeviceState *d, bool bsp);
 int apic_get_highest_priority_irr(DeviceState *dev);
 
 /* pc.c */
-DeviceState *cpu_get_current_apic(void);
+DeviceState *cpu_get_current_apic(int cpu_index);
 
 #endif
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 78cc131926..66645a669c 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -585,14 +585,11 @@ int cpu_get_pic_interrupt(CPUX86State *env)
     return intno;
 }
 
-DeviceState *cpu_get_current_apic(void)
+DeviceState *cpu_get_current_apic(int cpu_index)
 {
-    if (current_cpu) {
-        X86CPU *cpu = X86_CPU(current_cpu);
-        return cpu->apic_state;
-    } else {
-        return NULL;
-    }
+    CPUState *cs = qemu_get_cpu(cpu_index);
+    X86CPU *cpu = X86_CPU(cs);
+    return cpu->apic_state;
 }
 
 void gsi_handler(void *opaque, int n, int level)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 3df11c34d6..0a9897e64f 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -18,9 +18,11 @@
  */
 #include "qemu/osdep.h"
 #include "qemu/thread.h"
+#include "qemu/log.h"
 #include "hw/i386/apic_internal.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/ioapic.h"
+#include "hw/i386/ioapic_internal.h"
 #include "hw/intc/i8259.h"
 #include "hw/pci/msi.h"
 #include "qemu/host-utils.h"
@@ -634,21 +636,23 @@ static void apic_timer(void *opaque)
     apic_timer_update(s, s->next_time);
 }
 
-static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
+static MemTxResult apic_mem_read(void *opaque, hwaddr addr, uint64_t *data,
+                                 unsigned int size, MemTxAttrs attrs)
 {
     DeviceState *dev;
     APICCommonState *s;
     uint32_t val;
     int index;
 
-    if (size < 4) {
-        return 0;
+    if (attrs.requester_type != MTRT_CPU) {
+        return MEMTX_ACCESS_ERROR;
     }
+    dev = cpu_get_current_apic(attrs.requester_id);
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
-        return 0;
+    if (size < 4) {
+        return MEMTX_ERROR;
     }
+
     s = APIC(dev);
 
     index = (addr >> 4) & 0xff;
@@ -719,7 +723,8 @@ static uint64_t apic_mem_read(void *opaque, hwaddr addr, unsigned size)
         break;
     }
     trace_apic_mem_readl(addr, val);
-    return val;
+    *data = val;
+    return MEMTX_OK;
 }
 
 static void apic_send_msi(MSIMessage *msi)
@@ -735,32 +740,45 @@ static void apic_send_msi(MSIMessage *msi)
     apic_deliver_irq(dest, dest_mode, delivery, vector, trigger_mode);
 }
 
-static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
-                           unsigned size)
+static MemTxResult apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
+                                  unsigned int size, MemTxAttrs attrs)
 {
     DeviceState *dev;
     APICCommonState *s;
     int index = (addr >> 4) & 0xff;
 
     if (size < 4) {
-        return;
+        return MEMTX_ERROR;
     }
 
+    /*
+     * MSI and MMIO APIC are at the same memory location, but actually
+     * not on the global bus: MSI is on PCI bus APIC is connected
+     * directly to the CPU.
+     *
+     * We can check the MemTxAttrs to check they are coming from where
+     * we expect. Even though the MSI registers are reserved in APIC
+     * MMIO and vice versa they shouldn't respond to CPU writes.
+     */
     if (addr > 0xfff || !index) {
-        /* MSI and MMIO APIC are at the same memory location,
-         * but actually not on the global bus: MSI is on PCI bus
-         * APIC is connected directly to the CPU.
-         * Mapping them on the global bus happens to work because
-         * MSI registers are reserved in APIC MMIO and vice versa. */
+        switch (attrs.requester_type) {
+        case MTRT_MACHINE: /* MEMTX_IOPIC */
+        case MTRT_PCI:     /* PCI signalled MSI */
+            break;
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: rejecting write from %d",
+                          __func__, attrs.requester_id);
+            return MEMTX_ACCESS_ERROR;
+        }
         MSIMessage msi = { .address = addr, .data = val };
         apic_send_msi(&msi);
-        return;
+        return MEMTX_OK;
     }
 
-    dev = cpu_get_current_apic();
-    if (!dev) {
-        return;
+    if (attrs.requester_type != MTRT_CPU) {
+        return MEMTX_ACCESS_ERROR;
     }
+    dev = cpu_get_current_apic(attrs.requester_id);
     s = APIC(dev);
 
     trace_apic_mem_writel(addr, val);
@@ -839,6 +857,8 @@ static void apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         s->esr |= APIC_ESR_ILLEGAL_ADDRESS;
         break;
     }
+
+    return MEMTX_OK;
 }
 
 static void apic_pre_save(APICCommonState *s)
@@ -856,8 +876,8 @@ static void apic_post_load(APICCommonState *s)
 }
 
 static const MemoryRegionOps apic_io_ops = {
-    .read = apic_mem_read,
-    .write = apic_mem_write,
+    .read_with_attrs = apic_mem_read,
+    .write_with_attrs = apic_mem_write,
     .impl.min_access_size = 1,
     .impl.max_access_size = 4,
     .valid.min_access_size = 1,
-- 
2.34.1

[PATCH v5 19/20] hw/isa: derive CPUState from MemTxAttrs in apm_ioport_writeb

[Alex Bennée]

Some of the callbacks need a CPUState so extend the interface so we
can pass that down rather than relying on current_cpu hacks.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/isa/apm.h |  2 +-
 hw/acpi/ich9.c       |  1 -
 hw/acpi/piix4.c      |  2 +-
 hw/isa/apm.c         | 21 +++++++++++++++++----
 hw/isa/lpc_ich9.c    |  5 ++---
 5 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/hw/isa/apm.h b/include/hw/isa/apm.h
index b6e070c00e..eb952e1c1c 100644
--- a/include/hw/isa/apm.h
+++ b/include/hw/isa/apm.h
@@ -6,7 +6,7 @@
 #define APM_CNT_IOPORT  0xb2
 #define ACPI_PORT_SMI_CMD APM_CNT_IOPORT
 
-typedef void (*apm_ctrl_changed_t)(uint32_t val, void *arg);
+typedef void (*apm_ctrl_changed_t)(CPUState *cs, uint32_t val, void *arg);
 
 typedef struct APMState {
     uint8_t apmc;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index bd9bbade70..70ad1cd1ff 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -30,7 +30,6 @@
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/timer.h"
-#include "hw/core/cpu.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
 #include "hw/acpi/acpi.h"
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 0a81f1ad93..43b78ef8f9 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -70,7 +70,7 @@ static void pm_tmr_timer(ACPIREGS *ar)
     acpi_update_sci(&s->ar, s->irq);
 }
 
-static void apm_ctrl_changed(uint32_t val, void *arg)
+static void apm_ctrl_changed(CPUState *cs, uint32_t val, void *arg)
 {
     PIIX4PMState *s = arg;
     PCIDevice *d = PCI_DEVICE(s);
diff --git a/hw/isa/apm.c b/hw/isa/apm.c
index dfe9020d30..95efbf2457 100644
--- a/hw/isa/apm.c
+++ b/hw/isa/apm.c
@@ -21,6 +21,8 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/core/cpu.h"
 #include "hw/isa/apm.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
@@ -30,10 +32,19 @@
 /* fixed I/O location */
 #define APM_STS_IOPORT  0xb3
 
-static void apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
-                              unsigned size)
+static MemTxResult apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
+                                     unsigned size, MemTxAttrs attrs)
 {
     APMState *apm = opaque;
+    CPUState *cs;
+
+    if (attrs.requester_type != MTRT_CPU) {
+        qemu_log_mask(LOG_UNIMP | LOG_GUEST_ERROR,
+                      "%s: saw non-CPU transaction", __func__);
+        return MEMTX_ACCESS_ERROR;
+    }
+    cs = qemu_get_cpu(attrs.requester_id);
+
     addr &= 1;
 
     trace_apm_io_write(addr, val);
@@ -41,11 +52,13 @@ static void apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
         apm->apmc = val;
 
         if (apm->callback) {
-            (apm->callback)(val, apm->arg);
+            (apm->callback)(cs, val, apm->arg);
         }
     } else {
         apm->apms = val;
     }
+
+    return MEMTX_OK;
 }
 
 static uint64_t apm_ioport_readb(void *opaque, hwaddr addr, unsigned size)
@@ -77,7 +90,7 @@ const VMStateDescription vmstate_apm = {
 
 static const MemoryRegionOps apm_ops = {
     .read = apm_ioport_readb,
-    .write = apm_ioport_writeb,
+    .write_with_attrs = apm_ioport_writeb,
     .impl = {
         .min_access_size = 1,
         .max_access_size = 1,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 0b0a83e080..2700a18a65 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -443,7 +443,7 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
 
 /* APM */
 
-static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+static void ich9_apm_ctrl_changed(CPUState *cs, uint32_t val, void *arg)
 {
     ICH9LPCState *lpc = arg;
 
@@ -459,12 +459,11 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
     if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) {
         if (lpc->smi_negotiated_features &
             (UINT64_C(1) << ICH9_LPC_SMI_F_BROADCAST_BIT)) {
-            CPUState *cs;
             CPU_FOREACH(cs) {
                 cpu_interrupt(cs, CPU_INTERRUPT_SMI);
             }
         } else {
-            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+            cpu_interrupt(cs, CPU_INTERRUPT_SMI);
         }
     }
 }
-- 
2.34.1

[PATCH v5 20/20] include/hw: add commentary to current_cpu export

[Alex Bennée]

Document the intended use of current_cpu and discourage its use in new
HW emulation code. Once we have fully converted the tree we should
probably move this extern to another header.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/cpu.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8830546121..209b88e559 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -454,6 +454,20 @@ extern CPUTailQ cpus;
 #define CPU_FOREACH_SAFE(cpu, next_cpu) \
     QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus, node, next_cpu)
 
+/**
+ * current_cpu - TLS pointing to the current executing CPU
+ *
+ * current_cpu is a thread local convenience variable containing that
+ * threads executing CPUState. It is intended to be used deep in
+ * accelerator related operations where passing down CPUState is too
+ * fiddly.
+ *
+ * Its use in HW emulation is heavily discouraged in new code as not
+ * all memory accesses will necessarily be from an executing CPU (e.g.
+ * from a debugger). HW emulation should be using MemTxAttrs to derive
+ * the exact source of a memory access. If the access is from a CPU it
+ * can be derived from qemu_get_cpu(cpu_index).
+ */
 extern __thread CPUState *current_cpu;
 
 /**
-- 
2.34.1

Re: [PATCH v5 10/20] target/microblaze: initialise MemTxAttrs for CPU access

[Edgar E. Iglesias]

On Fri, Nov 11, 2022 at 06:25:25PM +0000, Alex Bennée wrote:
> Both of these functions deal with CPU based access (as is evidenced by
> the secure check straight after). Use the new MEMTXATTRS_CPU
> constructor to ensure the correct CPU id is filled in should it ever
> be needed by any devices later.

Looks good to me!

Reviewed-by: Edgar E. Iglesias <edgar@zeroasic.com>

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/microblaze/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/microblaze/helper.c b/target/microblaze/helper.c
> index 98bdb82de8..655be3b320 100644
> --- a/target/microblaze/helper.c
> +++ b/target/microblaze/helper.c
> @@ -44,7 +44,7 @@ bool mb_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      MicroBlazeMMULookup lu;
>      unsigned int hit;
>      int prot;
> -    MemTxAttrs attrs = {};
> +    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
>  
>      attrs.secure = mb_cpu_access_is_secure(cpu, access_type);
>  
> @@ -235,7 +235,7 @@ hwaddr mb_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>      unsigned int hit;
>  
>      /* Caller doesn't initialize */
> -    *attrs = (MemTxAttrs) {};
> +    *attrs = MEMTXATTRS_CPU(cs);
>      attrs->secure = mb_cpu_access_is_secure(cpu, MMU_DATA_LOAD);
>  
>      if (mmu_idx != MMU_NOMMU_IDX) {
> -- 
> 2.34.1
> 

Re: [PATCH v5 11/20] target/sparc: initialise MemTxAttrs for CPU access

[Mark Cave-Ayland]

On 11/11/2022 18:25, Alex Bennée wrote:

> Both of the TLB fill functions and the cpu_sparc_get_phys_page deal
> with CPU based access. Use the new MEMTXATTRS_CPU constructor to
> ensure the correct CPU id is filled in should it ever be needed by any
> devices later.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/sparc/mmu_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
> index 919448a494..eeb52b5ee6 100644
> --- a/target/sparc/mmu_helper.c
> +++ b/target/sparc/mmu_helper.c
> @@ -212,7 +212,7 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       target_ulong vaddr;
>       target_ulong page_size;
>       int error_code = 0, prot, access_index;
> -    MemTxAttrs attrs = {};
> +    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
>   
>       /*
>        * TODO: If we ever need tlb_vaddr_to_host for this target,
> @@ -771,7 +771,7 @@ bool sparc_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       target_ulong vaddr;
>       hwaddr paddr;
>       target_ulong page_size;
> -    MemTxAttrs attrs = {};
> +    MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
>       int error_code = 0, prot, access_index;
>   
>       address &= TARGET_PAGE_MASK;
> @@ -890,7 +890,7 @@ static int cpu_sparc_get_phys_page(CPUSPARCState *env, hwaddr *phys,
>   {
>       target_ulong page_size;
>       int prot, access_index;
> -    MemTxAttrs attrs = {};
> +    MemTxAttrs attrs = MEMTXATTRS_CPU(env_cpu(env));
>   
>       return get_physical_address(env, phys, &prot, &access_index, &attrs, addr,
>                                   rw, mmu_idx, &page_size);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v5 01/20] hw: encode accessing CPU index in MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> +/*
> + * Bus masters which don't specify any attributes will get this which
> + * indicates none of the attributes can be used.
> + */
> +#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) \
> +                                { .requester_type = MTRT_UNSPECIFIED })
> +
> +/*
> + * Helper for setting a basic CPU sourced transaction, it expects a
> + * CPUState *
> + */
> +#define MEMTXATTRS_CPU(cs) ((MemTxAttrs) \
> +                            {.requester_type = MTRT_CPU, \
> +                             .requester_id = cs->cpu_index})
> +
> +/*
> + * Helper for setting a basic PCI sourced transaction, it expects a
> + * PCIDevice *
>   */
> +#define MEMTXATTRS_PCI(dev) ((MemTxAttrs) \
> +                             {.requester_type = MTRT_PCI,   \
> +                             .requester_id = pci_requester_id(dev)})

Any reason these second two shouldn't be inlines?
Anything with arguments gets better type checking that way, unless you need preprocessor 
magic, which is not the case here.


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


r~

Re: [PATCH v5 02/20] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 3745ac9723..4b6683f90d 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2634,6 +2634,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
>       s1_lgpgsz = result->f.lg_page_size;
>       cacheattrs1 = result->cacheattrs;
>       memset(result, 0, sizeof(*result));
> +    result->f.attrs = MEMTXATTRS_CPU(env_cpu(env));

Ouch.  This means that f.secure has been reset too, which would break Secure EL1 running 
under Secure EL2.  I'll prepare a fix for 7.2...

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


r~

>   
>       ret = get_phys_addr_lpae(env, ptw, ipa, access_type, is_el0, result, fi);
>       fi->s2addr = ipa;
> @@ -2872,7 +2873,7 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>           .in_secure = arm_is_secure(env),
>           .in_debug = true,
>       };
> -    GetPhysAddrResult res = {};
> +    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
>       ARMMMUFaultInfo fi = {};
>       bool ret;
>   
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 0f4f4fc809..5960269421 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -208,7 +208,7 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                         bool probe, uintptr_t retaddr)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
> -    GetPhysAddrResult res = {};
> +    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
>       ARMMMUFaultInfo local_fi, *fi;
>       int ret;
>   

Re: [PATCH v5 02/20] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> @@ -2872,7 +2873,7 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>           .in_secure = arm_is_secure(env),
>           .in_debug = true,
>       };
> -    GetPhysAddrResult res = {};
> +    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };
>       ARMMMUFaultInfo fi = {};
>       bool ret;
>   
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 0f4f4fc809..5960269421 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -208,7 +208,7 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                         bool probe, uintptr_t retaddr)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
> -    GetPhysAddrResult res = {};
> +    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };

Not the right level for these.

Should be set in get_phys_addr_with_struct, alongside .secure right at the top of the 
function.


r~

Re: [PATCH v5 05/20] target/arm: ensure m-profile helpers set appropriate MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> There are a number of helpers for M-profile that deal with CPU
> initiated access to the vector and stack areas. While it is unlikely
> these coincided with memory mapped IO devices it is not inconceivable.
> Embedded targets tend to attract all sorts of interesting code and for
> completeness we should tag the transaction appropriately.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v5
>    - rebase fixes for refactoring
> ---
>   target/arm/m_helper.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
> index 355cd4d60a..2fb1ef95cd 100644
> --- a/target/arm/m_helper.c
> +++ b/target/arm/m_helper.c
> @@ -184,7 +184,7 @@ static bool v7m_stack_write(ARMCPU *cpu, uint32_t addr, uint32_t value,
>       CPUState *cs = CPU(cpu);
>       CPUARMState *env = &cpu->env;
>       MemTxResult txres;
> -    GetPhysAddrResult res = {};
> +    GetPhysAddrResult res = { .f.attrs = MEMTXATTRS_CPU(cs) };

This entire patch goes away with .attrs set properly in get_phys_addr_with_struct.


r~

Re: [PATCH v5 04/20] target/arm: ensure KVM traps set appropriate MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> Although most KVM users will use the in-kernel GIC emulation it is
> perfectly possible not to. In this case we need to ensure the
> MemTxAttrs are correctly populated so the GIC can divine the source
> CPU of the operation.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v3
>    - new for v3
> v5
>    - tags
>    - use MEMTXATTRS_PCI
> ---

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


r~

Re: [PATCH v5 09/20] hw/arm: remove current_cpu hack from pxa2xx access

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> We can derive the correct CPU from CPUARMState so lets not rely on
> current_cpu.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   hw/arm/pxa2xx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

Re: [PATCH v5 10/20] target/microblaze: initialise MemTxAttrs for CPU access

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> Both of these functions deal with CPU based access (as is evidenced by
> the secure check straight after). Use the new MEMTXATTRS_CPU
> constructor to ensure the correct CPU id is filled in should it ever
> be needed by any devices later.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   target/microblaze/helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

r~

Re: [PATCH v5 11/20] target/sparc: initialise MemTxAttrs for CPU access

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> Both of the TLB fill functions and the cpu_sparc_get_phys_page deal
> with CPU based access. Use the new MEMTXATTRS_CPU constructor to
> ensure the correct CPU id is filled in should it ever be needed by any
> devices later.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   target/sparc/mmu_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

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

r~

Re: [PATCH v5 13/20] target/i386: add explicit initialisation for MexTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c
> index b185ee8de4..337090e16f 100644
> --- a/target/i386/hax/hax-all.c
> +++ b/target/i386/hax/hax-all.c
> @@ -385,7 +385,7 @@ static int hax_handle_io(CPUArchState *env, uint32_t df, uint16_t port,
>   {
>       uint8_t *ptr;
>       int i;
> -    MemTxAttrs attrs = { 0 };
> +    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>   
>       if (!df) {
>           ptr = (uint8_t *) buffer;
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index b75738ee9c..cb0720a6fa 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -502,7 +502,7 @@ nvmm_vcpu_post_run(CPUState *cpu, struct nvmm_vcpu_exit *exit)
>   static void
>   nvmm_io_callback(struct nvmm_io *io)
>   {
> -    MemTxAttrs attrs = { 0 };
> +    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>       int ret;
>   
>       ret = address_space_rw(&address_space_io, io->port, attrs, io->data,
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index e738d83e81..42846144dd 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -791,7 +791,7 @@ static HRESULT CALLBACK whpx_emu_ioport_callback(
>       void *ctx,
>       WHV_EMULATOR_IO_ACCESS_INFO *IoAccess)
>   {
> -    MemTxAttrs attrs = { 0 };
> +    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>       address_space_rw(&address_space_io, IoAccess->Port, attrs,
>                        &IoAccess->Data, IoAccess->AccessSize,
>                        IoAccess->Direction);

All three of these are hypervisor callouts to handle i/o for the guest, just like kvm.


r~

Re: [PATCH v5 14/20] hw/audio: explicitly set .requester_type for intel-hda

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> This is simulating a bus master writing data back into system memory.
> Mark it as such.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/audio/intel-hda.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index f38117057b..95c28b315c 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -345,7 +345,7 @@ static void intel_hda_corb_run(IntelHDAState *d)
>   
>   static void intel_hda_response(HDACodecDevice *dev, bool solicited, uint32_t response)
>   {
> -    const MemTxAttrs attrs = { .memory = true };
> +    const MemTxAttrs attrs = { .requester_type = MTRT_PCI, .memory = true };

MEMTXATTRS_PCI?


r~

Re: [PATCH v5 15/20] hw/i386: update vapic_write to use MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> This allows us to drop the current_cpu hack and properly model an
> invalid access to the vapic.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   hw/i386/kvmvapic.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)

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

r~

Re: [PATCH v5 16/20] include: add MEMTXATTRS_MACHINE helper

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> We will need this shortly for machine specific transactions for the PC
> IOAPIC.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/exec/memattrs.h | 8 ++++++++
>   1 file changed, 8 insertions(+)

Fold into patch 1?  Anyway,

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


r~

Re: [PATCH v5 17/20] hw/intc: properly model IOAPIC MSI messages

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> On the real HW the IOAPIC is wired directly to the APIC and doesn't
> really generate memory accesses on the main bus of the system. To
> model this we can use the MTRT_MACHINE requester type and set the id
> as a magic number to represent the IOAPIC as the source.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> ---
>   include/hw/i386/ioapic_internal.h |  2 ++
>   hw/intc/ioapic.c                  | 35 ++++++++++++++++++++++++-------
>   2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
> index 9880443cc7..a8c7a1418a 100644
> --- a/include/hw/i386/ioapic_internal.h
> +++ b/include/hw/i386/ioapic_internal.h
> @@ -82,6 +82,8 @@
>   
>   #define IOAPIC_VER_ENTRIES_SHIFT        16
>   
> +/* Magic number to identify IOAPIC memory transactions */
> +#define MEMTX_IOAPIC                    0xA71C

Closing in on 1337 5p34k -- sure you didn't want a '4' there to start?  ;-)

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

r~

Re: [PATCH v5 18/20] hw/i386: convert apic access to use MemTxAttrs

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> +        switch (attrs.requester_type) {
> +        case MTRT_MACHINE: /* MEMTX_IOPIC */

Not checking the id?

> +        case MTRT_PCI:     /* PCI signalled MSI */
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: rejecting write from %d",
> +                          __func__, attrs.requester_id);
> +            return MEMTX_ACCESS_ERROR;
> +        }

Logging the requester_id by itself isn't great -- you need the type to know what you're 
looking at.

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


r~

Re: [PATCH v5 19/20] hw/isa: derive CPUState from MemTxAttrs in apm_ioport_writeb

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> Some of the callbacks need a CPUState so extend the interface so we
> can pass that down rather than relying on current_cpu hacks.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/hw/isa/apm.h |  2 +-
>   hw/acpi/ich9.c       |  1 -
>   hw/acpi/piix4.c      |  2 +-
>   hw/isa/apm.c         | 21 +++++++++++++++++----
>   hw/isa/lpc_ich9.c    |  5 ++---
>   5 files changed, 21 insertions(+), 10 deletions(-)

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

r~

Re: [PATCH v5 20/20] include/hw: add commentary to current_cpu export

[Richard Henderson]

On 11/12/22 04:25, Alex Bennée wrote:
> Document the intended use of current_cpu and discourage its use in new
> HW emulation code. Once we have fully converted the tree we should
> probably move this extern to another header.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/hw/core/cpu.h | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

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

r~

Re: [PATCH v5 09/20] hw/arm: remove current_cpu hack from pxa2xx access

[Philippe Mathieu-Daudé]

On 11/11/22 19:25, Alex Bennée wrote:
> We can derive the correct CPU from CPUARMState so lets not rely on
> current_cpu.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/arm/pxa2xx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v5 10/20] target/microblaze: initialise MemTxAttrs for CPU access

[Philippe Mathieu-Daudé]

On 11/11/22 19:25, Alex Bennée wrote:
> Both of these functions deal with CPU based access (as is evidenced by
> the secure check straight after). Use the new MEMTXATTRS_CPU
> constructor to ensure the correct CPU id is filled in should it ever
> be needed by any devices later.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/microblaze/helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v5 11/20] target/sparc: initialise MemTxAttrs for CPU access

[Philippe Mathieu-Daudé]

On 11/11/22 19:25, Alex Bennée wrote:
> Both of the TLB fill functions and the cpu_sparc_get_phys_page deal
> with CPU based access. Use the new MEMTXATTRS_CPU constructor to
> ensure the correct CPU id is filled in should it ever be needed by any
> devices later.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/sparc/mmu_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v5 14/20] hw/audio: explicitly set .requester_type for intel-hda

[Philippe Mathieu-Daudé]

On 12/11/22 06:50, Richard Henderson wrote:
> On 11/12/22 04:25, Alex Bennée wrote:
>> This is simulating a bus master writing data back into system memory.
>> Mark it as such.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   hw/audio/intel-hda.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
>> index f38117057b..95c28b315c 100644
>> --- a/hw/audio/intel-hda.c
>> +++ b/hw/audio/intel-hda.c
>> @@ -345,7 +345,7 @@ static void intel_hda_corb_run(IntelHDAState *d)
>>   static void intel_hda_response(HDACodecDevice *dev, bool solicited, 
>> uint32_t response)
>>   {
>> -    const MemTxAttrs attrs = { .memory = true };
>> +    const MemTxAttrs attrs = { .requester_type = MTRT_PCI, .memory = 
>> true };
> 
> MEMTXATTRS_PCI?

Then removing the 'const' qualifier and setting .memory after.

Re: [PATCH v5 15/20] hw/i386: update vapic_write to use MemTxAttrs

[Philippe Mathieu-Daudé]

On 11/11/22 19:25, Alex Bennée wrote:
> This allows us to drop the current_cpu hack and properly model an
> invalid access to the vapic.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/i386/kvmvapic.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 43f8a8f679..a76ed07199 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -635,20 +635,21 @@ static int vapic_prepare(VAPICROMState *s)
>       return 0;
>   }
>   
> -static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
> -                        unsigned int size)
> +static MemTxResult vapic_write(void *opaque, hwaddr addr, uint64_t data,
> +                               unsigned int size, MemTxAttrs attrs)
>   {
>       VAPICROMState *s = opaque;
> +    CPUState *cs;
>       X86CPU *cpu;
>       CPUX86State *env;
>       hwaddr rom_paddr;
>   
> -    if (!current_cpu) {
> -        return;
> +    if (attrs.requester_type != MTRT_CPU) {
> +        return MEMTX_ACCESS_ERROR;
>       }
> -
> -    cpu_synchronize_state(current_cpu);
> -    cpu = X86_CPU(current_cpu);
> +    cs = qemu_get_cpu(attrs.requester_id);
> +    cpu_synchronize_state(cs);
> +    cpu = X86_CPU(cs);
>       env = &cpu->env;
>   
>       /*
> @@ -708,6 +709,8 @@ static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
>           }
>           break;
>       }
> +
> +    return MEMTX_OK;
>   }
>   
>   static uint64_t vapic_read(void *opaque, hwaddr addr, unsigned size)
> @@ -716,7 +719,7 @@ static uint64_t vapic_read(void *opaque, hwaddr addr, unsigned size)
>   }
>   
>   static const MemoryRegionOps vapic_ops = {
> -    .write = vapic_write,
> +    .write_with_attrs = vapic_write,
>       .read = vapic_read,

Shouldn't we do the same for the read() path?

>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };

Re: [PATCH v5 19/20] hw/isa: derive CPUState from MemTxAttrs in apm_ioport_writeb

[Philippe Mathieu-Daudé]

On 11/11/22 19:25, Alex Bennée wrote:
> Some of the callbacks need a CPUState so extend the interface so we
> can pass that down rather than relying on current_cpu hacks.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/isa/apm.h |  2 +-
>   hw/acpi/ich9.c       |  1 -
>   hw/acpi/piix4.c      |  2 +-
>   hw/isa/apm.c         | 21 +++++++++++++++++----
>   hw/isa/lpc_ich9.c    |  5 ++---
>   5 files changed, 21 insertions(+), 10 deletions(-)


> -static void apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
> -                              unsigned size)
> +static MemTxResult apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
> +                                     unsigned size, MemTxAttrs attrs)
>   {
>       APMState *apm = opaque;
> +    CPUState *cs;
> +
> +    if (attrs.requester_type != MTRT_CPU) {
> +        qemu_log_mask(LOG_UNIMP | LOG_GUEST_ERROR,
> +                      "%s: saw non-CPU transaction", __func__);
> +        return MEMTX_ACCESS_ERROR;

Are you sure it is illegal?

> +    }
> +    cs = qemu_get_cpu(attrs.requester_id);
> +
>       addr &= 1;
>   
>       trace_apm_io_write(addr, val);
> @@ -41,11 +52,13 @@ static void apm_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>           apm->apmc = val;
>   
>           if (apm->callback) {
> -            (apm->callback)(val, apm->arg);
> +            (apm->callback)(cs, val, apm->arg);
>           }
>       } else {
>           apm->apms = val;
>       }
> +
> +    return MEMTX_OK;
>   }

Re: [PATCH v5 01/20] hw: encode accessing CPU index in MemTxAttrs

[Peter Maydell]

On Fri, 11 Nov 2022 at 18:25, 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 so CPU accesses can
> be explicitly marked.
>
> To achieve this we create a new requester_type which indicates to
> consumers how requester_id it to be consumed. We absorb the existing
> unspecified:1 bitfield into this type and also document a potential
> machine specific encoding which will be useful to (currently)
> out-of-tree extensions.
>
> Places that checked to see if things where unspecified now instead
> check the source if what they expected.
>
> 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
>   hypervisors offloading device emulation to QEMU
>
> I'll start addressing some of these in following patches.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>  include/exec/memattrs.h | 68 ++++++++++++++++++++++++++++++++---------
>  hw/i386/amd_iommu.c     |  6 ++--
>  hw/i386/intel_iommu.c   |  2 +-
>  hw/misc/tz-mpc.c        |  2 +-
>  hw/misc/tz-msc.c        |  6 ++--
>  hw/pci/pci.c            |  4 +--
>  6 files changed, 60 insertions(+), 28 deletions(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..8359fc448b 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -14,7 +14,32 @@
>  #ifndef MEMATTRS_H
>  #define MEMATTRS_H
>
> -/* Every memory transaction has associated with it a set of
> +/**
> + * typedef MemTxRequesterType - source of memory transaction
> + *
> + * Every memory transaction comes from a specific place which defines
> + * how requester_id should be handled if at all.
> + *
> + * UNSPECIFIED: the default for otherwise undefined MemTxAttrs
> + * CPU: requester_id is the global cpu_index
> + *      This needs further processing if you need to work out which
> + *      socket or complex it comes from
> + * PCI: indicates the requester_id is a PCI id
> + * MACHINE: indicates a machine specific encoding
> + *          This will require further processing to decode into its
> + *          constituent parts.
> + */
> +typedef enum MemTxRequesterType {
> +    MTRT_UNSPECIFIED = 0,
> +    MTRT_CPU,
> +    MTRT_PCI,
> +    MTRT_MACHINE
> +} MemTxRequesterType;

This ends up squashing two distinct things into one field:
 (1) what are the semantics of the requester_id field?
 (2) did the caller explicitly specify their attrs, or
     are they using the legacy MEMTXATTRS_UNSPECIFIED macro?

One might reasonably be explicitly specifying tx attrs
and yet not be using any of the 3 listed requester_id field
semantics. (In fact we have various places that do
exactly that, like tulip_desc_read().)

I think that the requester_type field should just be a
discriminator field that tells you how to interpret
requester_id, and nothing more, so the values would
be "CPU", "PCI", "machine" and "none".

I'm not super-enthusiastic about "MACHINE" here, but
I guess we can see how it works out over time.

> +/**
> + * typedef MemTxAttrs - attributes of a memory transaction
> + *
> + * Every memory transaction has associated with it a set of
>   * attributes. Some of these are generic (such as the ID of
>   * the bus master); some are specific to a particular kind of
>   * bus (such as the ARM Secure/NonSecure bit). We define them
> @@ -23,13 +48,12 @@
>   * different semantics.
>   */
>  typedef struct MemTxAttrs {
> -    /* Bus masters which don't specify any attributes will get this
> -     * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
> -     * distinguish "all attributes deliberately clear" from
> -     * "didn't specify" if necessary.
> -     */
> -    unsigned int unspecified:1;
> -    /* ARM/AMBA: TrustZone Secure access
> +    /* Requester type (e.g. CPU or PCI MSI) */
> +    MemTxRequesterType requester_type:2;
> +    /* Requester ID */
> +    unsigned int requester_id:16;
> +    /*
> +     * ARM/AMBA: TrustZone Secure access
>       * x86: System Management Mode access
>       */
>      unsigned int secure:1;
> @@ -43,8 +67,6 @@ typedef struct MemTxAttrs {
>       * (see MEMTX_ACCESS_ERROR).
>       */
>      unsigned int memory:1;
> -    /* Requester ID (for MSI for example) */
> -    unsigned int requester_id:16;
>      /* Invert endianness for this page */
>      unsigned int byte_swap:1;
>      /*
> @@ -59,12 +81,28 @@ typedef struct MemTxAttrs {
>      unsigned int target_tlb_bit2 : 1;
>  } MemTxAttrs;
>
> -/* Bus masters which don't specify any attributes will get this,
> - * which has all attribute bits clear except the topmost one
> - * (so that we can distinguish "all attributes deliberately clear"
> - * from "didn't specify" if necessary).
> +/*
> + * Bus masters which don't specify any attributes will get this which
> + * indicates none of the attributes can be used.
> + */
> +#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) \
> +                                { .requester_type = MTRT_UNSPECIFIED })

This isn't the semantics I intended for MEMTXATTRS_UNSPECIFIED --
the recipient of a MEMTXATTRS_UNSPECIFIED is still allowed to look
at the various attribute bits, which should be set to "plausible
defaults". It just means that you can tell, for instance, "non-secure
because sender specifically asked for that" from "non-secure because
sender is an old bit of non-memtxattrs-aware code that doesn't
care to specify". Almost all code that looks at attrs.secure, for
instance, does it without previously checking attrs.unspecified,
because it doesn't have to. There are only a very few places that
do need to check attrs.unspecified:
 * the two places changed in this patch, which are really trying
   to check "is this a PCI transaction with PCI requester_id"
 * tz-mpc.c, which wants to treat MEMTXATTRS_UNSPECIFIED as being
   Secure for the reasons described in the comment in
   tz_mpc_attrs_to_index()

Perhaps what we should do is address the reason that tz-mpc.c
is trying to use attrs.unspecified in a better way. Specifically,
we could add a MemTxAttrs:is_debug bit that indicates debug
writes, and arrange that code like the rom-image-loader sets
that appropriately. Then tz-mpc.c could say "pass through a
debug transaction regardless of its secure bit, but treat
real transactions the way the hardware does", instead of the
current fudge it does.

> +/*
> + * Helper for setting a basic CPU sourced transaction, it expects a
> + * CPUState *
> + */
> +#define MEMTXATTRS_CPU(cs) ((MemTxAttrs) \
> +                            {.requester_type = MTRT_CPU, \
> +                             .requester_id = cs->cpu_index})
> +
> +/*
> + * Helper for setting a basic PCI sourced transaction, it expects a
> + * PCIDevice *
>   */
> -#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
> +#define MEMTXATTRS_PCI(dev) ((MemTxAttrs) \
> +                             {.requester_type = MTRT_PCI,   \
> +                             .requester_id = pci_requester_id(dev)})

Indent looks odd.

>  /* New-style MMIO accessors can indicate that the transaction failed.
>   * A zero (MEMTX_OK) response means success; anything else is a failure
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 725f69095b..284359c16e 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -153,9 +153,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
>  {
>      MSIMessage msg = {};
> -    MemTxAttrs attrs = {
> -        .requester_id = pci_requester_id(&s->pci.dev)
> -    };
> +    MemTxAttrs attrs = MEMTXATTRS_PCI(&s->pci.dev);
>
>      if (msi_enabled(&s->pci.dev)) {
>          msg = msi_get_message(&s->pci.dev, 0);
> @@ -1356,7 +1354,7 @@ static MemTxResult amdvi_mem_ir_write(void *opaque, hwaddr addr,
>
>      trace_amdvi_mem_ir_write_req(addr, value, size);
>
> -    if (!attrs.unspecified) {
> +    if (attrs.requester_type == MTRT_PCI) {
>          /* We have explicit Source ID */
>          sid = attrs.requester_id;
>      }
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a08ee85edf..12752413eb 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3517,7 +3517,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
>      from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
>      from.data = (uint32_t) value;
>
> -    if (!attrs.unspecified) {
> +    if (attrs.requester_type == MTRT_PCI) {
>          /* We have explicit Source ID */
>          sid = attrs.requester_id;
>      }
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index 30481e1c90..4beb5daa1a 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -461,7 +461,7 @@ static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
>       * All the real during-emulation transactions from the CPU will
>       * specify attributes.
>       */
> -    return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
> +    return ((attrs.requester_type == MTRT_UNSPECIFIED) || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
>  }
>
>  static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
> diff --git a/hw/misc/tz-msc.c b/hw/misc/tz-msc.c
> index acbe94400b..e93bfc7083 100644
> --- a/hw/misc/tz-msc.c
> +++ b/hw/misc/tz-msc.c
> @@ -137,11 +137,9 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
>          return MEMTX_OK;
>      case MSCAllowSecure:
>          attrs.secure = 1;
> -        attrs.unspecified = 0;
>          break;
>      case MSCAllowNonSecure:
>          attrs.secure = 0;
> -        attrs.unspecified = 0;
>          break;
>      }
>
> @@ -179,11 +177,11 @@ static MemTxResult tz_msc_write(void *opaque, hwaddr addr, uint64_t val,
>          return MEMTX_OK;
>      case MSCAllowSecure:
>          attrs.secure = 1;
> -        attrs.unspecified = 0;
> +        attrs.requester_type = MTRT_CPU;
>          break;
>      case MSCAllowNonSecure:
>          attrs.secure = 0;
> -        attrs.unspecified = 0;
> +        attrs.requester_type = MTRT_CPU;
>          break;
>      }

Whatever we do here, the handling of read and write transactions
should be the same, not different.

Forcing the requester_type to "this is the CPU" looks odd,
because the transaction might not be from the CPU (it could
be from a DMA-capable device).

I think probably the right thing here is "set attrs.secure as
required; don't touch any other fields of the attrs".

> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2f450f6a72..1d0d8d866f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -319,9 +319,7 @@ void pci_device_deassert_intx(PCIDevice *dev)
>
>  static void pci_msi_trigger(PCIDevice *dev, MSIMessage msg)
>  {
> -    MemTxAttrs attrs = {};
> -
> -    attrs.requester_id = pci_requester_id(dev);
> +    MemTxAttrs attrs = MEMTXATTRS_PCI(dev);
>      address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
>                           attrs, NULL);
>  }
> --
> 2.34.1

thanks
-- PMM

Re: [PATCH v5 14/20] hw/audio: explicitly set .requester_type for intel-hda

[Peter Maydell]

On Fri, 11 Nov 2022 at 18:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This is simulating a bus master writing data back into system memory.
> Mark it as such.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/audio/intel-hda.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index f38117057b..95c28b315c 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -345,7 +345,7 @@ static void intel_hda_corb_run(IntelHDAState *d)
>
>  static void intel_hda_response(HDACodecDevice *dev, bool solicited, uint32_t response)
>  {
> -    const MemTxAttrs attrs = { .memory = true };
> +    const MemTxAttrs attrs = { .requester_type = MTRT_PCI, .memory = true };

This doesn't look right -- it says "the requester_id field
is a PCI requester ID" but it doesn't fill in requester_id.


>      HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
>      IntelHDAState *d = container_of(bus, IntelHDAState, codecs);
>      hwaddr addr;

What breaks if we don't set this? Put another way, why do
we need to change this but not all the other PCI device models that
do DMA writes, most of which use MEMTXATTRS_UNSPECIFIED ?

I wonder if stl_le_pci_dma() and friends should set the
requester_id on the attrs that they are passed ?

-- PMM

Re: [PATCH v5 18/20] hw/i386: convert apic access to use MemTxAttrs

[Peter Maydell]

On Fri, 11 Nov 2022 at 18:36, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This allows us to correctly model invalid accesses to the interrupt
> controller as well as avoiding the use of current_cpu hacks to find
> the APIC structure. We have to ensure we check for MSI signals first
> which shouldn't arrive from the CPU but are either triggered by PCI or
> internal IOAPIC writes.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>

> +static MemTxResult apic_mem_write(void *opaque, hwaddr addr, uint64_t val,
> +                                  unsigned int size, MemTxAttrs attrs)
>  {
>      DeviceState *dev;
>      APICCommonState *s;
>      int index = (addr >> 4) & 0xff;
>
>      if (size < 4) {
> -        return;
> +        return MEMTX_ERROR;
>      }
>
> +    /*
> +     * MSI and MMIO APIC are at the same memory location, but actually
> +     * not on the global bus: MSI is on PCI bus APIC is connected
> +     * directly to the CPU.
> +     *
> +     * We can check the MemTxAttrs to check they are coming from where
> +     * we expect. Even though the MSI registers are reserved in APIC
> +     * MMIO and vice versa they shouldn't respond to CPU writes.
> +     */
>      if (addr > 0xfff || !index) {
> -        /* MSI and MMIO APIC are at the same memory location,
> -         * but actually not on the global bus: MSI is on PCI bus
> -         * APIC is connected directly to the CPU.
> -         * Mapping them on the global bus happens to work because
> -         * MSI registers are reserved in APIC MMIO and vice versa. */
> +        switch (attrs.requester_type) {
> +        case MTRT_MACHINE: /* MEMTX_IOPIC */
> +        case MTRT_PCI:     /* PCI signalled MSI */
> +            break;

If we always treat MTRT_MACHINE and MTRT_PCI identically, do we really
need to have different MTRT types for them ?

> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: rejecting write from %d",
> +                          __func__, attrs.requester_id);
> +            return MEMTX_ACCESS_ERROR;
> +        }
>          MSIMessage msi = { .address = addr, .data = val };
>          apic_send_msi(&msi);
> -        return;
> +        return MEMTX_OK;
>      }

thanks
-- PMM

Re: [PATCH v5 14/20] hw/audio: explicitly set .requester_type for intel-hda

[Philippe Mathieu-Daudé]

On 21/11/22 19:39, Peter Maydell wrote:
> On Fri, 11 Nov 2022 at 18:35, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> This is simulating a bus master writing data back into system memory.
>> Mark it as such.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   hw/audio/intel-hda.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>

> I wonder if stl_le_pci_dma() and friends should set the
> requester_id on the attrs that they are passed ?

Very good point!