* [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring)
@ 2022-09-27 14:14 Alex Bennée
2022-09-27 14:14 ` [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
` (14 more replies)
0 siblings, 15 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée
Hi,
Following feedback on the MemTxAttrs updates we now:
- move unspecified to requester_type
- document a machine specific decoding of requester_id
- update the existing users
- update HVF/KVM/m-profile/page-walking access functions
- fail with MEMTX_ACCESS_ERROR if non-CPU accesses GIC
I suspect this will be my last run at this for a while so if it is
still wanting I'll split off the re-factoring work for a PR so it is
not held up. The following still need review:
- accel/kvm: move kvm_update_guest_debug to inline stub
- qtest: make read/write operation appear to be from CPU
- target/arm: ensure m-profile helpers set appropriate MemTxAttrs
- target/arm: ensure ptw accesses set appropriate MemTxAttrs
- target/arm: ensure KVM traps set appropriate MemTxAttrs
- target/arm: ensure HVF traps set appropriate MemTxAttrs
- target/arm: ensure TCG IO accesses set appropriate MemTxAttrs
- hw: encode accessing CPU index in MemTxAttrs
Alex Bennée (15):
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 ptw accesses 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
configure: move detected gdb to TCG's config-host.mak
gdbstub: move into its own sub directory
gdbstub: move sstep flags probing into AccelClass
gdbstub: move breakpoint logic to accel ops
gdbstub: move guest debug support check to ops
accel/kvm: move kvm_update_guest_debug to inline stub
configure | 7 ++
meson.build | 4 +-
accel/kvm/kvm-cpus.h | 4 +
gdbstub/internals.h | 17 ++++
gdbstub/trace.h | 1 +
include/exec/memattrs.h | 39 ++++++--
include/qemu/accel.h | 12 +++
include/sysemu/accel-ops.h | 7 ++
include/sysemu/cpus.h | 3 +
include/sysemu/kvm.h | 36 +++----
accel/accel-common.c | 10 ++
accel/kvm/kvm-accel-ops.c | 9 ++
accel/kvm/kvm-all.c | 48 ++++-----
accel/stubs/kvm-stub.c | 21 ----
accel/tcg/tcg-accel-ops.c | 98 +++++++++++++++++++
accel/tcg/tcg-all.c | 17 ++++
gdbstub.c => gdbstub/gdbstub.c | 156 +++--------------------------
gdbstub/softmmu.c | 51 ++++++++++
gdbstub/user.c | 68 +++++++++++++
hw/i386/amd_iommu.c | 3 +-
hw/i386/intel_iommu.c | 2 +-
hw/intc/arm_gic.c | 174 ++++++++++++++++++++++-----------
hw/misc/tz-mpc.c | 2 +-
hw/misc/tz-msc.c | 8 +-
hw/pci/pci.c | 7 +-
hw/timer/arm_mptimer.c | 25 ++---
softmmu/cpus.c | 7 ++
softmmu/qtest.c | 26 ++---
target/arm/hvf/hvf.c | 4 +-
target/arm/kvm.c | 12 ++-
target/arm/m_helper.c | 12 +--
target/arm/ptw.c | 7 +-
MAINTAINERS | 2 +-
gdbstub/meson.build | 9 ++
gdbstub/trace-events | 29 ++++++
trace-events | 28 ------
36 files changed, 615 insertions(+), 350 deletions(-)
create mode 100644 gdbstub/internals.h
create mode 100644 gdbstub/trace.h
rename gdbstub.c => gdbstub/gdbstub.c (95%)
create mode 100644 gdbstub/softmmu.c
create mode 100644 gdbstub/user.c
create mode 100644 gdbstub/meson.build
create mode 100644 gdbstub/trace-events
--
2.34.1
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:42 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs Alex Bennée
` (13 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Michael S. Tsirkin, Marcel Apfelbaum,
Paolo Bonzini, Richard Henderson, Eduardo Habkost, Peter Xu,
Jason Wang, Peter Maydell
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.
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
---
include/exec/memattrs.h | 39 +++++++++++++++++++++++++++++++--------
hw/i386/amd_iommu.c | 3 ++-
hw/i386/intel_iommu.c | 2 +-
hw/misc/tz-mpc.c | 2 +-
hw/misc/tz-msc.c | 8 ++++----
hw/pci/pci.c | 7 ++++---
6 files changed, 43 insertions(+), 18 deletions(-)
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..67625c6344 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -14,6 +14,24 @@
#ifndef MEMATTRS_H
#define MEMATTRS_H
+/*
+ * Every memory transaction comes from a specific place which defines
+ * how requester_id should be handled. For CPU's the requester_id is
+ * the global cpu_index which needs further processing if you need to
+ * work out which socket or complex it comes from. UNSPECIFIED is the
+ * default for otherwise undefined MemTxAttrs. PCI indicates the
+ * requester_id is a PCI id. MACHINE indicates a machine specific
+ * encoding which needs further processing to decode into its
+ * constituent parts.
+ */
+typedef enum MemTxRequesterType {
+ MTRT_CPU = 0,
+ MTRT_UNSPECIFIED,
+ MTRT_PCI,
+ MTRT_MACHINE
+} MemTxRequesterType;
+
+
/* 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
@@ -23,12 +41,6 @@
* 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
* x86: System Management Mode access
*/
@@ -43,7 +55,9 @@ typedef struct MemTxAttrs {
* (see MEMTX_ACCESS_ERROR).
*/
unsigned int memory:1;
- /* Requester ID (for MSI for example) */
+ /* Requester type (e.g. CPU or PCI MSI) */
+ MemTxRequesterType requester_type:2;
+ /* Requester ID */
unsigned int requester_id:16;
/* Invert endianness for this page */
unsigned int byte_swap:1;
@@ -64,7 +78,16 @@ typedef struct MemTxAttrs {
* (so that we can distinguish "all attributes deliberately clear"
* from "didn't specify" if necessary).
*/
-#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
+#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})
/* 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..8db2b6b692 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -154,6 +154,7 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
{
MSIMessage msg = {};
MemTxAttrs attrs = {
+ .requester_type = MTRT_PCI,
.requester_id = pci_requester_id(&s->pci.dev)
};
@@ -1356,7 +1357,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 05d53a1aa9..89b9b9a3e6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3394,7 +3394,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..0b47972a46 100644
--- a/hw/misc/tz-msc.c
+++ b/hw/misc/tz-msc.c
@@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
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;
}
@@ -179,11 +179,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..ccdd71e4ba 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -319,9 +319,10 @@ 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 = {
+ .requester_type = MTRT_PCI,
+ .requester_id = pci_requester_id(dev)
+ };
address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
attrs, NULL);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
2022-09-27 14:14 ` [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:45 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 03/15] target/arm: ensure HVF traps " Alex Bennée
` (12 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée, Peter Maydell
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.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v3
- reword commit summary
---
target/arm/ptw.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2ddfc028ab..4b0dc9bd14 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2289,6 +2289,9 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
bool is_secure = regime_is_secure(env, mmu_idx);
+ attrs->requester_type = MEMTXATTRS_CPU;
+ attrs->requester_id = env_cpu(env)->cpu_index;
+
if (mmu_idx != s1_mmu_idx) {
/*
* Call ourselves recursively to do the stage 1 and then stage 2
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 03/15] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
2022-09-27 14:14 ` [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
2022-09-27 14:14 ` [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:47 ` Richard Henderson
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:14 ` Alex Bennée
` (11 subsequent siblings)
14 siblings, 2 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Alexander Graf, Mads Ynddal, Peter Maydell
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).
Acked-by: Alexander Graf <agraf@csgraf.de>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mads Ynddal <mads@ynddal.dk>
---
v2
- update MEMTXATTRS macro
---
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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 04/15] target/arm: ensure KVM traps set appropriate MemTxAttrs
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-27 14:14 ` [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs Alex Bennée
` (13 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Peter Maydell, Paolo Bonzini,
open list:Overall KVM CPUs
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.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v3
- new for v3
---
target/arm/kvm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index e5c1bd50d2..05056562f4 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -801,13 +801,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);
@@ -848,7 +849,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)
@@ -1003,6 +1004,10 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
hwaddr xlat, len, doorbell_gpa;
MemoryRegionSection mrs;
MemoryRegion *mr;
+ MemTxAttrs attrs = {
+ .requester_type = MTRT_PCI,
+ .requester_id = pci_requester_id(dev)
+ };
if (as == &address_space_memory) {
return 0;
@@ -1012,8 +1017,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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 04/15] target/arm: ensure KVM traps set appropriate MemTxAttrs
@ 2022-09-27 14:14 ` Alex Bennée
0 siblings, 0 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Peter Maydell, Paolo Bonzini,
open list:Overall KVM CPUs
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.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v3
- new for v3
---
target/arm/kvm.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index e5c1bd50d2..05056562f4 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -801,13 +801,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);
@@ -848,7 +849,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)
@@ -1003,6 +1004,10 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
hwaddr xlat, len, doorbell_gpa;
MemoryRegionSection mrs;
MemoryRegion *mr;
+ MemTxAttrs attrs = {
+ .requester_type = MTRT_PCI,
+ .requester_id = pci_requester_id(dev)
+ };
if (as == &address_space_memory) {
return 0;
@@ -1012,8 +1017,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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 05/15] target/arm: ensure ptw accesses set appropriate MemTxAttrs
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (3 preceding siblings ...)
2022-09-27 14:14 ` Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:52 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 06/15] target/arm: ensure m-profile helpers " Alex Bennée
` (9 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée, Peter Maydell
While mapping your page table base to the GICs address space would be
an "interesting" design choice the resultant loads would still be CPU
initiated so should be tagged as such.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
target/arm/ptw.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 4b0dc9bd14..62d32d660a 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -252,7 +252,7 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
ARMMMUIdx mmu_idx, ARMMMUFaultInfo *fi)
{
CPUState *cs = env_cpu(env);
- MemTxAttrs attrs = {};
+ MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
MemTxResult result = MEMTX_OK;
AddressSpace *as;
uint32_t data;
@@ -280,7 +280,7 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
ARMMMUIdx mmu_idx, ARMMMUFaultInfo *fi)
{
CPUState *cs = env_cpu(env);
- MemTxAttrs attrs = {};
+ MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
MemTxResult result = MEMTX_OK;
AddressSpace *as;
uint64_t data;
@@ -2289,8 +2289,8 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
bool is_secure = regime_is_secure(env, mmu_idx);
- attrs->requester_type = MEMTXATTRS_CPU;
- attrs->requester_id = env_cpu(env)->cpu_index;
+ result->attrs.requester_type = MTRT_CPU;
+ result->attrs.requester_id = env_cpu(env)->cpu_index;
if (mmu_idx != s1_mmu_idx) {
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 06/15] target/arm: ensure m-profile helpers set appropriate MemTxAttrs
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (4 preceding siblings ...)
2022-09-27 14:14 ` [PATCH v3 05/15] target/arm: ensure ptw accesses " Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:57 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 07/15] qtest: make read/write operation appear to be from CPU Alex Bennée
` (8 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée, Peter Maydell
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>
---
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 5ee4ee15b3..d244e9c1c5 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 = { .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 = { .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,
CPUState *cs = CPU(cpu);
CPUARMState *env = &cpu->env;
V8M_SAttributes sattrs = {};
- GetPhysAddrResult res = {};
+ GetPhysAddrResult res = { .attrs = MEMTXATTRS_CPU(cs) };
ARMMMUFaultInfo fi = {};
MemTxResult txres;
@@ -2048,7 +2048,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 = { .attrs = MEMTXATTRS_CPU(cs) };
ARMMMUFaultInfo fi = {};
uint32_t value;
@@ -2806,7 +2806,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 = { .attrs = MEMTXATTRS_CPU(env_cpu(env)) };
ARMMMUFaultInfo fi = {};
/* We can ignore the return value as prot is always set */
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 07/15] qtest: make read/write operation appear to be from CPU
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (5 preceding siblings ...)
2022-09-27 14:14 ` [PATCH v3 06/15] target/arm: ensure m-profile helpers " Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:58 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
` (7 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Thomas Huth, Laurent Vivier, Paolo Bonzini
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.
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 f8acef2628..7d29d54a4c 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (6 preceding siblings ...)
2022-09-27 14:14 ` [PATCH v3 07/15] qtest: make read/write operation appear to be from CPU Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 17:03 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
` (6 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée, Richard Henderson, Peter Maydell
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
---
hw/intc/arm_gic.c | 174 +++++++++++++++++++++++++++++++---------------
1 file changed, 118 insertions(+), 56 deletions(-)
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 492b2421ab..7b4f3fb81a 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -56,17 +56,42 @@ 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.
+ */
+typedef struct {
+ bool valid;
+ int cpu_index;
+} GicCPU;
+
+static inline GicCPU gic_get_current_cpu(GICState *s, 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 (GicCPU) { .valid = false };
}
- return 0;
+ g_assert(attrs.requester_id < s->num_cpu);
+
+ return (GicCPU) { .valid = true, .cpu_index = attrs.requester_id };
}
-static inline int gic_get_current_vcpu(GICState *s)
+static inline GicCPU gic_get_current_vcpu(GICState *s, MemTxAttrs attrs)
{
- return gic_get_current_cpu(s) + GIC_NCPU;
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+ if (cpu.valid) {
+ cpu.cpu_index += GIC_NCPU;
+ }
+ return cpu;
}
/* Return true if this GIC config has interrupt groups, which is
@@ -941,17 +966,14 @@ static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
gic_update(s);
}
-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 == 0) { /* GICD_CTLR */
@@ -1152,19 +1174,26 @@ bad_reg:
static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
unsigned size, MemTxAttrs attrs)
{
+ GICState *s = (GICState *)opaque;
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+
+ if (!cpu.valid) {
+ return MEMTX_ACCESS_ERROR;
+ }
+
switch (size) {
case 1:
- *data = gic_dist_readb(opaque, offset, attrs);
+ *data = gic_dist_readb(s, cpu.cpu_index, 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.cpu_index, offset, attrs);
+ *data |= gic_dist_readb(s, cpu.cpu_index, 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.cpu_index, offset, attrs);
+ *data |= gic_dist_readb(s, cpu.cpu_index, offset + 1, attrs) << 8;
+ *data |= gic_dist_readb(s, cpu.cpu_index, offset + 2, attrs) << 16;
+ *data |= gic_dist_readb(s, cpu.cpu_index, offset + 3, attrs) << 24;
break;
default:
return MEMTX_ERROR;
@@ -1174,15 +1203,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) {
@@ -1459,24 +1485,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:
@@ -1503,24 +1526,31 @@ 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;
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+
+ if (!cpu.valid) {
+ return MEMTX_ACCESS_ERROR;
+ }
+
trace_gic_dist_write(offset, size, data);
switch (size) {
case 1:
- gic_dist_writeb(opaque, offset, data, attrs);
+ gic_dist_writeb(s, cpu.cpu_index, offset, data, attrs);
return MEMTX_OK;
case 2:
- gic_dist_writew(opaque, offset, data, attrs);
+ gic_dist_writew(s, cpu.cpu_index, offset, data, attrs);
return MEMTX_OK;
case 4:
- gic_dist_writel(opaque, offset, data, attrs);
+ gic_dist_writel(s, cpu.cpu_index, offset, data, attrs);
return MEMTX_OK;
default:
return MEMTX_ERROR;
@@ -1780,7 +1810,12 @@ 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);
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+ if (cpu.valid) {
+ return gic_cpu_read(s, cpu.cpu_index, addr, data, attrs);
+ } else {
+ return MEMTX_ACCESS_ERROR;
+ }
}
static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
@@ -1788,7 +1823,12 @@ 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);
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+ if (cpu.valid) {
+ return gic_cpu_write(s, cpu.cpu_index, addr, value, attrs);
+ } else {
+ return MEMTX_ACCESS_ERROR;
+ }
}
/* Wrappers to read/write the GIC CPU interface for a specific CPU.
@@ -1817,8 +1857,12 @@ 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);
+ GicCPU cpu = gic_get_current_vcpu(s, attrs);
+ if (cpu.valid) {
+ return gic_cpu_read(s, cpu.cpu_index, addr, data, attrs);
+ } else {
+ return MEMTX_ACCESS_ERROR;
+ }
}
static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
@@ -1826,8 +1870,12 @@ 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);
+ GicCPU cpu = gic_get_current_vcpu(s, attrs);
+ if (cpu.valid) {
+ return gic_cpu_write(s, cpu.cpu_index, addr, value, attrs);
+ } else {
+ return MEMTX_ACCESS_ERROR;
+ }
}
static uint32_t gic_compute_eisr(GICState *s, int cpu, int lr_start)
@@ -1858,9 +1906,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;
@@ -1881,7 +1928,10 @@ static MemTxResult gic_hyp_read(void *opaque, int cpu, hwaddr addr,
uint64_t *data, MemTxAttrs attrs)
{
GICState *s = ARM_GIC(opaque);
- int vcpu = cpu + GIC_NCPU;
+ GicCPU vcpu = gic_get_current_vcpu(s, attrs);
+ if (!vcpu.valid) {
+ return MEMTX_ACCESS_ERROR;
+ }
switch (addr) {
case A_GICH_HCR: /* Hypervisor Control */
@@ -1898,11 +1948,11 @@ static MemTxResult gic_hyp_read(void *opaque, int cpu, hwaddr addr,
case A_GICH_VMCR: /* Virtual Machine Control */
*data = FIELD_DP32(0, GICH_VMCR, VMCCtlr,
- extract32(s->cpu_ctlr[vcpu], 0, 10));
- *data = FIELD_DP32(*data, GICH_VMCR, VMABP, s->abpr[vcpu]);
- *data = FIELD_DP32(*data, GICH_VMCR, VMBP, s->bpr[vcpu]);
+ extract32(s->cpu_ctlr[vcpu.cpu_index], 0, 10));
+ *data = FIELD_DP32(*data, GICH_VMCR, VMABP, s->abpr[vcpu.cpu_index]);
+ *data = FIELD_DP32(*data, GICH_VMCR, VMBP, s->bpr[vcpu.cpu_index]);
*data = FIELD_DP32(*data, GICH_VMCR, VMPriMask,
- extract32(s->priority_mask[vcpu], 3, 5));
+ extract32(s->priority_mask[vcpu.cpu_index], 3, 5));
break;
case A_GICH_MISR: /* Maintenance Interrupt Status */
@@ -1949,7 +1999,10 @@ static MemTxResult gic_hyp_write(void *opaque, int cpu, hwaddr addr,
uint64_t value, MemTxAttrs attrs)
{
GICState *s = ARM_GIC(opaque);
- int vcpu = cpu + GIC_NCPU;
+ GicCPU vcpu = gic_get_current_vcpu(s, attrs);
+ if (!vcpu.valid) {
+ return MEMTX_ACCESS_ERROR;
+ }
trace_gic_hyp_write(addr, value);
@@ -1959,12 +2012,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.cpu_index, 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.cpu_index] =
+ gic_get_prio_from_apr_bits(s, vcpu.cpu_index);
break;
case A_GICH_LR0 ... A_GICH_LR63: /* List Registers */
@@ -1991,20 +2045,28 @@ 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);
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+ if (cpu.valid) {
+ return gic_hyp_read(s, cpu.cpu_index, addr, data, attrs);
+ } else {
+ return MEMTX_ACCESS_ERROR;
+ }
}
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);
+ GicCPU cpu = gic_get_current_cpu(s, attrs);
+ if (cpu.valid) {
+ return gic_hyp_write(s, cpu.cpu_index, addr, value, attrs);
+ } else {
+ return MEMTX_ACCESS_ERROR;
+ }
}
static MemTxResult gic_do_hyp_read(void *opaque, hwaddr addr, uint64_t *data,
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (7 preceding siblings ...)
2022-09-27 14:14 ` [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-28 17:04 ` Richard Henderson
2022-10-19 10:46 ` Philippe Mathieu-Daudé
2022-09-27 14:14 ` [PATCH v3 10/15] configure: move detected gdb to TCG's config-host.mak Alex Bennée
` (5 subsequent siblings)
14 siblings, 2 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée, Richard Henderson, Peter Maydell
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
---
hw/timer/arm_mptimer.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index cdfca3000b..34693a2534 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -41,9 +41,10 @@
* which is used in both the ARM11MPCore and Cortex-A9MP.
*/
-static inline int get_current_cpu(ARMMPTimerState *s)
+static inline int get_current_cpu(ARMMPTimerState *s, MemTxAttrs attrs)
{
- int cpu_id = current_cpu ? current_cpu->cpu_index : 0;
+ int cpu_id = attrs.requester_id;
+ g_assert(attrs.requester_type == MTRT_CPU);
if (cpu_id >= s->num_cpu) {
hw_error("arm_mptimer: num-cpu %d but this cpu is %d!\n",
@@ -178,25 +179,27 @@ static void timerblock_write(void *opaque, hwaddr addr,
/* Wrapper functions to implement the "read timer/watchdog for
* the current CPU" memory regions.
*/
-static uint64_t arm_thistimer_read(void *opaque, hwaddr addr,
- unsigned size)
+static MemTxResult arm_thistimer_read(void *opaque, hwaddr addr, uint64_t *data,
+ unsigned size, MemTxAttrs attrs)
{
ARMMPTimerState *s = (ARMMPTimerState *)opaque;
- int id = get_current_cpu(s);
- return timerblock_read(&s->timerblock[id], addr, size);
+ int id = get_current_cpu(s, attrs);
+ *data = timerblock_read(&s->timerblock[id], addr, size);
+ return MEMTX_OK;
}
-static void arm_thistimer_write(void *opaque, hwaddr addr,
- uint64_t value, unsigned size)
+static MemTxResult arm_thistimer_write(void *opaque, hwaddr addr,
+ uint64_t value, unsigned size, MemTxAttrs attrs)
{
ARMMPTimerState *s = (ARMMPTimerState *)opaque;
- int id = get_current_cpu(s);
+ int id = get_current_cpu(s, attrs);
timerblock_write(&s->timerblock[id], addr, value, size);
+ return MEMTX_OK;
}
static const MemoryRegionOps arm_thistimer_ops = {
- .read = arm_thistimer_read,
- .write = arm_thistimer_write,
+ .read_with_attrs = arm_thistimer_read,
+ .write_with_attrs = arm_thistimer_write,
.valid = {
.min_access_size = 4,
.max_access_size = 4,
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 10/15] configure: move detected gdb to TCG's config-host.mak
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (8 preceding siblings ...)
2022-09-27 14:14 ` [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
@ 2022-09-27 14:14 ` Alex Bennée
2022-09-27 14:15 ` [PATCH v3 11/15] gdbstub: move into its own sub directory Alex Bennée
` (4 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:14 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Alex Bennée, Richard Henderson
When tests/tcg gained it's own config-host.mak we forgot to move the
GDB detection.
Fixes: 544f4a2578 (tests/tcg: isolate from QEMU's config-host.mak)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
configure | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/configure b/configure
index cc4ecd6008..f6a36a5361 100755
--- a/configure
+++ b/configure
@@ -2475,6 +2475,8 @@ if test -n "$gdb_bin"; then
gdb_version=$($gdb_bin --version | head -n 1)
if version_ge ${gdb_version##* } 9.1; then
echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
+ else
+ gdb_bin=""
fi
fi
@@ -2559,6 +2561,11 @@ echo "# Automatically generated by configure - do not modify" > $config_host_mak
echo "SRC_PATH=$source_path" >> $config_host_mak
echo "HOST_CC=$host_cc" >> $config_host_mak
+# versioned checked in the main config_host.mak above
+if test -n "$gdb_bin"; then
+ echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
+fi
+
tcg_tests_targets=
for target in $target_list; do
arch=${target%%-*}
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 11/15] gdbstub: move into its own sub directory
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (9 preceding siblings ...)
2022-09-27 14:14 ` [PATCH v3 10/15] configure: move detected gdb to TCG's config-host.mak Alex Bennée
@ 2022-09-27 14:15 ` Alex Bennée
2022-10-19 10:47 ` Philippe Mathieu-Daudé
2022-09-27 14:15 ` [PATCH v3 12/15] gdbstub: move sstep flags probing into AccelClass Alex Bennée
` (3 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Richard Henderson,
Philippe Mathieu-Daudé,
Stefan Hajnoczi
This is in preparation of future refactoring as well as cleaning up
the source tree. Aside from the minor tweaks to meson and trace.h this
is pure code motion.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
meson.build | 4 +++-
gdbstub/trace.h | 1 +
gdbstub.c => gdbstub/gdbstub.c | 2 +-
MAINTAINERS | 2 +-
gdbstub/meson.build | 1 +
gdbstub/trace-events | 29 +++++++++++++++++++++++++++++
trace-events | 28 ----------------------------
7 files changed, 36 insertions(+), 31 deletions(-)
create mode 100644 gdbstub/trace.h
rename gdbstub.c => gdbstub/gdbstub.c (99%)
create mode 100644 gdbstub/meson.build
create mode 100644 gdbstub/trace-events
diff --git a/meson.build b/meson.build
index 3885fc1076..2c9209c2b8 100644
--- a/meson.build
+++ b/meson.build
@@ -2914,6 +2914,7 @@ trace_events_subdirs = [
'qom',
'monitor',
'util',
+ 'gdbstub',
]
if have_linux_user
trace_events_subdirs += [ 'linux-user' ]
@@ -3037,6 +3038,7 @@ subdir('authz')
subdir('crypto')
subdir('ui')
subdir('hw')
+subdir('gdbstub')
if enable_modules
@@ -3114,7 +3116,7 @@ common_ss.add(files('cpus-common.c'))
subdir('softmmu')
common_ss.add(capstone)
-specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone)
+specific_ss.add(files('cpu.c', 'disas.c'), capstone)
# Work around a gcc bug/misfeature wherein constant propagation looks
# through an alias:
diff --git a/gdbstub/trace.h b/gdbstub/trace.h
new file mode 100644
index 0000000000..dee87b1238
--- /dev/null
+++ b/gdbstub/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-gdbstub.h"
diff --git a/gdbstub.c b/gdbstub/gdbstub.c
similarity index 99%
rename from gdbstub.c
rename to gdbstub/gdbstub.c
index cf869b10e3..7d8fe475b3 100644
--- a/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -29,7 +29,7 @@
#include "qemu/ctype.h"
#include "qemu/cutils.h"
#include "qemu/module.h"
-#include "trace/trace-root.h"
+#include "trace.h"
#include "exec/gdbstub.h"
#ifdef CONFIG_USER_ONLY
#include "qemu.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index 738c4eb647..82575b2486 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2670,7 +2670,7 @@ GDB stub
M: Alex Bennée <alex.bennee@linaro.org>
R: Philippe Mathieu-Daudé <f4bug@amsat.org>
S: Maintained
-F: gdbstub*
+F: gdbstub/*
F: include/exec/gdbstub.h
F: gdb-xml/
F: tests/tcg/multiarch/gdbstub/
diff --git a/gdbstub/meson.build b/gdbstub/meson.build
new file mode 100644
index 0000000000..6d4ae2d03c
--- /dev/null
+++ b/gdbstub/meson.build
@@ -0,0 +1 @@
+specific_ss.add(files('gdbstub.c'))
diff --git a/gdbstub/trace-events b/gdbstub/trace-events
new file mode 100644
index 0000000000..03f0c303bf
--- /dev/null
+++ b/gdbstub/trace-events
@@ -0,0 +1,29 @@
+# See docs/devel/tracing.rst for syntax documentation.
+
+# gdbstub.c
+gdbstub_op_start(const char *device) "Starting gdbstub using device %s"
+gdbstub_op_exiting(uint8_t code) "notifying exit with code=0x%02x"
+gdbstub_op_continue(void) "Continuing all CPUs"
+gdbstub_op_continue_cpu(int cpu_index) "Continuing CPU %d"
+gdbstub_op_stepping(int cpu_index) "Stepping CPU %d"
+gdbstub_op_extra_info(const char *info) "Thread extra info: %s"
+gdbstub_hit_watchpoint(const char *type, int cpu_gdb_index, uint64_t vaddr) "Watchpoint hit, type=\"%s\" cpu=%d, vaddr=0x%" PRIx64 ""
+gdbstub_hit_internal_error(void) "RUN_STATE_INTERNAL_ERROR"
+gdbstub_hit_break(void) "RUN_STATE_DEBUG"
+gdbstub_hit_paused(void) "RUN_STATE_PAUSED"
+gdbstub_hit_shutdown(void) "RUN_STATE_SHUTDOWN"
+gdbstub_hit_io_error(void) "RUN_STATE_IO_ERROR"
+gdbstub_hit_watchdog(void) "RUN_STATE_WATCHDOG"
+gdbstub_hit_unknown(int state) "Unknown run state=0x%x"
+gdbstub_io_reply(const char *message) "Sent: %s"
+gdbstub_io_binaryreply(size_t ofs, const char *line) "0x%04zx: %s"
+gdbstub_io_command(const char *command) "Received: %s"
+gdbstub_io_got_ack(void) "Got ACK"
+gdbstub_io_got_unexpected(uint8_t ch) "Got 0x%02x when expecting ACK/NACK"
+gdbstub_err_got_nack(void) "Got NACK, retransmitting"
+gdbstub_err_garbage(uint8_t ch) "received garbage between packets: 0x%02x"
+gdbstub_err_overrun(void) "command buffer overrun, dropping command"
+gdbstub_err_invalid_repeat(uint8_t ch) "got invalid RLE count: 0x%02x"
+gdbstub_err_invalid_rle(void) "got invalid RLE sequence"
+gdbstub_err_checksum_invalid(uint8_t ch) "got invalid command checksum digit: 0x%02x"
+gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packet with incorrect checksum, expected=0x%02x, received=0x%02x"
diff --git a/trace-events b/trace-events
index bc71006675..035f3d570d 100644
--- a/trace-events
+++ b/trace-events
@@ -46,34 +46,6 @@ ram_block_discard_range(const char *rbname, void *hva, size_t length, bool need_
memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u"
memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
-# gdbstub.c
-gdbstub_op_start(const char *device) "Starting gdbstub using device %s"
-gdbstub_op_exiting(uint8_t code) "notifying exit with code=0x%02x"
-gdbstub_op_continue(void) "Continuing all CPUs"
-gdbstub_op_continue_cpu(int cpu_index) "Continuing CPU %d"
-gdbstub_op_stepping(int cpu_index) "Stepping CPU %d"
-gdbstub_op_extra_info(const char *info) "Thread extra info: %s"
-gdbstub_hit_watchpoint(const char *type, int cpu_gdb_index, uint64_t vaddr) "Watchpoint hit, type=\"%s\" cpu=%d, vaddr=0x%" PRIx64 ""
-gdbstub_hit_internal_error(void) "RUN_STATE_INTERNAL_ERROR"
-gdbstub_hit_break(void) "RUN_STATE_DEBUG"
-gdbstub_hit_paused(void) "RUN_STATE_PAUSED"
-gdbstub_hit_shutdown(void) "RUN_STATE_SHUTDOWN"
-gdbstub_hit_io_error(void) "RUN_STATE_IO_ERROR"
-gdbstub_hit_watchdog(void) "RUN_STATE_WATCHDOG"
-gdbstub_hit_unknown(int state) "Unknown run state=0x%x"
-gdbstub_io_reply(const char *message) "Sent: %s"
-gdbstub_io_binaryreply(size_t ofs, const char *line) "0x%04zx: %s"
-gdbstub_io_command(const char *command) "Received: %s"
-gdbstub_io_got_ack(void) "Got ACK"
-gdbstub_io_got_unexpected(uint8_t ch) "Got 0x%02x when expecting ACK/NACK"
-gdbstub_err_got_nack(void) "Got NACK, retransmitting"
-gdbstub_err_garbage(uint8_t ch) "received garbage between packets: 0x%02x"
-gdbstub_err_overrun(void) "command buffer overrun, dropping command"
-gdbstub_err_invalid_repeat(uint8_t ch) "got invalid RLE count: 0x%02x"
-gdbstub_err_invalid_rle(void) "got invalid RLE sequence"
-gdbstub_err_checksum_invalid(uint8_t ch) "got invalid command checksum digit: 0x%02x"
-gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packet with incorrect checksum, expected=0x%02x, received=0x%02x"
-
# job.c
job_state_transition(void *job, int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)"
job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 12/15] gdbstub: move sstep flags probing into AccelClass
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (10 preceding siblings ...)
2022-09-27 14:15 ` [PATCH v3 11/15] gdbstub: move into its own sub directory Alex Bennée
@ 2022-09-27 14:15 ` Alex Bennée
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:15 ` [PATCH v3 13/15] gdbstub: move breakpoint logic to accel ops Alex Bennée
` (2 subsequent siblings)
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Richard Henderson,
Philippe Mathieu-Daudé,
Mads Ynddal, Paolo Bonzini, open list:Overall KVM CPUs
The support of single-stepping is very much dependent on support from
the accelerator we are using. To avoid special casing in gdbstub move
the probing out to an AccelClass function so future accelerators can
put their code there.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mads Ynddal <mads@ynddal.dk>
---
include/qemu/accel.h | 12 ++++++++++++
include/sysemu/kvm.h | 8 --------
accel/accel-common.c | 10 ++++++++++
accel/kvm/kvm-all.c | 14 +++++++++++++-
accel/tcg/tcg-all.c | 17 +++++++++++++++++
gdbstub/gdbstub.c | 22 ++++------------------
6 files changed, 56 insertions(+), 27 deletions(-)
diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index be56da1b99..ce4747634a 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -43,6 +43,10 @@ typedef struct AccelClass {
bool (*has_memory)(MachineState *ms, AddressSpace *as,
hwaddr start_addr, hwaddr size);
#endif
+
+ /* gdbstub related hooks */
+ int (*gdbstub_supported_sstep_flags)(void);
+
bool *allowed;
/*
* Array of global properties that would be applied when specific
@@ -92,4 +96,12 @@ void accel_cpu_instance_init(CPUState *cpu);
*/
bool accel_cpu_realizefn(CPUState *cpu, Error **errp);
+/**
+ * accel_supported_gdbstub_sstep_flags:
+ *
+ * Returns the supported single step modes for the configured
+ * accelerator.
+ */
+int accel_supported_gdbstub_sstep_flags(void);
+
#endif /* QEMU_ACCEL_H */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index efd6dee818..a20ad51aad 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -47,7 +47,6 @@ extern bool kvm_direct_msi_allowed;
extern bool kvm_ioeventfd_any_length_allowed;
extern bool kvm_msi_use_devid;
extern bool kvm_has_guest_debug;
-extern int kvm_sstep_flags;
#define kvm_enabled() (kvm_allowed)
/**
@@ -174,12 +173,6 @@ extern int kvm_sstep_flags;
*/
#define kvm_supports_guest_debug() (kvm_has_guest_debug)
-/*
- * kvm_supported_sstep_flags
- * Returns: SSTEP_* flags that KVM supports for guest debug
- */
-#define kvm_get_supported_sstep_flags() (kvm_sstep_flags)
-
#else
#define kvm_enabled() (0)
@@ -198,7 +191,6 @@ extern int kvm_sstep_flags;
#define kvm_ioeventfd_any_length_enabled() (false)
#define kvm_msi_devid_required() (false)
#define kvm_supports_guest_debug() (false)
-#define kvm_get_supported_sstep_flags() (0)
#endif /* CONFIG_KVM_IS_POSSIBLE */
diff --git a/accel/accel-common.c b/accel/accel-common.c
index 50035bda55..df72cc989a 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -129,6 +129,16 @@ bool accel_cpu_realizefn(CPUState *cpu, Error **errp)
return true;
}
+int accel_supported_gdbstub_sstep_flags(void)
+{
+ AccelState *accel = current_accel();
+ AccelClass *acc = ACCEL_GET_CLASS(accel);
+ if (acc->gdbstub_supported_sstep_flags) {
+ return acc->gdbstub_supported_sstep_flags();
+ }
+ return 0;
+}
+
static const TypeInfo accel_cpu_type = {
.name = TYPE_ACCEL_CPU,
.parent = TYPE_OBJECT,
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 5acab1767f..c55938453a 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -175,7 +175,7 @@ bool kvm_direct_msi_allowed;
bool kvm_ioeventfd_any_length_allowed;
bool kvm_msi_use_devid;
bool kvm_has_guest_debug;
-int kvm_sstep_flags;
+static int kvm_sstep_flags;
static bool kvm_immediate_exit;
static hwaddr kvm_max_slot_size = ~0;
@@ -3712,6 +3712,17 @@ static void kvm_accel_instance_init(Object *obj)
s->kvm_dirty_ring_size = 0;
}
+/**
+ * kvm_gdbstub_sstep_flags():
+ *
+ * Returns: SSTEP_* flags that KVM supports for guest debug. The
+ * support is probed during kvm_init()
+ */
+static int kvm_gdbstub_sstep_flags(void)
+{
+ return kvm_sstep_flags;
+}
+
static void kvm_accel_class_init(ObjectClass *oc, void *data)
{
AccelClass *ac = ACCEL_CLASS(oc);
@@ -3719,6 +3730,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
ac->init_machine = kvm_init;
ac->has_memory = kvm_accel_has_memory;
ac->allowed = &kvm_allowed;
+ ac->gdbstub_supported_sstep_flags = kvm_gdbstub_sstep_flags;
object_class_property_add(oc, "kernel-irqchip", "on|off|split",
NULL, kvm_set_kernel_irqchip,
diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 47952eecd7..30b503fb22 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "sysemu/tcg.h"
+#include "sysemu/replay.h"
#include "sysemu/cpu-timers.h"
#include "tcg/tcg.h"
#include "qapi/error.h"
@@ -207,12 +208,28 @@ static void tcg_set_splitwx(Object *obj, bool value, Error **errp)
s->splitwx_enabled = value;
}
+static int tcg_gdbstub_supported_sstep_flags(void)
+{
+ /*
+ * In replay mode all events will come from the log and can't be
+ * suppressed otherwise we would break determinism. However as those
+ * events are tied to the number of executed instructions we won't see
+ * them occurring every time we single step.
+ */
+ if (replay_mode != REPLAY_MODE_NONE) {
+ return SSTEP_ENABLE;
+ } else {
+ return SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
+ }
+}
+
static void tcg_accel_class_init(ObjectClass *oc, void *data)
{
AccelClass *ac = ACCEL_CLASS(oc);
ac->name = "tcg";
ac->init_machine = tcg_init_machine;
ac->allowed = &tcg_allowed;
+ ac->gdbstub_supported_sstep_flags = tcg_gdbstub_supported_sstep_flags;
object_class_property_add_str(oc, "thread",
tcg_get_thread,
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 7d8fe475b3..a0755e6505 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -383,27 +383,13 @@ static void init_gdbserver_state(void)
gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 4);
/*
- * In replay mode all events will come from the log and can't be
- * suppressed otherwise we would break determinism. However as those
- * events are tied to the number of executed instructions we won't see
- * them occurring every time we single step.
- */
- if (replay_mode != REPLAY_MODE_NONE) {
- gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
- } else if (kvm_enabled()) {
- gdbserver_state.supported_sstep_flags = kvm_get_supported_sstep_flags();
- } else {
- gdbserver_state.supported_sstep_flags =
- SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
- }
-
- /*
- * By default use no IRQs and no timers while single stepping so as to
- * make single stepping like an ICE HW step.
+ * What single-step modes are supported is accelerator dependent.
+ * By default try to use no IRQs and no timers while single
+ * stepping so as to make single stepping like a typical ICE HW step.
*/
+ gdbserver_state.supported_sstep_flags = accel_supported_gdbstub_sstep_flags();
gdbserver_state.sstep_flags = SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
gdbserver_state.sstep_flags &= gdbserver_state.supported_sstep_flags;
-
}
#ifndef CONFIG_USER_ONLY
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 13/15] gdbstub: move breakpoint logic to accel ops
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (11 preceding siblings ...)
2022-09-27 14:15 ` [PATCH v3 12/15] gdbstub: move sstep flags probing into AccelClass Alex Bennée
@ 2022-09-27 14:15 ` Alex Bennée
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:15 ` [PATCH v3 14/15] gdbstub: move guest debug support check to ops Alex Bennée
2022-09-27 14:15 ` Alex Bennée
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Richard Henderson, Mads Ynddal,
Paolo Bonzini, Philippe Mathieu-Daudé,
open list:Overall KVM CPUs
As HW virtualization requires specific support to handle breakpoints
lets push out special casing out of the core gdbstub code and into
AccelOpsClass. This will make it easier to add other accelerator
support and reduces some of the stub shenanigans.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mads Ynddal <mads@ynddal.dk>
---
accel/kvm/kvm-cpus.h | 3 +
gdbstub/internals.h | 16 +++++
include/sysemu/accel-ops.h | 6 ++
include/sysemu/cpus.h | 3 +
include/sysemu/kvm.h | 5 --
accel/kvm/kvm-accel-ops.c | 8 +++
accel/kvm/kvm-all.c | 24 +------
accel/stubs/kvm-stub.c | 16 -----
accel/tcg/tcg-accel-ops.c | 92 +++++++++++++++++++++++++++
gdbstub/gdbstub.c | 127 +++----------------------------------
gdbstub/softmmu.c | 42 ++++++++++++
gdbstub/user.c | 62 ++++++++++++++++++
softmmu/cpus.c | 7 ++
gdbstub/meson.build | 8 +++
14 files changed, 259 insertions(+), 160 deletions(-)
create mode 100644 gdbstub/internals.h
create mode 100644 gdbstub/softmmu.c
create mode 100644 gdbstub/user.c
diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
index bf0bd1bee4..33e435d62b 100644
--- a/accel/kvm/kvm-cpus.h
+++ b/accel/kvm/kvm-cpus.h
@@ -18,5 +18,8 @@ void kvm_destroy_vcpu(CPUState *cpu);
void kvm_cpu_synchronize_post_reset(CPUState *cpu);
void kvm_cpu_synchronize_post_init(CPUState *cpu);
void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
+int kvm_insert_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len);
+int kvm_remove_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len);
+void kvm_remove_all_breakpoints(CPUState *cpu);
#endif /* KVM_CPUS_H */
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
new file mode 100644
index 0000000000..41e2e72dbf
--- /dev/null
+++ b/gdbstub/internals.h
@@ -0,0 +1,16 @@
+/*
+ * gdbstub internals
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _INTERNALS_H_
+#define _INTERNALS_H_
+
+int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len);
+int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len);
+void gdb_breakpoint_remove_all(CPUState *cs);
+
+#endif /* _INTERNALS_H_ */
diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index a0572ea87a..86794ac273 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -10,6 +10,7 @@
#ifndef ACCEL_OPS_H
#define ACCEL_OPS_H
+#include "exec/hwaddr.h"
#include "qom/object.h"
#define ACCEL_OPS_SUFFIX "-ops"
@@ -44,6 +45,11 @@ struct AccelOpsClass {
int64_t (*get_virtual_clock)(void);
int64_t (*get_elapsed_ticks)(void);
+
+ /* gdbstub hooks */
+ int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len);
+ int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len);
+ void (*remove_all_breakpoints)(CPUState *cpu);
};
#endif /* ACCEL_OPS_H */
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index b5c87d48b3..1bace3379b 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -7,6 +7,9 @@
/* register accel-specific operations */
void cpus_register_accel(const AccelOpsClass *i);
+/* return registers ops */
+const AccelOpsClass *cpus_get_accel(void);
+
/* accel/dummy-cpus.c */
/* Create a dummy vcpu for AccelOpsClass->create_vcpu_thread */
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a20ad51aad..21d3f1d01e 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -254,11 +254,6 @@ int kvm_on_sigbus(int code, void *addr);
void kvm_flush_coalesced_mmio_buffer(void);
-int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type);
-int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type);
-void kvm_remove_all_breakpoints(CPUState *cpu);
int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
/* internal API */
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index c4244a23c6..5c0e37514c 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -16,12 +16,14 @@
#include "qemu/osdep.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
+#include "sysemu/kvm.h"
#include "sysemu/kvm_int.h"
#include "sysemu/runstate.h"
#include "sysemu/cpus.h"
#include "qemu/guest-random.h"
#include "qapi/error.h"
+#include <linux/kvm.h>
#include "kvm-cpus.h"
static void *kvm_vcpu_thread_fn(void *arg)
@@ -95,6 +97,12 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data)
ops->synchronize_post_init = kvm_cpu_synchronize_post_init;
ops->synchronize_state = kvm_cpu_synchronize_state;
ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm;
+
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+ ops->insert_breakpoint = kvm_insert_breakpoint;
+ ops->remove_breakpoint = kvm_remove_breakpoint;
+ ops->remove_all_breakpoints = kvm_remove_all_breakpoints;
+#endif
}
static const TypeInfo kvm_accel_ops_type = {
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c55938453a..b8c734fe3a 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3287,8 +3287,7 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
return data.err;
}
-int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type)
+int kvm_insert_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len)
{
struct kvm_sw_breakpoint *bp;
int err;
@@ -3326,8 +3325,7 @@ int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
return 0;
}
-int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type)
+int kvm_remove_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len)
{
struct kvm_sw_breakpoint *bp;
int err;
@@ -3393,26 +3391,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
#else /* !KVM_CAP_SET_GUEST_DEBUG */
-int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
+static int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
{
return -EINVAL;
}
-
-int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type)
-{
- return -EINVAL;
-}
-
-int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type)
-{
- return -EINVAL;
-}
-
-void kvm_remove_all_breakpoints(CPUState *cpu)
-{
-}
#endif /* !KVM_CAP_SET_GUEST_DEBUG */
static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 2ac5f9c036..2d79333143 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -51,22 +51,6 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
return -ENOSYS;
}
-int kvm_insert_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type)
-{
- return -EINVAL;
-}
-
-int kvm_remove_breakpoint(CPUState *cpu, target_ulong addr,
- target_ulong len, int type)
-{
- return -EINVAL;
-}
-
-void kvm_remove_all_breakpoints(CPUState *cpu)
-{
-}
-
int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
{
return 1;
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 786d90c08f..965c2ad581 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -32,6 +32,8 @@
#include "qemu/main-loop.h"
#include "qemu/guest-random.h"
#include "exec/exec-all.h"
+#include "exec/hwaddr.h"
+#include "exec/gdbstub.h"
#include "tcg-accel-ops.h"
#include "tcg-accel-ops-mttcg.h"
@@ -91,6 +93,92 @@ void tcg_handle_interrupt(CPUState *cpu, int mask)
}
}
+/* Translate GDB watchpoint type to a flags value for cpu_watchpoint_* */
+static inline int xlat_gdb_type(CPUState *cpu, int gdbtype)
+{
+ static const int xlat[] = {
+ [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
+ [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
+ [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
+ };
+
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+ int cputype = xlat[gdbtype];
+
+ if (cc->gdb_stop_before_watchpoint) {
+ cputype |= BP_STOP_BEFORE_ACCESS;
+ }
+ return cputype;
+}
+
+static int tcg_insert_breakpoint(CPUState *cs, int type, hwaddr addr, hwaddr len)
+{
+ CPUState *cpu;
+ int err = 0;
+
+ switch (type) {
+ case GDB_BREAKPOINT_SW:
+ case GDB_BREAKPOINT_HW:
+ CPU_FOREACH(cpu) {
+ err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
+ if (err) {
+ break;
+ }
+ }
+ return err;
+ case GDB_WATCHPOINT_WRITE:
+ case GDB_WATCHPOINT_READ:
+ case GDB_WATCHPOINT_ACCESS:
+ CPU_FOREACH(cpu) {
+ err = cpu_watchpoint_insert(cpu, addr, len,
+ xlat_gdb_type(cpu, type), NULL);
+ if (err) {
+ break;
+ }
+ }
+ return err;
+ default:
+ return -ENOSYS;
+ }
+}
+
+static int tcg_remove_breakpoint(CPUState *cs, int type, hwaddr addr, hwaddr len)
+{
+ CPUState *cpu;
+ int err = 0;
+
+ switch (type) {
+ case GDB_BREAKPOINT_SW:
+ case GDB_BREAKPOINT_HW:
+ CPU_FOREACH(cpu) {
+ err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
+ if (err) {
+ break;
+ }
+ }
+ return err;
+ case GDB_WATCHPOINT_WRITE:
+ case GDB_WATCHPOINT_READ:
+ case GDB_WATCHPOINT_ACCESS:
+ CPU_FOREACH(cpu) {
+ err = cpu_watchpoint_remove(cpu, addr, len,
+ xlat_gdb_type(cpu, type));
+ if (err) {
+ break;
+ }
+ }
+ return err;
+ default:
+ return -ENOSYS;
+ }
+}
+
+static inline void tcg_remove_all_breakpoints(CPUState *cpu)
+{
+ cpu_breakpoint_remove_all(cpu, BP_GDB);
+ cpu_watchpoint_remove_all(cpu, BP_GDB);
+}
+
static void tcg_accel_ops_init(AccelOpsClass *ops)
{
if (qemu_tcg_mttcg_enabled()) {
@@ -109,6 +197,10 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
ops->handle_interrupt = tcg_handle_interrupt;
}
}
+
+ ops->insert_breakpoint = tcg_insert_breakpoint;
+ ops->remove_breakpoint = tcg_remove_breakpoint;
+ ops->remove_all_breakpoints = tcg_remove_all_breakpoints;
}
static void tcg_accel_ops_class_init(ObjectClass *oc, void *data)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index a0755e6505..ff9f3f9586 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -49,8 +49,11 @@
#include "sysemu/runstate.h"
#include "semihosting/semihost.h"
#include "exec/exec-all.h"
+#include "exec/hwaddr.h"
#include "sysemu/replay.h"
+#include "internals.h"
+
#ifdef CONFIG_USER_ONLY
#define GDB_ATTACHED "0"
#else
@@ -1012,130 +1015,16 @@ void gdb_register_coprocessor(CPUState *cpu,
}
}
-#ifndef CONFIG_USER_ONLY
-/* Translate GDB watchpoint type to a flags value for cpu_watchpoint_* */
-static inline int xlat_gdb_type(CPUState *cpu, int gdbtype)
-{
- static const int xlat[] = {
- [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
- [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
- [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
- };
-
- CPUClass *cc = CPU_GET_CLASS(cpu);
- int cputype = xlat[gdbtype];
-
- if (cc->gdb_stop_before_watchpoint) {
- cputype |= BP_STOP_BEFORE_ACCESS;
- }
- return cputype;
-}
-#endif
-
-static int gdb_breakpoint_insert(int type, target_ulong addr, target_ulong len)
-{
- CPUState *cpu;
- int err = 0;
-
- if (kvm_enabled()) {
- return kvm_insert_breakpoint(gdbserver_state.c_cpu, addr, len, type);
- }
-
- switch (type) {
- case GDB_BREAKPOINT_SW:
- case GDB_BREAKPOINT_HW:
- CPU_FOREACH(cpu) {
- err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
- if (err) {
- break;
- }
- }
- return err;
-#ifndef CONFIG_USER_ONLY
- case GDB_WATCHPOINT_WRITE:
- case GDB_WATCHPOINT_READ:
- case GDB_WATCHPOINT_ACCESS:
- CPU_FOREACH(cpu) {
- err = cpu_watchpoint_insert(cpu, addr, len,
- xlat_gdb_type(cpu, type), NULL);
- if (err) {
- break;
- }
- }
- return err;
-#endif
- default:
- return -ENOSYS;
- }
-}
-
-static int gdb_breakpoint_remove(int type, target_ulong addr, target_ulong len)
-{
- CPUState *cpu;
- int err = 0;
-
- if (kvm_enabled()) {
- return kvm_remove_breakpoint(gdbserver_state.c_cpu, addr, len, type);
- }
-
- switch (type) {
- case GDB_BREAKPOINT_SW:
- case GDB_BREAKPOINT_HW:
- CPU_FOREACH(cpu) {
- err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
- if (err) {
- break;
- }
- }
- return err;
-#ifndef CONFIG_USER_ONLY
- case GDB_WATCHPOINT_WRITE:
- case GDB_WATCHPOINT_READ:
- case GDB_WATCHPOINT_ACCESS:
- CPU_FOREACH(cpu) {
- err = cpu_watchpoint_remove(cpu, addr, len,
- xlat_gdb_type(cpu, type));
- if (err)
- break;
- }
- return err;
-#endif
- default:
- return -ENOSYS;
- }
-}
-
-static inline void gdb_cpu_breakpoint_remove_all(CPUState *cpu)
-{
- cpu_breakpoint_remove_all(cpu, BP_GDB);
-#ifndef CONFIG_USER_ONLY
- cpu_watchpoint_remove_all(cpu, BP_GDB);
-#endif
-}
-
static void gdb_process_breakpoint_remove_all(GDBProcess *p)
{
CPUState *cpu = get_first_cpu_in_process(p);
while (cpu) {
- gdb_cpu_breakpoint_remove_all(cpu);
+ gdb_breakpoint_remove_all(cpu);
cpu = gdb_next_cpu_in_process(cpu);
}
}
-static void gdb_breakpoint_remove_all(void)
-{
- CPUState *cpu;
-
- if (kvm_enabled()) {
- kvm_remove_all_breakpoints(gdbserver_state.c_cpu);
- return;
- }
-
- CPU_FOREACH(cpu) {
- gdb_cpu_breakpoint_remove_all(cpu);
- }
-}
static void gdb_set_cpu_pc(target_ulong pc)
{
@@ -1667,7 +1556,8 @@ static void handle_insert_bp(GArray *params, void *user_ctx)
return;
}
- res = gdb_breakpoint_insert(get_param(params, 0)->val_ul,
+ res = gdb_breakpoint_insert(gdbserver_state.c_cpu,
+ get_param(params, 0)->val_ul,
get_param(params, 1)->val_ull,
get_param(params, 2)->val_ull);
if (res >= 0) {
@@ -1690,7 +1580,8 @@ static void handle_remove_bp(GArray *params, void *user_ctx)
return;
}
- res = gdb_breakpoint_remove(get_param(params, 0)->val_ul,
+ res = gdb_breakpoint_remove(gdbserver_state.c_cpu,
+ get_param(params, 0)->val_ul,
get_param(params, 1)->val_ull,
get_param(params, 2)->val_ull);
if (res >= 0) {
@@ -2541,7 +2432,7 @@ static void handle_target_halt(GArray *params, void *user_ctx)
* because gdb is doing an initial connect and the state
* should be cleaned up.
*/
- gdb_breakpoint_remove_all();
+ gdb_breakpoint_remove_all(gdbserver_state.c_cpu);
}
static int gdb_handle_packet(const char *line_buf)
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
new file mode 100644
index 0000000000..4e73890379
--- /dev/null
+++ b/gdbstub/softmmu.c
@@ -0,0 +1,42 @@
+/*
+ * gdb server stub - softmmu specific bits
+ *
+ * Debug integration depends on support from the individual
+ * accelerators so most of this involves calling the ops helpers.
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "exec/gdbstub.h"
+#include "exec/hwaddr.h"
+#include "sysemu/cpus.h"
+#include "internals.h"
+
+int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len)
+{
+ const AccelOpsClass *ops = cpus_get_accel();
+ if (ops->insert_breakpoint) {
+ return ops->insert_breakpoint(cs, type, addr, len);
+ }
+ return -ENOSYS;
+}
+
+int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len)
+{
+ const AccelOpsClass *ops = cpus_get_accel();
+ if (ops->remove_breakpoint) {
+ return ops->remove_breakpoint(cs, type, addr, len);
+ }
+ return -ENOSYS;
+}
+
+void gdb_breakpoint_remove_all(CPUState *cs)
+{
+ const AccelOpsClass *ops = cpus_get_accel();
+ if (ops->remove_all_breakpoints) {
+ ops->remove_all_breakpoints(cs);
+ }
+}
diff --git a/gdbstub/user.c b/gdbstub/user.c
new file mode 100644
index 0000000000..42652b28a7
--- /dev/null
+++ b/gdbstub/user.c
@@ -0,0 +1,62 @@
+/*
+ * gdbstub user-mode helper routines.
+ *
+ * We know for user-mode we are using TCG so we can call stuff directly.
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
+#include "exec/gdbstub.h"
+#include "hw/core/cpu.h"
+#include "internals.h"
+
+int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len)
+{
+ CPUState *cpu;
+ int err = 0;
+
+ switch (type) {
+ case GDB_BREAKPOINT_SW:
+ case GDB_BREAKPOINT_HW:
+ CPU_FOREACH(cpu) {
+ err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
+ if (err) {
+ break;
+ }
+ }
+ return err;
+ default:
+ /* user-mode doesn't support watchpoints */
+ return -ENOSYS;
+ }
+}
+
+int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len)
+{
+ CPUState *cpu;
+ int err = 0;
+
+ switch (type) {
+ case GDB_BREAKPOINT_SW:
+ case GDB_BREAKPOINT_HW:
+ CPU_FOREACH(cpu) {
+ err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
+ if (err) {
+ break;
+ }
+ }
+ return err;
+ default:
+ /* user-mode doesn't support watchpoints */
+ return -ENOSYS;
+ }
+}
+
+void gdb_breakpoint_remove_all(CPUState *cs)
+{
+ cpu_breakpoint_remove_all(cs, BP_GDB);
+}
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 23b30484b2..61b27ff59d 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -617,6 +617,13 @@ void cpus_register_accel(const AccelOpsClass *ops)
cpus_accel = ops;
}
+const AccelOpsClass *cpus_get_accel(void)
+{
+ /* broken if we call this early */
+ assert(cpus_accel);
+ return cpus_accel;
+}
+
void qemu_init_vcpu(CPUState *cpu)
{
MachineState *ms = MACHINE(qdev_get_machine());
diff --git a/gdbstub/meson.build b/gdbstub/meson.build
index 6d4ae2d03c..fc895a2c39 100644
--- a/gdbstub/meson.build
+++ b/gdbstub/meson.build
@@ -1 +1,9 @@
+#
+# The main gdbstub still relies on per-build definitions of various
+# types. The bits pushed to softmmu/user.c try to use guest agnostic
+# types such as hwaddr.
+#
+
specific_ss.add(files('gdbstub.c'))
+softmmu_ss.add(files('softmmu.c'))
+user_ss.add(files('user.c'))
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 14/15] gdbstub: move guest debug support check to ops
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
` (12 preceding siblings ...)
2022-09-27 14:15 ` [PATCH v3 13/15] gdbstub: move breakpoint logic to accel ops Alex Bennée
@ 2022-09-27 14:15 ` Alex Bennée
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:15 ` Alex Bennée
14 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Philippe Mathieu-Daudé,
Mads Ynddal, Paolo Bonzini, Richard Henderson,
open list:Overall KVM CPUs
This removes the final hard coding of kvm_enabled() in gdbstub and
moves the check to an AccelOps.
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mads Ynddal <mads@ynddal.dk>
---
accel/kvm/kvm-cpus.h | 1 +
gdbstub/internals.h | 1 +
include/sysemu/accel-ops.h | 1 +
include/sysemu/kvm.h | 7 -------
accel/kvm/kvm-accel-ops.c | 1 +
accel/kvm/kvm-all.c | 6 ++++++
accel/tcg/tcg-accel-ops.c | 6 ++++++
gdbstub/gdbstub.c | 5 ++---
gdbstub/softmmu.c | 9 +++++++++
gdbstub/user.c | 6 ++++++
10 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h
index 33e435d62b..fd63fe6a59 100644
--- a/accel/kvm/kvm-cpus.h
+++ b/accel/kvm/kvm-cpus.h
@@ -18,6 +18,7 @@ void kvm_destroy_vcpu(CPUState *cpu);
void kvm_cpu_synchronize_post_reset(CPUState *cpu);
void kvm_cpu_synchronize_post_init(CPUState *cpu);
void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
+bool kvm_supports_guest_debug(void);
int kvm_insert_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len);
int kvm_remove_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len);
void kvm_remove_all_breakpoints(CPUState *cpu);
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 41e2e72dbf..eabb0341d1 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -9,6 +9,7 @@
#ifndef _INTERNALS_H_
#define _INTERNALS_H_
+bool gdb_supports_guest_debug(void);
int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len);
int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len);
void gdb_breakpoint_remove_all(CPUState *cs);
diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index 86794ac273..8cc7996def 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -47,6 +47,7 @@ struct AccelOpsClass {
int64_t (*get_elapsed_ticks)(void);
/* gdbstub hooks */
+ bool (*supports_guest_debug)(void);
int (*insert_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len);
int (*remove_breakpoint)(CPUState *cpu, int type, hwaddr addr, hwaddr len);
void (*remove_all_breakpoints)(CPUState *cpu);
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 21d3f1d01e..6e1bd01725 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -46,7 +46,6 @@ extern bool kvm_readonly_mem_allowed;
extern bool kvm_direct_msi_allowed;
extern bool kvm_ioeventfd_any_length_allowed;
extern bool kvm_msi_use_devid;
-extern bool kvm_has_guest_debug;
#define kvm_enabled() (kvm_allowed)
/**
@@ -168,11 +167,6 @@ extern bool kvm_has_guest_debug;
*/
#define kvm_msi_devid_required() (kvm_msi_use_devid)
-/*
- * Does KVM support guest debugging
- */
-#define kvm_supports_guest_debug() (kvm_has_guest_debug)
-
#else
#define kvm_enabled() (0)
@@ -190,7 +184,6 @@ extern bool kvm_has_guest_debug;
#define kvm_direct_msi_enabled() (false)
#define kvm_ioeventfd_any_length_enabled() (false)
#define kvm_msi_devid_required() (false)
-#define kvm_supports_guest_debug() (false)
#endif /* CONFIG_KVM_IS_POSSIBLE */
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index 5c0e37514c..fbf4fe3497 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -99,6 +99,7 @@ static void kvm_accel_ops_class_init(ObjectClass *oc, void *data)
ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm;
#ifdef KVM_CAP_SET_GUEST_DEBUG
+ ops->supports_guest_debug = kvm_supports_guest_debug;
ops->insert_breakpoint = kvm_insert_breakpoint;
ops->remove_breakpoint = kvm_remove_breakpoint;
ops->remove_all_breakpoints = kvm_remove_all_breakpoints;
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b8c734fe3a..6ebff6e5a6 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3287,6 +3287,12 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
return data.err;
}
+bool kvm_supports_guest_debug(void)
+{
+ /* probed during kvm_init() */
+ return kvm_has_guest_debug;
+}
+
int kvm_insert_breakpoint(CPUState *cpu, int type, hwaddr addr, hwaddr len)
{
struct kvm_sw_breakpoint *bp;
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 965c2ad581..19cbf1db3a 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -93,6 +93,11 @@ void tcg_handle_interrupt(CPUState *cpu, int mask)
}
}
+static bool tcg_supports_guest_debug(void)
+{
+ return true;
+}
+
/* Translate GDB watchpoint type to a flags value for cpu_watchpoint_* */
static inline int xlat_gdb_type(CPUState *cpu, int gdbtype)
{
@@ -198,6 +203,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops)
}
}
+ ops->supports_guest_debug = tcg_supports_guest_debug;
ops->insert_breakpoint = tcg_insert_breakpoint;
ops->remove_breakpoint = tcg_remove_breakpoint;
ops->remove_all_breakpoints = tcg_remove_all_breakpoints;
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index ff9f3f9586..be88ca0d71 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -45,7 +45,6 @@
#include "qemu/sockets.h"
#include "sysemu/hw_accel.h"
-#include "sysemu/kvm.h"
#include "sysemu/runstate.h"
#include "semihosting/semihost.h"
#include "exec/exec-all.h"
@@ -3447,8 +3446,8 @@ int gdbserver_start(const char *device)
return -1;
}
- if (kvm_enabled() && !kvm_supports_guest_debug()) {
- error_report("gdbstub: KVM doesn't support guest debugging");
+ if (!gdb_supports_guest_debug()) {
+ error_report("gdbstub: current accelerator doesn't support guest debugging");
return -1;
}
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 4e73890379..f208c6cf15 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -15,6 +15,15 @@
#include "sysemu/cpus.h"
#include "internals.h"
+bool gdb_supports_guest_debug(void)
+{
+ const AccelOpsClass *ops = cpus_get_accel();
+ if (ops->supports_guest_debug) {
+ return ops->supports_guest_debug();
+ }
+ return false;
+}
+
int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len)
{
const AccelOpsClass *ops = cpus_get_accel();
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 42652b28a7..033e5fdd71 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -14,6 +14,12 @@
#include "hw/core/cpu.h"
#include "internals.h"
+bool gdb_supports_guest_debug(void)
+{
+ /* user-mode == TCG == supported */
+ return true;
+}
+
int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len)
{
CPUState *cpu;
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 15/15] accel/kvm: move kvm_update_guest_debug to inline stub
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
@ 2022-09-27 14:15 ` Alex Bennée
2022-09-27 14:14 ` [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs Alex Bennée
` (13 subsequent siblings)
14 siblings, 0 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Paolo Bonzini, open list:Overall KVM CPUs
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
include/sysemu/kvm.h | 16 ++++++++++++++++
accel/kvm/kvm-all.c | 6 ------
accel/stubs/kvm-stub.c | 5 -----
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6e1bd01725..790d35ef78 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -247,7 +247,23 @@ int kvm_on_sigbus(int code, void *addr);
void kvm_flush_coalesced_mmio_buffer(void);
+/**
+ * kvm_update_guest_debug(): ensure KVM debug structures updated
+ * @cs: the CPUState for this cpu
+ * @reinject_trap: KVM trap injection control
+ *
+ * There are usually per-arch specifics which will be handled by
+ * calling down to kvm_arch_update_guest_debug after the generic
+ * fields have been set.
+ */
+#ifdef KVM_CAP_SET_GUEST_DEBUG
int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
+#else
+static inline int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
+{
+ return -EINVAL;
+}
+#endif
/* internal API */
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 6ebff6e5a6..423fb1936f 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3395,12 +3395,6 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
}
}
-#else /* !KVM_CAP_SET_GUEST_DEBUG */
-
-static int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
-{
- return -EINVAL;
-}
#endif /* !KVM_CAP_SET_GUEST_DEBUG */
static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 2d79333143..5d2dd8f351 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -46,11 +46,6 @@ int kvm_has_many_ioeventfds(void)
return 0;
}
-int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
-{
- return -ENOSYS;
-}
-
int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
{
return 1;
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 15/15] accel/kvm: move kvm_update_guest_debug to inline stub
@ 2022-09-27 14:15 ` Alex Bennée
0 siblings, 0 replies; 43+ messages in thread
From: Alex Bennée @ 2022-09-27 14:15 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Alex Bennée, Paolo Bonzini, open list:Overall KVM CPUs
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
include/sysemu/kvm.h | 16 ++++++++++++++++
accel/kvm/kvm-all.c | 6 ------
accel/stubs/kvm-stub.c | 5 -----
3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6e1bd01725..790d35ef78 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -247,7 +247,23 @@ int kvm_on_sigbus(int code, void *addr);
void kvm_flush_coalesced_mmio_buffer(void);
+/**
+ * kvm_update_guest_debug(): ensure KVM debug structures updated
+ * @cs: the CPUState for this cpu
+ * @reinject_trap: KVM trap injection control
+ *
+ * There are usually per-arch specifics which will be handled by
+ * calling down to kvm_arch_update_guest_debug after the generic
+ * fields have been set.
+ */
+#ifdef KVM_CAP_SET_GUEST_DEBUG
int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
+#else
+static inline int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
+{
+ return -EINVAL;
+}
+#endif
/* internal API */
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 6ebff6e5a6..423fb1936f 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3395,12 +3395,6 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
}
}
-#else /* !KVM_CAP_SET_GUEST_DEBUG */
-
-static int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
-{
- return -EINVAL;
-}
#endif /* !KVM_CAP_SET_GUEST_DEBUG */
static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 2d79333143..5d2dd8f351 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -46,11 +46,6 @@ int kvm_has_many_ioeventfds(void)
return 0;
}
-int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
-{
- return -ENOSYS;
-}
-
int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
{
return 1;
--
2.34.1
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-09-27 14:14 ` [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
@ 2022-09-28 16:42 ` Richard Henderson
2022-09-28 18:56 ` Peter Maydell
0 siblings, 1 reply; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:42 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
Eduardo Habkost, Peter Xu, Jason Wang, Peter Maydell
On 9/27/22 07:14, Alex Bennée wrote:
> +/*
> + * Every memory transaction comes from a specific place which defines
> + * how requester_id should be handled. For CPU's the requester_id is
> + * the global cpu_index which needs further processing if you need to
> + * work out which socket or complex it comes from. UNSPECIFIED is the
> + * default for otherwise undefined MemTxAttrs. PCI indicates the
> + * requester_id is a PCI id. MACHINE indicates a machine specific
> + * encoding which needs further processing to decode into its
> + * constituent parts.
> + */
> +typedef enum MemTxRequesterType {
> + MTRT_CPU = 0,
> + MTRT_UNSPECIFIED,
> + MTRT_PCI,
> + MTRT_MACHINE
> +} MemTxRequesterType;
I wonder if UNSPECIFIED should be zero, or even ILLEGAL, attempting to catch MemTxAttrs
foo = { }, which is now ill-formed as it doesn't initialize .requester_id to match CPU.
Perhaps a preceeding patch, should introduce
#define MEMTXATTRS_CPU(cpu) ((MemTxAttrs){ })
and modify all uses of "= { }", at least under target/.
> --- a/hw/misc/tz-msc.c
> +++ b/hw/misc/tz-msc.c
> @@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
> 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;
This is surely incomplete. You can't just set "cpu" without saying where it's from.
Since this device is only used by the ARMSSE machine, I would hope that attrs.unspecified
should never be set before the patch, and thus MTRT_CPU should be set afterward.
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs
2022-09-27 14:14 ` [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs Alex Bennée
@ 2022-09-28 16:45 ` Richard Henderson
0 siblings, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:45 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: qemu-arm, Peter Maydell
On 9/27/22 07:14, Alex Bennée wrote:
> 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.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v3
> - reword commit summary
> ---
> target/arm/ptw.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 2ddfc028ab..4b0dc9bd14 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2289,6 +2289,9 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
> ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
> bool is_secure = regime_is_secure(env, mmu_idx);
>
> + attrs->requester_type = MEMTXATTRS_CPU;
This shouldn't build since you renamed the enumerator.
You should use *attrs = MEMTXATTRS_CPU(env_cpu(env)).
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 03/15] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-27 14:14 ` [PATCH v3 03/15] target/arm: ensure HVF traps " Alex Bennée
@ 2022-09-28 16:47 ` Richard Henderson
2022-10-04 10:25 ` Mads Ynddal
1 sibling, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:47 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Alexander Graf, Mads Ynddal, Peter Maydell
On 9/27/22 07:14, Alex Bennée wrote:
> 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).
>
> Acked-by: Alexander Graf<agraf@csgraf.de>
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> Cc: Mads Ynddal<mads@ynddal.dk>
>
> ---
> v2
> - update MEMTXATTRS macro
> ---
> target/arm/hvf/hvf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 04/15] target/arm: ensure KVM traps set appropriate MemTxAttrs
2022-09-27 14:14 ` Alex Bennée
(?)
@ 2022-09-28 16:49 ` Richard Henderson
-1 siblings, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:49 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Peter Maydell, Paolo Bonzini, open list:Overall KVM CPUs
On 9/27/22 07:14, 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.
>
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
>
> ---
> v3
> - new for v3
> ---
> target/arm/kvm.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 05/15] target/arm: ensure ptw accesses set appropriate MemTxAttrs
2022-09-27 14:14 ` [PATCH v3 05/15] target/arm: ensure ptw accesses " Alex Bennée
@ 2022-09-28 16:52 ` Richard Henderson
0 siblings, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:52 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: qemu-arm, Peter Maydell
On 9/27/22 07:14, Alex Bennée wrote:
> @@ -2289,8 +2289,8 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
> ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
> bool is_secure = regime_is_secure(env, mmu_idx);
>
> - attrs->requester_type = MEMTXATTRS_CPU;
> - attrs->requester_id = env_cpu(env)->cpu_index;
> + result->attrs.requester_type = MTRT_CPU;
> + result->attrs.requester_id = env_cpu(env)->cpu_index;
This hunk shouldn't compile, or the earlier patch shouldn't.
I think you have a rebase error in there somewhere.
> @@ -280,7 +280,7 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
> ARMMMUIdx mmu_idx, ARMMMUFaultInfo *fi)
> {
> CPUState *cs = env_cpu(env);
> - MemTxAttrs attrs = {};
> + MemTxAttrs attrs = MEMTXATTRS_CPU(cs);
> MemTxResult result = MEMTX_OK;
> AddressSpace *as;
> uint64_t data;
Would be handled by a new patch introducing MEMTXATTRS_CPU, as described earlier.
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 06/15] target/arm: ensure m-profile helpers set appropriate MemTxAttrs
2022-09-27 14:14 ` [PATCH v3 06/15] target/arm: ensure m-profile helpers " Alex Bennée
@ 2022-09-28 16:57 ` Richard Henderson
0 siblings, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:57 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: qemu-arm, Peter Maydell
On 9/27/22 07:14, 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>
> ---
> 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 5ee4ee15b3..d244e9c1c5 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 = { .attrs = MEMTXATTRS_CPU(cs) };
> ARMMMUFaultInfo fi = {};
> bool secure = mmu_idx & ARM_MMU_IDX_M_S;
> int exc;
Surely this is redundant with the initialization by get_phys_page()?
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 07/15] qtest: make read/write operation appear to be from CPU
2022-09-27 14:14 ` [PATCH v3 07/15] qtest: make read/write operation appear to be from CPU Alex Bennée
@ 2022-09-28 16:58 ` Richard Henderson
0 siblings, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 16:58 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Thomas Huth, Laurent Vivier, Paolo Bonzini
On 9/27/22 07:14, Alex Bennée wrote:
> The point of qtest is to simulate how running code might interact with
> the system. However because it's not a real system we have places in
> the code which especially handle check qtest_enabled() before
> referencing current_cpu. Now we can encode these details in the
> MemTxAttrs lets do that so we can start removing them.
>
> 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(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-27 14:14 ` [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
@ 2022-09-28 17:03 ` Richard Henderson
0 siblings, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 17:03 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: qemu-arm, Peter Maydell
On 9/27/22 07:14, Alex Bennée wrote:
> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
> This solves edge cases like accessing via gdbstub or qtest. 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
> ---
> hw/intc/arm_gic.c | 174 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 118 insertions(+), 56 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 492b2421ab..7b4f3fb81a 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -56,17 +56,42 @@ 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.
> + */
> +typedef struct {
> + bool valid;
> + int cpu_index;
> +} GicCPU;
> +
> +static inline GicCPU gic_get_current_cpu(GICState *s, 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 (GicCPU) { .valid = false };
> }
> - return 0;
> + g_assert(attrs.requester_id < s->num_cpu);
> +
> + return (GicCPU) { .valid = true, .cpu_index = attrs.requester_id };
> }
I think you should split this function, and do away with the struct.
> static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
> unsigned size, MemTxAttrs attrs)
> {
> + GICState *s = (GICState *)opaque;
> + GicCPU cpu = gic_get_current_cpu(s, attrs);
> +
> + if (!cpu.valid) {
> + return MEMTX_ACCESS_ERROR;
> + }
This would become
if (!gic_valid_cpu(attrs)) {
return MEMTX_ACCESS_ERROR;
}
cpu = gic_get_cpu(attrs);
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index
2022-09-27 14:14 ` [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
@ 2022-09-28 17:04 ` Richard Henderson
2022-10-19 10:46 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 43+ messages in thread
From: Richard Henderson @ 2022-09-28 17:04 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: qemu-arm, Peter Maydell
On 9/27/22 07:14, Alex Bennée wrote:
> 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
> ---
> hw/timer/arm_mptimer.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index cdfca3000b..34693a2534 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -41,9 +41,10 @@
> * which is used in both the ARM11MPCore and Cortex-A9MP.
> */
>
> -static inline int get_current_cpu(ARMMPTimerState *s)
> +static inline int get_current_cpu(ARMMPTimerState *s, MemTxAttrs attrs)
> {
> - int cpu_id = current_cpu ? current_cpu->cpu_index : 0;
> + int cpu_id = attrs.requester_id;
> + g_assert(attrs.requester_type == MTRT_CPU);
I would guess this needs the same non-assert treatment as the gic.
r~
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-09-28 16:42 ` Richard Henderson
@ 2022-09-28 18:56 ` Peter Maydell
2022-10-04 13:32 ` Alex Bennée
0 siblings, 1 reply; 43+ messages in thread
From: Peter Maydell @ 2022-09-28 18:56 UTC (permalink / raw)
To: Richard Henderson
Cc: Alex Bennée, qemu-devel, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Paolo Bonzini, Eduardo Habkost, Peter Xu,
Jason Wang
On Wed, 28 Sept 2022 at 17:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/27/22 07:14, Alex Bennée wrote:
> > --- a/hw/misc/tz-msc.c
> > +++ b/hw/misc/tz-msc.c
> > @@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
> > 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;
>
> This is surely incomplete. You can't just set "cpu" without saying where it's from.
>
> Since this device is only used by the ARMSSE machine, I would hope that attrs.unspecified
> should never be set before the patch, and thus MTRT_CPU should be set afterward.
The MSC is in the address map like most other stuff, and thus there is
no restriction on whether it can be accessed by other things than CPUs
(DMAing to it would be silly but is perfectly possible).
The intent of the code is "pass this transaction through, but force
it to be Secure/NonSecure regardless of what it was before". That
should not involve a change of the requester type.
thanks
-- PMM
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 12/15] gdbstub: move sstep flags probing into AccelClass
2022-09-27 14:15 ` [PATCH v3 12/15] gdbstub: move sstep flags probing into AccelClass Alex Bennée
@ 2022-10-04 10:25 ` Mads Ynddal
0 siblings, 0 replies; 43+ messages in thread
From: Mads Ynddal @ 2022-10-04 10:25 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, open list:ARM cores, Richard Henderson,
Philippe Mathieu-Daudé,
Paolo Bonzini, open list:Overall KVM CPUs
> The support of single-stepping is very much dependent on support from
> the accelerator we are using. To avoid special casing in gdbstub move
> the probing out to an AccelClass function so future accelerators can
> put their code there.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> ---
> include/qemu/accel.h | 12 ++++++++++++
> include/sysemu/kvm.h | 8 --------
> accel/accel-common.c | 10 ++++++++++
> accel/kvm/kvm-all.c | 14 +++++++++++++-
> accel/tcg/tcg-all.c | 17 +++++++++++++++++
> gdbstub/gdbstub.c | 22 ++++------------------
> 6 files changed, 56 insertions(+), 27 deletions(-)
Reviewed-by: Mads Ynddal <mads@ynddal.dk>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 03/15] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-27 14:14 ` [PATCH v3 03/15] target/arm: ensure HVF traps " Alex Bennée
2022-09-28 16:47 ` Richard Henderson
@ 2022-10-04 10:25 ` Mads Ynddal
1 sibling, 0 replies; 43+ messages in thread
From: Mads Ynddal @ 2022-10-04 10:25 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, qemu-arm, Alexander Graf, Peter Maydell
> 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).
>
> Acked-by: Alexander Graf <agraf@csgraf.de>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
>
> ---
> v2
> - update MEMTXATTRS macro
> ---
> target/arm/hvf/hvf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Mads Ynddal <mads@ynddal.dk>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 13/15] gdbstub: move breakpoint logic to accel ops
2022-09-27 14:15 ` [PATCH v3 13/15] gdbstub: move breakpoint logic to accel ops Alex Bennée
@ 2022-10-04 10:25 ` Mads Ynddal
0 siblings, 0 replies; 43+ messages in thread
From: Mads Ynddal @ 2022-10-04 10:25 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, qemu-arm, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé,
open list:Overall KVM CPUs
> As HW virtualization requires specific support to handle breakpoints
> lets push out special casing out of the core gdbstub code and into
> AccelOpsClass. This will make it easier to add other accelerator
> support and reduces some of the stub shenanigans.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> ---
> accel/kvm/kvm-cpus.h | 3 +
> gdbstub/internals.h | 16 +++++
> include/sysemu/accel-ops.h | 6 ++
> include/sysemu/cpus.h | 3 +
> include/sysemu/kvm.h | 5 --
> accel/kvm/kvm-accel-ops.c | 8 +++
> accel/kvm/kvm-all.c | 24 +------
> accel/stubs/kvm-stub.c | 16 -----
> accel/tcg/tcg-accel-ops.c | 92 +++++++++++++++++++++++++++
> gdbstub/gdbstub.c | 127 +++----------------------------------
> gdbstub/softmmu.c | 42 ++++++++++++
> gdbstub/user.c | 62 ++++++++++++++++++
> softmmu/cpus.c | 7 ++
> gdbstub/meson.build | 8 +++
> 14 files changed, 259 insertions(+), 160 deletions(-)
Reviewed-by: Mads Ynddal <mads@ynddal.dk>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 14/15] gdbstub: move guest debug support check to ops
2022-09-27 14:15 ` [PATCH v3 14/15] gdbstub: move guest debug support check to ops Alex Bennée
@ 2022-10-04 10:25 ` Mads Ynddal
0 siblings, 0 replies; 43+ messages in thread
From: Mads Ynddal @ 2022-10-04 10:25 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, open list:ARM cores, Philippe Mathieu-Daudé,
Paolo Bonzini, Richard Henderson, open list:Overall KVM CPUs
> This removes the final hard coding of kvm_enabled() in gdbstub and
> moves the check to an AccelOps.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> ---
> accel/kvm/kvm-cpus.h | 1 +
> gdbstub/internals.h | 1 +
> include/sysemu/accel-ops.h | 1 +
> include/sysemu/kvm.h | 7 -------
> accel/kvm/kvm-accel-ops.c | 1 +
> accel/kvm/kvm-all.c | 6 ++++++
> accel/tcg/tcg-accel-ops.c | 6 ++++++
> gdbstub/gdbstub.c | 5 ++---
> gdbstub/softmmu.c | 9 +++++++++
> gdbstub/user.c | 6 ++++++
> 10 files changed, 33 insertions(+), 10 deletions(-)
Reviewed-by: Mads Ynddal <mads@ynddal.dk>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-09-28 18:56 ` Peter Maydell
@ 2022-10-04 13:32 ` Alex Bennée
2022-10-04 14:54 ` Peter Maydell
0 siblings, 1 reply; 43+ messages in thread
From: Alex Bennée @ 2022-10-04 13:32 UTC (permalink / raw)
To: Peter Maydell
Cc: Richard Henderson, qemu-devel, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Paolo Bonzini, Eduardo Habkost, Peter Xu,
Jason Wang
Peter Maydell <peter.maydell@linaro.org> writes:
> On Wed, 28 Sept 2022 at 17:42, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 9/27/22 07:14, Alex Bennée wrote:
>> > --- a/hw/misc/tz-msc.c
>> > +++ b/hw/misc/tz-msc.c
>> > @@ -137,11 +137,11 @@ static MemTxResult tz_msc_read(void *opaque, hwaddr addr, uint64_t *pdata,
>> > 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;
>>
>> This is surely incomplete. You can't just set "cpu" without saying where it's from.
>>
>> Since this device is only used by the ARMSSE machine, I would hope that attrs.unspecified
>> should never be set before the patch, and thus MTRT_CPU should be set afterward.
>
> The MSC is in the address map like most other stuff, and thus there is
> no restriction on whether it can be accessed by other things than CPUs
> (DMAing to it would be silly but is perfectly possible).
>
> The intent of the code is "pass this transaction through, but force
> it to be Secure/NonSecure regardless of what it was before". That
> should not involve a change of the requester type.
Should we assert (or warn) when the requester_type is unspecified?
>
> thanks
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-10-04 13:32 ` Alex Bennée
@ 2022-10-04 14:54 ` Peter Maydell
2022-10-31 12:08 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 43+ messages in thread
From: Peter Maydell @ 2022-10-04 14:54 UTC (permalink / raw)
To: Alex Bennée
Cc: Richard Henderson, qemu-devel, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Paolo Bonzini, Eduardo Habkost, Peter Xu,
Jason Wang
On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > The MSC is in the address map like most other stuff, and thus there is
> > no restriction on whether it can be accessed by other things than CPUs
> > (DMAing to it would be silly but is perfectly possible).
> >
> > The intent of the code is "pass this transaction through, but force
> > it to be Secure/NonSecure regardless of what it was before". That
> > should not involve a change of the requester type.
>
> Should we assert (or warn) when the requester_type is unspecified?
Not in the design of MemTxAttrs that's currently in git, no:
in that design it's perfectly fine for something generating
memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
to meaning a bunch of things including "not secure").
thanks
-- PMM
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 04/15] target/arm: ensure KVM traps set appropriate MemTxAttrs
2022-09-27 14:14 ` Alex Bennée
(?)
(?)
@ 2022-10-19 10:44 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-10-19 10:44 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Peter Maydell, Paolo Bonzini, open list:Overall KVM CPUs
On 27/9/22 16:14, 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.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v3
> - new for v3
> ---
> target/arm/kvm.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
> void kvm_arm_vm_state_change(void *opaque, bool running, RunState state)
> @@ -1003,6 +1004,10 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> hwaddr xlat, len, doorbell_gpa;
> MemoryRegionSection mrs;
> MemoryRegion *mr;
> + MemTxAttrs attrs = {
> + .requester_type = MTRT_PCI,
> + .requester_id = pci_requester_id(dev)
> + };
Can we add a MEMTXATTRS_PCI() macro similar to MEMTXATTRS_CPU()?
#define MEMTXATTRS_PCI(pci_dev) ((MemTxAttrs) \
{.requester_type = MTRT_PCI, \
.requester_id = pci_requester_id(pci_dev)})
So here we can use:
MemTxAttrs attrs = MEMTXATTRS_PCI(dev);
> @@ -1012,8 +1017,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;
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index
2022-09-27 14:14 ` [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
2022-09-28 17:04 ` Richard Henderson
@ 2022-10-19 10:46 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-10-19 10:46 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: qemu-arm, Richard Henderson, Peter Maydell
On 27/9/22 16:14, Alex Bennée wrote:
> 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
> ---
> hw/timer/arm_mptimer.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 11/15] gdbstub: move into its own sub directory
2022-09-27 14:15 ` [PATCH v3 11/15] gdbstub: move into its own sub directory Alex Bennée
@ 2022-10-19 10:47 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-10-19 10:47 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Richard Henderson, Philippe Mathieu-Daudé,
Stefan Hajnoczi
On 27/9/22 16:15, Alex Bennée wrote:
> This is in preparation of future refactoring as well as cleaning up
> the source tree. Aside from the minor tweaks to meson and trace.h this
> is pure code motion.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> meson.build | 4 +++-
> gdbstub/trace.h | 1 +
> gdbstub.c => gdbstub/gdbstub.c | 2 +-
> MAINTAINERS | 2 +-
> gdbstub/meson.build | 1 +
> gdbstub/trace-events | 29 +++++++++++++++++++++++++++++
> trace-events | 28 ----------------------------
> 7 files changed, 36 insertions(+), 31 deletions(-)
> create mode 100644 gdbstub/trace.h
> rename gdbstub.c => gdbstub/gdbstub.c (99%)
> create mode 100644 gdbstub/meson.build
> create mode 100644 gdbstub/trace-events
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 15/15] accel/kvm: move kvm_update_guest_debug to inline stub
2022-09-27 14:15 ` Alex Bennée
(?)
@ 2022-10-19 10:53 ` Philippe Mathieu-Daudé
-1 siblings, 0 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-10-19 10:53 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: qemu-arm, Paolo Bonzini, open list:Overall KVM CPUs
On 27/9/22 16:15, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/sysemu/kvm.h | 16 ++++++++++++++++
> accel/kvm/kvm-all.c | 6 ------
> accel/stubs/kvm-stub.c | 5 -----
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 6e1bd01725..790d35ef78 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -247,7 +247,23 @@ int kvm_on_sigbus(int code, void *addr);
>
> void kvm_flush_coalesced_mmio_buffer(void);
>
> +/**
> + * kvm_update_guest_debug(): ensure KVM debug structures updated
> + * @cs: the CPUState for this cpu
> + * @reinject_trap: KVM trap injection control
> + *
> + * There are usually per-arch specifics which will be handled by
> + * calling down to kvm_arch_update_guest_debug after the generic
> + * fields have been set.
> + */
> +#ifdef KVM_CAP_SET_GUEST_DEBUG
> int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap);
> +#else
> +static inline int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
> +{
> + return -EINVAL;
Wouldn't -ENOSYS make more sense in this case?
> +}
> +#endif
>
> /* internal API */
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 6ebff6e5a6..423fb1936f 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3395,12 +3395,6 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
> }
> }
>
> -#else /* !KVM_CAP_SET_GUEST_DEBUG */
> -
> -static int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
> -{
> - return -EINVAL;
> -}
> #endif /* !KVM_CAP_SET_GUEST_DEBUG */
>
> static int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 2d79333143..5d2dd8f351 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -46,11 +46,6 @@ int kvm_has_many_ioeventfds(void)
> return 0;
> }
>
> -int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
> -{
> - return -ENOSYS;
> -}
> -
> int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
> {
> return 1;
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-10-04 14:54 ` Peter Maydell
@ 2022-10-31 12:08 ` Philippe Mathieu-Daudé
2022-10-31 13:03 ` Peter Maydell
0 siblings, 1 reply; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-10-31 12:08 UTC (permalink / raw)
To: Peter Maydell, Alex Bennée
Cc: Richard Henderson, qemu-devel, qemu-arm, Michael S. Tsirkin,
Marcel Apfelbaum, Paolo Bonzini, Eduardo Habkost, Peter Xu,
Jason Wang
On 4/10/22 16:54, Peter Maydell wrote:
> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> The MSC is in the address map like most other stuff, and thus there is
>>> no restriction on whether it can be accessed by other things than CPUs
>>> (DMAing to it would be silly but is perfectly possible).
>>>
>>> The intent of the code is "pass this transaction through, but force
>>> it to be Secure/NonSecure regardless of what it was before". That
>>> should not involve a change of the requester type.
>>
>> Should we assert (or warn) when the requester_type is unspecified?
>
> Not in the design of MemTxAttrs that's currently in git, no:
> in that design it's perfectly fine for something generating
> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
> to meaning a bunch of things including "not secure").
In tz_mpc_handle_block():
static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
attrs)
{
/* Handle a blocked transaction: raise IRQ, capture info, etc */
if (!s->int_stat) {
s->int_info1 = addr;
s->int_info2 = 0;
s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
attrs.requester_id & 0xffff);
s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
~attrs.secure);
s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
tz_mpc_cfg_ns(s, addr));
Should we check whether the requester is MTRT_CPU?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-10-31 12:08 ` Philippe Mathieu-Daudé
@ 2022-10-31 13:03 ` Peter Maydell
2022-11-11 13:23 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 43+ messages in thread
From: Peter Maydell @ 2022-10-31 13:03 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alex Bennée, Richard Henderson, qemu-devel, qemu-arm,
Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
Eduardo Habkost, Peter Xu, Jason Wang
On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 4/10/22 16:54, Peter Maydell wrote:
> > On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >>
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>> The MSC is in the address map like most other stuff, and thus there is
> >>> no restriction on whether it can be accessed by other things than CPUs
> >>> (DMAing to it would be silly but is perfectly possible).
> >>>
> >>> The intent of the code is "pass this transaction through, but force
> >>> it to be Secure/NonSecure regardless of what it was before". That
> >>> should not involve a change of the requester type.
> >>
> >> Should we assert (or warn) when the requester_type is unspecified?
> >
> > Not in the design of MemTxAttrs that's currently in git, no:
> > in that design it's perfectly fine for something generating
> > memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
> > to meaning a bunch of things including "not secure").
>
> In tz_mpc_handle_block():
>
> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
> attrs)
> {
> /* Handle a blocked transaction: raise IRQ, capture info, etc */
> if (!s->int_stat) {
>
> s->int_info1 = addr;
> s->int_info2 = 0;
> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
> attrs.requester_id & 0xffff);
> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
> ~attrs.secure);
> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
> tz_mpc_cfg_ns(s, addr));
>
>
> Should we check whether the requester is MTRT_CPU?
That code is basically assuming that the requester_id is the AMBA AHB
'HMASTER' field (i.e. something hopefully unique to all things that
send out transactions, not necessarily limited only to CPUs), which is a
somewhat bogus assumption given that it isn't currently any such thing...
I'm not sure if/how this patchset plans to model generic "ID of transaction
generator".
-- PMM
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-10-31 13:03 ` Peter Maydell
@ 2022-11-11 13:23 ` Philippe Mathieu-Daudé
2022-11-11 13:58 ` Alex Bennée
2022-11-14 10:06 ` Peter Maydell
0 siblings, 2 replies; 43+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-11 13:23 UTC (permalink / raw)
To: Peter Maydell
Cc: Alex Bennée, Richard Henderson, qemu-devel, qemu-arm,
Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
Eduardo Habkost, Peter Xu, Jason Wang
On 31/10/22 14:03, Peter Maydell wrote:
> On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 4/10/22 16:54, Peter Maydell wrote:
>>> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>>
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>> The MSC is in the address map like most other stuff, and thus there is
>>>>> no restriction on whether it can be accessed by other things than CPUs
>>>>> (DMAing to it would be silly but is perfectly possible).
>>>>>
>>>>> The intent of the code is "pass this transaction through, but force
>>>>> it to be Secure/NonSecure regardless of what it was before". That
>>>>> should not involve a change of the requester type.
>>>>
>>>> Should we assert (or warn) when the requester_type is unspecified?
>>>
>>> Not in the design of MemTxAttrs that's currently in git, no:
>>> in that design it's perfectly fine for something generating
>>> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
>>> to meaning a bunch of things including "not secure").
>>
>> In tz_mpc_handle_block():
>>
>> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
>> attrs)
>> {
>> /* Handle a blocked transaction: raise IRQ, capture info, etc */
>> if (!s->int_stat) {
>>
>> s->int_info1 = addr;
>> s->int_info2 = 0;
>> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
>> attrs.requester_id & 0xffff);
>> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
>> ~attrs.secure);
>> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
>> tz_mpc_cfg_ns(s, addr));
>>
>>
>> Should we check whether the requester is MTRT_CPU?
>
> That code is basically assuming that the requester_id is the AMBA AHB
> 'HMASTER' field (i.e. something hopefully unique to all things that
> send out transactions, not necessarily limited only to CPUs), which is a
> somewhat bogus assumption given that it isn't currently any such thing...
>
> I'm not sure if/how this patchset plans to model generic "ID of transaction
> generator".
Does your 'generic "ID of transaction generator"' fit into MTRT_MACHINE
described as "for more complex encoding":
'MACHINE indicates a machine specific encoding which needs further
processing to decode into its constituent parts.'
?
>
> -- PMM
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-11-11 13:23 ` Philippe Mathieu-Daudé
@ 2022-11-11 13:58 ` Alex Bennée
2022-11-14 10:06 ` Peter Maydell
1 sibling, 0 replies; 43+ messages in thread
From: Alex Bennée @ 2022-11-11 13:58 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Richard Henderson, qemu-devel, qemu-arm,
Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
Eduardo Habkost, Peter Xu, Jason Wang
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 31/10/22 14:03, Peter Maydell wrote:
>> On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> On 4/10/22 16:54, Peter Maydell wrote:
>>>> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>
>>>>>
>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>> The MSC is in the address map like most other stuff, and thus there is
>>>>>> no restriction on whether it can be accessed by other things than CPUs
>>>>>> (DMAing to it would be silly but is perfectly possible).
>>>>>>
>>>>>> The intent of the code is "pass this transaction through, but force
>>>>>> it to be Secure/NonSecure regardless of what it was before". That
>>>>>> should not involve a change of the requester type.
>>>>>
>>>>> Should we assert (or warn) when the requester_type is unspecified?
>>>>
>>>> Not in the design of MemTxAttrs that's currently in git, no:
>>>> in that design it's perfectly fine for something generating
>>>> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
>>>> to meaning a bunch of things including "not secure").
>>>
>>> In tz_mpc_handle_block():
>>>
>>> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
>>> attrs)
>>> {
>>> /* Handle a blocked transaction: raise IRQ, capture info, etc */
>>> if (!s->int_stat) {
>>>
>>> s->int_info1 = addr;
>>> s->int_info2 = 0;
>>> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
>>> attrs.requester_id & 0xffff);
>>> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
>>> ~attrs.secure);
>>> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
>>> tz_mpc_cfg_ns(s, addr));
>>>
>>>
>>> Should we check whether the requester is MTRT_CPU?
>> That code is basically assuming that the requester_id is the AMBA
>> AHB
>> 'HMASTER' field (i.e. something hopefully unique to all things that
>> send out transactions, not necessarily limited only to CPUs), which is a
>> somewhat bogus assumption given that it isn't currently any such thing...
>> I'm not sure if/how this patchset plans to model generic "ID of
>> transaction
>> generator".
>
> Does your 'generic "ID of transaction generator"' fit into
> MTRT_MACHINE described as "for more complex encoding":
>
> 'MACHINE indicates a machine specific encoding which needs further
> processing to decode into its constituent parts.'
>
> ?
Yes - I've just done something similar to model the IOAPIC on x86.
Currently that uses a magic number of requester_id that is unique to the
"machine bus" but it could multiplex multiple bits of data on more
complex topologies.
I'll post v5 soon now I have x86 working.
--
Alex Bennée
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs
2022-11-11 13:23 ` Philippe Mathieu-Daudé
2022-11-11 13:58 ` Alex Bennée
@ 2022-11-14 10:06 ` Peter Maydell
1 sibling, 0 replies; 43+ messages in thread
From: Peter Maydell @ 2022-11-14 10:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alex Bennée, Richard Henderson, qemu-devel, qemu-arm,
Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
Eduardo Habkost, Peter Xu, Jason Wang
On Fri, 11 Nov 2022 at 13:23, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 31/10/22 14:03, Peter Maydell wrote:
> > On Mon, 31 Oct 2022 at 12:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> On 4/10/22 16:54, Peter Maydell wrote:
> >>> On Tue, 4 Oct 2022 at 14:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>>
> >>>>
> >>>> Peter Maydell <peter.maydell@linaro.org> writes:
> >>>>> The MSC is in the address map like most other stuff, and thus there is
> >>>>> no restriction on whether it can be accessed by other things than CPUs
> >>>>> (DMAing to it would be silly but is perfectly possible).
> >>>>>
> >>>>> The intent of the code is "pass this transaction through, but force
> >>>>> it to be Secure/NonSecure regardless of what it was before". That
> >>>>> should not involve a change of the requester type.
> >>>>
> >>>> Should we assert (or warn) when the requester_type is unspecified?
> >>>
> >>> Not in the design of MemTxAttrs that's currently in git, no:
> >>> in that design it's perfectly fine for something generating
> >>> memory transactions to use MEMTXATTRS_UNSPECIFIED (which defaults
> >>> to meaning a bunch of things including "not secure").
> >>
> >> In tz_mpc_handle_block():
> >>
> >> static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs
> >> attrs)
> >> {
> >> /* Handle a blocked transaction: raise IRQ, capture info, etc */
> >> if (!s->int_stat) {
> >>
> >> s->int_info1 = addr;
> >> s->int_info2 = 0;
> >> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
> >> attrs.requester_id & 0xffff);
> >> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
> >> ~attrs.secure);
> >> s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
> >> tz_mpc_cfg_ns(s, addr));
> >>
> >>
> >> Should we check whether the requester is MTRT_CPU?
> >
> > That code is basically assuming that the requester_id is the AMBA AHB
> > 'HMASTER' field (i.e. something hopefully unique to all things that
> > send out transactions, not necessarily limited only to CPUs), which is a
> > somewhat bogus assumption given that it isn't currently any such thing...
> >
> > I'm not sure if/how this patchset plans to model generic "ID of transaction
> > generator".
>
> Does your 'generic "ID of transaction generator"' fit into MTRT_MACHINE
> described as "for more complex encoding":
>
> 'MACHINE indicates a machine specific encoding which needs further
> processing to decode into its constituent parts.'
>
> ?
Not really, because "generic ID of a transaction generator" is a
superset of "ID per CPU", not something separate that sits
alongside it...
-- PMM
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2022-11-15 0:25 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 14:14 [PATCH v3 00/15] gdbstub/next (MemTxAttrs, re-factoring) Alex Bennée
2022-09-27 14:14 ` [PATCH v3 01/15] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
2022-09-28 16:42 ` Richard Henderson
2022-09-28 18:56 ` Peter Maydell
2022-10-04 13:32 ` Alex Bennée
2022-10-04 14:54 ` Peter Maydell
2022-10-31 12:08 ` Philippe Mathieu-Daudé
2022-10-31 13:03 ` Peter Maydell
2022-11-11 13:23 ` Philippe Mathieu-Daudé
2022-11-11 13:58 ` Alex Bennée
2022-11-14 10:06 ` Peter Maydell
2022-09-27 14:14 ` [PATCH v3 02/15] target/arm: ensure TCG IO accesses set appropriate MemTxAttrs Alex Bennée
2022-09-28 16:45 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 03/15] target/arm: ensure HVF traps " Alex Bennée
2022-09-28 16:47 ` Richard Henderson
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:14 ` [PATCH v3 04/15] target/arm: ensure KVM " Alex Bennée
2022-09-27 14:14 ` Alex Bennée
2022-09-28 16:49 ` Richard Henderson
2022-10-19 10:44 ` Philippe Mathieu-Daudé
2022-09-27 14:14 ` [PATCH v3 05/15] target/arm: ensure ptw accesses " Alex Bennée
2022-09-28 16:52 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 06/15] target/arm: ensure m-profile helpers " Alex Bennée
2022-09-28 16:57 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 07/15] qtest: make read/write operation appear to be from CPU Alex Bennée
2022-09-28 16:58 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 08/15] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
2022-09-28 17:03 ` Richard Henderson
2022-09-27 14:14 ` [PATCH v3 09/15] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
2022-09-28 17:04 ` Richard Henderson
2022-10-19 10:46 ` Philippe Mathieu-Daudé
2022-09-27 14:14 ` [PATCH v3 10/15] configure: move detected gdb to TCG's config-host.mak Alex Bennée
2022-09-27 14:15 ` [PATCH v3 11/15] gdbstub: move into its own sub directory Alex Bennée
2022-10-19 10:47 ` Philippe Mathieu-Daudé
2022-09-27 14:15 ` [PATCH v3 12/15] gdbstub: move sstep flags probing into AccelClass Alex Bennée
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:15 ` [PATCH v3 13/15] gdbstub: move breakpoint logic to accel ops Alex Bennée
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:15 ` [PATCH v3 14/15] gdbstub: move guest debug support check to ops Alex Bennée
2022-10-04 10:25 ` Mads Ynddal
2022-09-27 14:15 ` [PATCH v3 15/15] accel/kvm: move kvm_update_guest_debug to inline stub Alex Bennée
2022-09-27 14:15 ` Alex Bennée
2022-10-19 10:53 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.