* [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-25 10:08 ` Richard Henderson
2022-09-22 14:58 ` [PATCH v1 2/9] qtest: make read/write operation appear to be from CPU Alex Bennée
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, Alex Bennée, Richard Henderson, Paolo Bonzini
We currently have hacks across the hw/ to reference current_cpu to
work out what the current accessing CPU is. This breaks in some cases
including using gdbstub to access HW state. As we have MemTxAttrs to
describe details about the access lets extend it to mention if this is
a CPU access and which one it is.
There are a number of places we need to fix up including:
CPU helpers directly calling address_space_*() fns
models in hw/ fishing the data out of current_cpu
I'll start addressing some of these in following patches.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v2
- use separate field cpu_index
- bool for requester_is_cpu
---
include/exec/memattrs.h | 4 ++++
accel/tcg/cputlb.c | 22 ++++++++++++++++------
hw/core/cpu-sysemu.c | 17 +++++++++++++----
3 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..e83a993c21 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -43,6 +43,10 @@ typedef struct MemTxAttrs {
* (see MEMTX_ACCESS_ERROR).
*/
unsigned int memory:1;
+ /* Requester is CPU (or as CPU, e.g. debug) */
+ bool requester_is_cpu:1;
+ /* cpu_index (if requester_is_cpu) */
+ unsigned int cpu_index:16;
/* Requester ID (for MSI for example) */
unsigned int requester_id:16;
/* Invert endianness for this page */
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 8fad2d9b83..5d88569eb5 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1340,8 +1340,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
uint64_t val;
bool locked = false;
MemTxResult r;
+ MemTxAttrs attrs = iotlbentry->attrs;
- section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+ /* encode the accessing CPU */
+ attrs.requester_is_cpu = 1;
+ attrs.cpu_index = cpu->cpu_index;
+
+ section = iotlb_to_section(cpu, iotlbentry->addr, attrs);
mr = section->mr;
mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
cpu->mem_io_pc = retaddr;
@@ -1353,14 +1358,14 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
qemu_mutex_lock_iothread();
locked = true;
}
- r = memory_region_dispatch_read(mr, mr_offset, &val, op, iotlbentry->attrs);
+ r = memory_region_dispatch_read(mr, mr_offset, &val, op, attrs);
if (r != MEMTX_OK) {
hwaddr physaddr = mr_offset +
section->offset_within_address_space -
section->offset_within_region;
cpu_transaction_failed(cpu, physaddr, addr, memop_size(op), access_type,
- mmu_idx, iotlbentry->attrs, r, retaddr);
+ mmu_idx, attrs, r, retaddr);
}
if (locked) {
qemu_mutex_unlock_iothread();
@@ -1395,8 +1400,13 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
MemoryRegion *mr;
bool locked = false;
MemTxResult r;
+ MemTxAttrs attrs = iotlbentry->attrs;
+
+ /* encode the accessing CPU */
+ attrs.requester_is_cpu = true;
+ attrs.cpu_index = cpu->cpu_index;
- section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+ section = iotlb_to_section(cpu, iotlbentry->addr, attrs);
mr = section->mr;
mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
if (!cpu->can_do_io) {
@@ -1414,14 +1424,14 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
qemu_mutex_lock_iothread();
locked = true;
}
- r = memory_region_dispatch_write(mr, mr_offset, val, op, iotlbentry->attrs);
+ r = memory_region_dispatch_write(mr, mr_offset, val, op, attrs);
if (r != MEMTX_OK) {
hwaddr physaddr = mr_offset +
section->offset_within_address_space -
section->offset_within_region;
cpu_transaction_failed(cpu, physaddr, addr, memop_size(op),
- MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
+ MMU_DATA_STORE, mmu_idx, attrs, r,
retaddr);
}
if (locked) {
diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
index 00253f8929..cdabc577d2 100644
--- a/hw/core/cpu-sysemu.c
+++ b/hw/core/cpu-sysemu.c
@@ -51,13 +51,22 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
MemTxAttrs *attrs)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
+ MemTxAttrs local = { };
+ hwaddr res;
if (cc->sysemu_ops->get_phys_page_attrs_debug) {
- return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
+ res = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, &local);
+ } else {
+ /* Fallback for CPUs which don't implement the _attrs_ hook */
+ local = MEMTXATTRS_UNSPECIFIED;
+ res = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
}
- /* Fallback for CPUs which don't implement the _attrs_ hook */
- *attrs = MEMTXATTRS_UNSPECIFIED;
- return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
+
+ /* debug access is treated as though it came from the CPU */
+ local.requester_is_cpu = 1;
+ local.cpu_index = cpu->cpu_index;
+ *attrs = local;
+ return res;
}
hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs
2022-09-22 14:58 ` [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
@ 2022-09-25 10:08 ` Richard Henderson
2022-09-25 13:02 ` Alex Bennée
0 siblings, 1 reply; 25+ messages in thread
From: Richard Henderson @ 2022-09-25 10:08 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: f4bug, mads, Paolo Bonzini
On 9/22/22 14:58, Alex Bennée wrote:
> We currently have hacks across the hw/ to reference current_cpu to
> work out what the current accessing CPU is. This breaks in some cases
> including using gdbstub to access HW state. As we have MemTxAttrs to
> describe details about the access lets extend it to mention if this is
> a CPU access and which one it is.
>
> There are a number of places we need to fix up including:
>
> CPU helpers directly calling address_space_*() fns
> models in hw/ fishing the data out of current_cpu
>
> I'll start addressing some of these in following patches.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
> - use separate field cpu_index
> - bool for requester_is_cpu
> ---
> include/exec/memattrs.h | 4 ++++
> accel/tcg/cputlb.c | 22 ++++++++++++++++------
> hw/core/cpu-sysemu.c | 17 +++++++++++++----
> 3 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..e83a993c21 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -43,6 +43,10 @@ typedef struct MemTxAttrs {
> * (see MEMTX_ACCESS_ERROR).
> */
> unsigned int memory:1;
> + /* Requester is CPU (or as CPU, e.g. debug) */
> + bool requester_is_cpu:1;
> + /* cpu_index (if requester_is_cpu) */
> + unsigned int cpu_index:16;
> /* Requester ID (for MSI for example) */
> unsigned int requester_id:16;
I'm not keen on adding another field like this.
I don't think it addresses Peter's point about unique identifiers on e.g. the MSI bus.
But addressing that is surely an problem for any host/pci bridge that supports PCI.
Because we're already talking about two different busses -- PCI, and the one between the
cpu and the bridge.
What bounds our max number of cpus at the moment? We use "int" in struct CPUCore, but
that's almost certainly for convenience.
target/s390x/cpu.h:#define S390_MAX_CPUS 248
hw/i386/pc_piix.c: m->max_cpus = HVM_MAX_VCPUS;
hw/i386/pc_q35.c: m->max_cpus = 288;
hw/arm/virt.c: mc->max_cpus = 512;
hw/arm/sbsa-ref.c: mc->max_cpus = 512;
hw/i386/microvm.c: mc->max_cpus = 288;
hw/ppc/spapr.c: mc->max_cpus = INT32_MAX;
Most of these are nicely bounded, but HVM_MAX_VCPUS is a magic number from Xen, and ppc
appears to be prepared for 31 bits worth of cpus.
> @@ -1340,8 +1340,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
> uint64_t val;
> bool locked = false;
> MemTxResult r;
> + MemTxAttrs attrs = iotlbentry->attrs;
>
> - section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
> + /* encode the accessing CPU */
> + attrs.requester_is_cpu = 1;
> + attrs.cpu_index = cpu->cpu_index;
As I said before, we cannot set these generically, or MEMTXATTRS_UNSPECIFIED means
nothing. Furthermore, they should be set at the point we create the tlb entry, not while
we're reading it. Thus this must be done by each target's tlb_fill function.
> @@ -51,13 +51,22 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
> MemTxAttrs *attrs)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> + MemTxAttrs local = { };
> + hwaddr res;
>
> if (cc->sysemu_ops->get_phys_page_attrs_debug) {
> - return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
> + res = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, &local);
> + } else {
> + /* Fallback for CPUs which don't implement the _attrs_ hook */
> + local = MEMTXATTRS_UNSPECIFIED;
> + res = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
> }
> - /* Fallback for CPUs which don't implement the _attrs_ hook */
> - *attrs = MEMTXATTRS_UNSPECIFIED;
> - return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
> +
> + /* debug access is treated as though it came from the CPU */
> + local.requester_is_cpu = 1;
> + local.cpu_index = cpu->cpu_index;
> + *attrs = local;
> + return res;
Again, I don't think it makes any sense to have .unspecified set, and anything else non-zero.
r~
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs
2022-09-25 10:08 ` Richard Henderson
@ 2022-09-25 13:02 ` Alex Bennée
2022-09-26 7:33 ` Richard Henderson
0 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-25 13:02 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, f4bug, mads, Paolo Bonzini
Richard Henderson <richard.henderson@linaro.org> writes:
> On 9/22/22 14:58, Alex Bennée wrote:
>> We currently have hacks across the hw/ to reference current_cpu to
>> work out what the current accessing CPU is. This breaks in some cases
>> including using gdbstub to access HW state. As we have MemTxAttrs to
>> describe details about the access lets extend it to mention if this is
>> a CPU access and which one it is.
>> There are a number of places we need to fix up including:
>> CPU helpers directly calling address_space_*() fns
>> models in hw/ fishing the data out of current_cpu
>> I'll start addressing some of these in following patches.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> v2
>> - use separate field cpu_index
>> - bool for requester_is_cpu
>> ---
>> include/exec/memattrs.h | 4 ++++
>> accel/tcg/cputlb.c | 22 ++++++++++++++++------
>> hw/core/cpu-sysemu.c | 17 +++++++++++++----
>> 3 files changed, 33 insertions(+), 10 deletions(-)
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index 9fb98bc1ef..e83a993c21 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -43,6 +43,10 @@ typedef struct MemTxAttrs {
>> * (see MEMTX_ACCESS_ERROR).
>> */
>> unsigned int memory:1;
>> + /* Requester is CPU (or as CPU, e.g. debug) */
>> + bool requester_is_cpu:1;
>> + /* cpu_index (if requester_is_cpu) */
>> + unsigned int cpu_index:16;
>> /* Requester ID (for MSI for example) */
>> unsigned int requester_id:16;
>
> I'm not keen on adding another field like this.
Hmm I thought it was unavoidable from Edgar's comment:
"CPU's can also have a Master-IDs (Requester IDs) which are unrelated to
the Clusters CPU index. This is used for example in the Xilinx ZynqMP
and Xilinx Versal and the XMPU (Memory Protection Units).
Anyway, I think this approach is an improvement from the current state
but would rather see a new separate field from requester_id. Overloading
requester_id will break some of our use-cases (in the Xilinx tree)..."
Of course we don't have to care about external use cases but it seemed
to indicate we might need both.
> I don't think it addresses Peter's point about unique identifiers on
> e.g. the MSI bus. But addressing that is surely an problem for any
> host/pci bridge that supports PCI. Because we're already talking about
> two different busses -- PCI, and the one between the cpu and the
> bridge.
We can return to overloading requester_id with a enum to indicate the
type of bus.
> What bounds our max number of cpus at the moment? We use "int" in
> struct CPUCore, but that's almost certainly for convenience.
>
> target/s390x/cpu.h:#define S390_MAX_CPUS 248
> hw/i386/pc_piix.c: m->max_cpus = HVM_MAX_VCPUS;
>
> hw/i386/pc_q35.c: m->max_cpus = 288;
>
> hw/arm/virt.c: mc->max_cpus = 512;
>
> hw/arm/sbsa-ref.c: mc->max_cpus = 512;
>
> hw/i386/microvm.c: mc->max_cpus = 288;
>
> hw/ppc/spapr.c: mc->max_cpus = INT32_MAX;
>
>
> Most of these are nicely bounded, but HVM_MAX_VCPUS is a magic number
> from Xen, and ppc appears to be prepared for 31 bits worth of cpus.
From 5642e4513e (spapr.c: do not use MachineClass::max_cpus to limit
CPUs) I think it is being a little optimistic. Even with the beefiest
hosts you start to see diminishing returns by ~12 vCPUs and it won't
take long before each extra vCPU just slows you down.
>
>
>> @@ -1340,8 +1340,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>> uint64_t val;
>> bool locked = false;
>> MemTxResult r;
>> + MemTxAttrs attrs = iotlbentry->attrs;
>> - section = iotlb_to_section(cpu, iotlbentry->addr,
>> iotlbentry->attrs);
>> + /* encode the accessing CPU */
>> + attrs.requester_is_cpu = 1;
>> + attrs.cpu_index = cpu->cpu_index;
>
>
> As I said before, we cannot set these generically, or
> MEMTXATTRS_UNSPECIFIED means nothing. Furthermore, they should be set
> at the point we create the tlb entry, not while we're reading it.
> Thus this must be done by each target's tlb_fill function.
I was confused by the last comment because I forgot the TLBs are not
shared between cores. So I can just bang:
MemTxAttrs attrs = { .cpu_index = cs->cpu_index };
into arm_cpu_tlb_fill and be done with it? Or only when we know it is an
IOTLB being filled?
>
>> @@ -51,13 +51,22 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>> MemTxAttrs *attrs)
>> {
>> CPUClass *cc = CPU_GET_CLASS(cpu);
>> + MemTxAttrs local = { };
>> + hwaddr res;
>> if (cc->sysemu_ops->get_phys_page_attrs_debug) {
>> - return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
>> + res = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, &local);
>> + } else {
>> + /* Fallback for CPUs which don't implement the _attrs_ hook */
>> + local = MEMTXATTRS_UNSPECIFIED;
>> + res = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>> }
>> - /* Fallback for CPUs which don't implement the _attrs_ hook */
>> - *attrs = MEMTXATTRS_UNSPECIFIED;
>> - return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>> +
>> + /* debug access is treated as though it came from the CPU */
>> + local.requester_is_cpu = 1;
>> + local.cpu_index = cpu->cpu_index;
>> + *attrs = local;
>> + return res;
>
> Again, I don't think it makes any sense to have .unspecified set, and anything else non-zero.
>
>
> r~
--
Alex Bennée
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs
2022-09-25 13:02 ` Alex Bennée
@ 2022-09-26 7:33 ` Richard Henderson
0 siblings, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2022-09-26 7:33 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, Paolo Bonzini
On 9/25/22 13:02, Alex Bennée wrote:
>> I'm not keen on adding another field like this.
>
> Hmm I thought it was unavoidable from Edgar's comment:
>
> "CPU's can also have a Master-IDs (Requester IDs) which are unrelated to
> the Clusters CPU index. This is used for example in the Xilinx ZynqMP
> and Xilinx Versal and the XMPU (Memory Protection Units).
>
> Anyway, I think this approach is an improvement from the current state
> but would rather see a new separate field from requester_id. Overloading
> requester_id will break some of our use-cases (in the Xilinx tree)..."
>
> Of course we don't have to care about external use cases but it seemed
> to indicate we might need both.
I missed that one.
>> What bounds our max number of cpus at the moment? We use "int" in
>> struct CPUCore, but that's almost certainly for convenience.
>>
>> target/s390x/cpu.h:#define S390_MAX_CPUS 248
>> hw/i386/pc_piix.c: m->max_cpus = HVM_MAX_VCPUS;
>>
>> hw/i386/pc_q35.c: m->max_cpus = 288;
>>
>> hw/arm/virt.c: mc->max_cpus = 512;
>>
>> hw/arm/sbsa-ref.c: mc->max_cpus = 512;
>>
>> hw/i386/microvm.c: mc->max_cpus = 288;
>>
>> hw/ppc/spapr.c: mc->max_cpus = INT32_MAX;
>>
>>
>> Most of these are nicely bounded, but HVM_MAX_VCPUS is a magic number
>> from Xen, and ppc appears to be prepared for 31 bits worth of cpus.
>
> From 5642e4513e (spapr.c: do not use MachineClass::max_cpus to limit
> CPUs) I think it is being a little optimistic. Even with the beefiest
> hosts you start to see diminishing returns by ~12 vCPUs and it won't
> take long before each extra vCPU just slows you down.
Ok, I guess. If we decided to add an assert that the cpuid fit in this field, we'd want
to revisit this, I think. Not an issue for the moment.
> I was confused by the last comment because I forgot the TLBs are not
> shared between cores. So I can just bang:
>
> MemTxAttrs attrs = { .cpu_index = cs->cpu_index };
>
> into arm_cpu_tlb_fill and be done with it?
Yes, it looks like it. I don't see any bulk overwrite of attrs in get_phys_addr and
subroutines. Mostly modifications of attrs->secure, as expected.
r~
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v1 2/9] qtest: make read/write operation appear to be from CPU
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
2022-09-22 14:58 ` [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-22 14:58 ` [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
` (6 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, 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.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
v2
- use a common macro instead of specific MEMTXATTRS_QTEST
---
include/exec/memattrs.h | 4 ++++
softmmu/qtest.c | 26 +++++++++++++-------------
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index e83a993c21..021b68dd06 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -70,6 +70,10 @@ typedef struct MemTxAttrs {
*/
#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
+/* Helper for setting a basic CPU id */
+#define MEMTXATTRS_CPU(id) ((MemTxAttrs) \
+ {.requester_is_cpu = true, .cpu_index = id})
+
/* New-style MMIO accessors can indicate that the transaction failed.
* A zero (MEMTX_OK) response means success; anything else is a failure
* of some kind. The memory subsystem will bitwise-OR together results
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8acef2628..3aa2218b95 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->cpu_index),
&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->cpu_index),
&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->cpu_index),
&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->cpu_index),
&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->cpu_index),
&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->cpu_index),
&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->cpu_index),
&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->cpu_index),
&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->cpu_index), 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->cpu_index), 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->cpu_index), 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->cpu_index),
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->cpu_index), data,
len);
qtest_send_prefix(chr);
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
2022-09-22 14:58 ` [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
2022-09-22 14:58 ` [PATCH v1 2/9] qtest: make read/write operation appear to be from CPU Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-25 10:11 ` Richard Henderson
2022-09-26 10:56 ` mads
2022-09-22 14:58 ` [PATCH v1 4/9] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
` (5 subsequent siblings)
8 siblings, 2 replies; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, Alex Bennée, Peter Maydell, open list:ARM cores
Now that MxTxAttrs encodes a CPU we should use that to figure it out.
This solves edge cases like accessing via gdbstub or qtest.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
---
v2
- update for new field
- bool asserts
---
hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 492b2421ab..b58d3c4a95 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
};
-static inline int gic_get_current_cpu(GICState *s)
+static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
{
- if (!qtest_enabled() && s->num_cpu > 1) {
- return current_cpu->cpu_index;
- }
- return 0;
+ /*
+ * Something other than a CPU accessing the GIC would be a bug as
+ * would a CPU index higher than the GICState expects to be
+ * handling
+ */
+ g_assert(attrs.requester_is_cpu);
+ g_assert(attrs.cpu_index < s->num_cpu);
+
+ return attrs.requester_id;
}
-static inline int gic_get_current_vcpu(GICState *s)
+static inline int gic_get_current_vcpu(GICState *s, MemTxAttrs attrs)
{
- return gic_get_current_cpu(s) + GIC_NCPU;
+ return gic_get_current_cpu(s, attrs) + GIC_NCPU;
}
/* Return true if this GIC config has interrupt groups, which is
@@ -951,7 +956,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
int cm;
int mask;
- cpu = gic_get_current_cpu(s);
+ cpu = gic_get_current_cpu(s, attrs);
cm = 1 << cpu;
if (offset < 0x100) {
if (offset == 0) { /* GICD_CTLR */
@@ -1182,7 +1187,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
int i;
int cpu;
- cpu = gic_get_current_cpu(s);
+ cpu = gic_get_current_cpu(s, attrs);
if (offset < 0x100) {
if (offset == 0) {
if (s->security_extn && !attrs.secure) {
@@ -1476,7 +1481,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
int mask;
int target_cpu;
- cpu = gic_get_current_cpu(s);
+ cpu = gic_get_current_cpu(s, attrs);
irq = value & 0xf;
switch ((value >> 24) & 3) {
case 0:
@@ -1780,7 +1785,7 @@ static MemTxResult gic_thiscpu_read(void *opaque, hwaddr addr, uint64_t *data,
unsigned size, MemTxAttrs attrs)
{
GICState *s = (GICState *)opaque;
- return gic_cpu_read(s, gic_get_current_cpu(s), addr, data, attrs);
+ return gic_cpu_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
}
static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
@@ -1788,7 +1793,7 @@ static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
MemTxAttrs attrs)
{
GICState *s = (GICState *)opaque;
- return gic_cpu_write(s, gic_get_current_cpu(s), addr, value, attrs);
+ return gic_cpu_write(s, gic_get_current_cpu(s, attrs), addr, value, attrs);
}
/* Wrappers to read/write the GIC CPU interface for a specific CPU.
@@ -1818,7 +1823,7 @@ static MemTxResult gic_thisvcpu_read(void *opaque, hwaddr addr, uint64_t *data,
{
GICState *s = (GICState *)opaque;
- return gic_cpu_read(s, gic_get_current_vcpu(s), addr, data, attrs);
+ return gic_cpu_read(s, gic_get_current_vcpu(s, attrs), addr, data, attrs);
}
static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
@@ -1827,7 +1832,7 @@ static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
{
GICState *s = (GICState *)opaque;
- return gic_cpu_write(s, gic_get_current_vcpu(s), addr, value, attrs);
+ return gic_cpu_write(s, gic_get_current_vcpu(s, attrs), addr, value, attrs);
}
static uint32_t gic_compute_eisr(GICState *s, int cpu, int lr_start)
@@ -1860,7 +1865,7 @@ static uint32_t gic_compute_elrsr(GICState *s, int cpu, int lr_start)
static void gic_vmcr_write(GICState *s, uint32_t value, MemTxAttrs attrs)
{
- int vcpu = gic_get_current_vcpu(s);
+ int vcpu = gic_get_current_vcpu(s, attrs);
uint32_t ctlr;
uint32_t abpr;
uint32_t bpr;
@@ -1995,7 +2000,7 @@ static MemTxResult gic_thiscpu_hyp_read(void *opaque, hwaddr addr, uint64_t *dat
{
GICState *s = (GICState *)opaque;
- return gic_hyp_read(s, gic_get_current_cpu(s), addr, data, attrs);
+ return gic_hyp_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
}
static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
@@ -2004,7 +2009,7 @@ static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
{
GICState *s = (GICState *)opaque;
- return gic_hyp_write(s, gic_get_current_cpu(s), addr, value, attrs);
+ return gic_hyp_write(s, gic_get_current_cpu(s, attrs), addr, value, attrs);
}
static MemTxResult gic_do_hyp_read(void *opaque, hwaddr addr, uint64_t *data,
--
2.34.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-22 14:58 ` [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
@ 2022-09-25 10:11 ` Richard Henderson
2022-09-26 10:56 ` mads
1 sibling, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2022-09-25 10:11 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: f4bug, mads, Peter Maydell, open list:ARM cores
On 9/22/22 14:58, Alex Bennée wrote:
> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
> This solves edge cases like accessing via gdbstub or qtest.
>
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/124
>
> ---
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-22 14:58 ` [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
2022-09-25 10:11 ` Richard Henderson
@ 2022-09-26 10:56 ` mads
2022-09-26 11:01 ` Alex Bennée
1 sibling, 1 reply; 25+ messages in thread
From: mads @ 2022-09-26 10:56 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, Peter Maydell, open list:ARM cores
> On 22 Sep 2022, at 16.58, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
> This solves edge cases like accessing via gdbstub or qtest.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
>
> ---
> v2
> - update for new field
> - bool asserts
> ---
> hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 492b2421ab..b58d3c4a95 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
> 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
> };
>
> -static inline int gic_get_current_cpu(GICState *s)
> +static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
> {
> - if (!qtest_enabled() && s->num_cpu > 1) {
> - return current_cpu->cpu_index;
> - }
> - return 0;
> + /*
> + * Something other than a CPU accessing the GIC would be a bug as
> + * would a CPU index higher than the GICState expects to be
> + * handling
> + */
> + g_assert(attrs.requester_is_cpu);
> + g_assert(attrs.cpu_index < s->num_cpu);
> +
> + return attrs.requester_id;
> }
The asserts here abort on macOS, with HVF accelerator:
ERROR:../hw/intc/arm_gic.c:66:gic_get_current_cpu: assertion failed: (attrs.requester_is_cpu)
Bail out! ERROR:../hw/intc/arm_gic.c:66:gic_get_current_cpu: assertion failed: (attrs.requester_is_cpu)
If I revert the changes inside this function, it seemingly works again.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 10:56 ` mads
@ 2022-09-26 11:01 ` Alex Bennée
2022-09-26 11:20 ` mads
0 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-26 11:01 UTC (permalink / raw)
To: mads; +Cc: qemu-devel, f4bug, Peter Maydell, open list:ARM cores
mads@ynddal.dk writes:
>> On 22 Sep 2022, at 16.58, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
>> This solves edge cases like accessing via gdbstub or qtest.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
>>
>> ---
>> v2
>> - update for new field
>> - bool asserts
>> ---
>> hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
>> 1 file changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 492b2421ab..b58d3c4a95 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
>> 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>> };
>>
>> -static inline int gic_get_current_cpu(GICState *s)
>> +static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
>> {
>> - if (!qtest_enabled() && s->num_cpu > 1) {
>> - return current_cpu->cpu_index;
>> - }
>> - return 0;
>> + /*
>> + * Something other than a CPU accessing the GIC would be a bug as
>> + * would a CPU index higher than the GICState expects to be
>> + * handling
>> + */
>> + g_assert(attrs.requester_is_cpu);
>> + g_assert(attrs.cpu_index < s->num_cpu);
>> +
>> + return attrs.requester_id;
>> }
>
> The asserts here abort on macOS, with HVF accelerator:
>
> ERROR:../hw/intc/arm_gic.c:66:gic_get_current_cpu: assertion failed: (attrs.requester_is_cpu)
> Bail out! ERROR:../hw/intc/arm_gic.c:66:gic_get_current_cpu: assertion failed: (attrs.requester_is_cpu)
>
> If I revert the changes inside this function, it seemingly works
> again.
Thanks for testing.
I guess this is because the we have a soft GIC for HVF. Somewhere in the
hvf code path we must encode up an MemTxAttrs when the gic is accessed.
Could you try in the EC_DATAABORT path in
target/arm/hvf/hvf.c:hvf_vcpu_exec:
if (iswrite) {
val = hvf_get_reg(cpu, srt);
address_space_write(&address_space_memory,
hvf_exit->exception.physical_address,
MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
} else {
address_space_read(&address_space_memory,
hvf_exit->exception.physical_address,
MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
hvf_set_reg(cpu, srt, val);
}
if that works I'll cook up a proper patch.
--
Alex Bennée
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 11:01 ` Alex Bennée
@ 2022-09-26 11:20 ` mads
0 siblings, 0 replies; 25+ messages in thread
From: mads @ 2022-09-26 11:20 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, Peter Maydell, open list:ARM cores
>>> On 22 Sep 2022, at 16.58, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> Now that MxTxAttrs encodes a CPU we should use that to figure it out.
>>> This solves edge cases like accessing via gdbstub or qtest.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124
>>>
>>> ---
>>> v2
>>> - update for new field
>>> - bool asserts
>>> ---
>>> hw/intc/arm_gic.c | 39 ++++++++++++++++++++++-----------------
>>> 1 file changed, 22 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index 492b2421ab..b58d3c4a95 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -56,17 +56,22 @@ static const uint8_t gic_id_gicv2[] = {
>>> 0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> };
>>>
>>> -static inline int gic_get_current_cpu(GICState *s)
>>> +static inline int gic_get_current_cpu(GICState *s, MemTxAttrs attrs)
>>> {
>>> - if (!qtest_enabled() && s->num_cpu > 1) {
>>> - return current_cpu->cpu_index;
>>> - }
>>> - return 0;
>>> + /*
>>> + * Something other than a CPU accessing the GIC would be a bug as
>>> + * would a CPU index higher than the GICState expects to be
>>> + * handling
>>> + */
>>> + g_assert(attrs.requester_is_cpu);
>>> + g_assert(attrs.cpu_index < s->num_cpu);
>>> +
>>> + return attrs.requester_id;
>>> }
>>
>> The asserts here abort on macOS, with HVF accelerator:
>>
>> ERROR:../hw/intc/arm_gic.c:66:gic_get_current_cpu: assertion failed: (attrs.requester_is_cpu)
>> Bail out! ERROR:../hw/intc/arm_gic.c:66:gic_get_current_cpu: assertion failed: (attrs.requester_is_cpu)
>>
>> If I revert the changes inside this function, it seemingly works
>> again.
>
> Thanks for testing.
>
> I guess this is because the we have a soft GIC for HVF. Somewhere in the
> hvf code path we must encode up an MemTxAttrs when the gic is accessed.
>
> Could you try in the EC_DATAABORT path in
> target/arm/hvf/hvf.c:hvf_vcpu_exec:
>
> if (iswrite) {
> val = hvf_get_reg(cpu, srt);
> address_space_write(&address_space_memory,
> hvf_exit->exception.physical_address,
> MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
> } else {
> address_space_read(&address_space_memory,
> hvf_exit->exception.physical_address,
> MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
> hvf_set_reg(cpu, srt, val);
> }
>
> if that works I'll cook up a proper patch.
>
> --
> Alex Bennée
Perfect. This fixes the issue.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v1 4/9] hw/timer: convert mptimer access to attrs to derive cpu index
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
` (2 preceding siblings ...)
2022-09-22 14:58 ` [PATCH v1 3/9] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-25 10:11 ` Richard Henderson
2022-09-22 14:58 ` [PATCH v1 5/9] configure: move detected gdb to TCG's config-host.mak Alex Bennée
` (4 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, Alex Bennée, Peter Maydell, open list:ARM cores
This removes the hacks to deal with empty current_cpu.
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..b48536f6a3 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.cpu_index;
+ g_assert(attrs.requester_is_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] 25+ messages in thread* [PATCH v1 5/9] configure: move detected gdb to TCG's config-host.mak
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
` (3 preceding siblings ...)
2022-09-22 14:58 ` [PATCH v1 4/9] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-25 10:11 ` Richard Henderson
2022-09-22 14:58 ` [PATCH v1 6/9] gdbstub: move into its own sub directory Alex Bennée
` (3 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, mads, Alex Bennée
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)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
configure | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/configure b/configure
index 0bbf9d28af..fce677bd4a 100755
--- a/configure
+++ b/configure
@@ -2481,6 +2481,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
@@ -2565,6 +2567,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] 25+ messages in thread* [PATCH v1 6/9] gdbstub: move into its own sub directory
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
` (4 preceding siblings ...)
2022-09-22 14:58 ` [PATCH v1 5/9] configure: move detected gdb to TCG's config-host.mak Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-25 10:13 ` Richard Henderson
2022-09-22 14:58 ` [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass Alex Bennée
` (2 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, mads, Alex Bennée, 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.
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] 25+ messages in thread* Re: [PATCH v1 6/9] gdbstub: move into its own sub directory
2022-09-22 14:58 ` [PATCH v1 6/9] gdbstub: move into its own sub directory Alex Bennée
@ 2022-09-25 10:13 ` Richard Henderson
0 siblings, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2022-09-25 10:13 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: f4bug, mads, Stefan Hajnoczi
On 9/22/22 14:58, 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.
>
> 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: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
` (5 preceding siblings ...)
2022-09-22 14:58 ` [PATCH v1 6/9] gdbstub: move into its own sub directory Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-22 21:49 ` Philippe Mathieu-Daudé via
2022-09-25 10:14 ` Richard Henderson
2022-09-22 14:58 ` [PATCH v1 8/9] gdbstub: move breakpoint logic to accel ops Alex Bennée
2022-09-22 14:58 ` [PATCH v1 9/9] gdbstub: move guest debug support check to ops Alex Bennée
8 siblings, 2 replies; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, Alex Bennée, Richard Henderson, 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.
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] 25+ messages in thread* Re: [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass
2022-09-22 14:58 ` [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass Alex Bennée
@ 2022-09-22 21:49 ` Philippe Mathieu-Daudé via
2022-09-25 10:14 ` Richard Henderson
1 sibling, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-22 21:49 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: mads, Richard Henderson, Paolo Bonzini, open list:Overall KVM CPUs
On 22/9/22 16:58, Alex Bennée wrote:
> 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.
>
> 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: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass
2022-09-22 14:58 ` [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass Alex Bennée
2022-09-22 21:49 ` Philippe Mathieu-Daudé via
@ 2022-09-25 10:14 ` Richard Henderson
1 sibling, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2022-09-25 10:14 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: f4bug, mads, Paolo Bonzini, open list:Overall KVM CPUs
On 9/22/22 14:58, Alex Bennée wrote:
> 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.
>
> 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: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v1 8/9] gdbstub: move breakpoint logic to accel ops
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
` (6 preceding siblings ...)
2022-09-22 14:58 ` [PATCH v1 7/9] gdbstub: move sstep flags probing into AccelClass Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-25 10:18 ` Richard Henderson
2022-09-22 14:58 ` [PATCH v1 9/9] gdbstub: move guest debug support check to ops Alex Bennée
8 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, Alex Bennée, Paolo Bonzini, Richard Henderson,
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.
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] 25+ messages in thread* Re: [PATCH v1 8/9] gdbstub: move breakpoint logic to accel ops
2022-09-22 14:58 ` [PATCH v1 8/9] gdbstub: move breakpoint logic to accel ops Alex Bennée
@ 2022-09-25 10:18 ` Richard Henderson
0 siblings, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2022-09-25 10:18 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: f4bug, mads, Paolo Bonzini, open list:Overall KVM CPUs
On 9/22/22 14:58, Alex Bennée wrote:
> 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.
>
> 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
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v1 9/9] gdbstub: move guest debug support check to ops
2022-09-22 14:58 [PATCH v1 0/9] MemTxAttrs cpu_index and gdbstub/next Alex Bennée
` (7 preceding siblings ...)
2022-09-22 14:58 ` [PATCH v1 8/9] gdbstub: move breakpoint logic to accel ops Alex Bennée
@ 2022-09-22 14:58 ` Alex Bennée
2022-09-22 21:51 ` Philippe Mathieu-Daudé via
2022-09-25 10:18 ` Richard Henderson
8 siblings, 2 replies; 25+ messages in thread
From: Alex Bennée @ 2022-09-22 14:58 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, Alex Bennée, 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.
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] 25+ messages in thread* Re: [PATCH v1 9/9] gdbstub: move guest debug support check to ops
2022-09-22 14:58 ` [PATCH v1 9/9] gdbstub: move guest debug support check to ops Alex Bennée
@ 2022-09-22 21:51 ` Philippe Mathieu-Daudé via
2022-09-25 10:18 ` Richard Henderson
1 sibling, 0 replies; 25+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-22 21:51 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: mads, Paolo Bonzini, Richard Henderson, open list:Overall KVM CPUs
On 22/9/22 16:58, Alex Bennée wrote:
> This removes the final hard coding of kvm_enabled() in gdbstub and
> moves the check to an AccelOps.
>
> 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: Philippe Mathieu-Daudé <f4bug@amsat.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v1 9/9] gdbstub: move guest debug support check to ops
2022-09-22 14:58 ` [PATCH v1 9/9] gdbstub: move guest debug support check to ops Alex Bennée
2022-09-22 21:51 ` Philippe Mathieu-Daudé via
@ 2022-09-25 10:18 ` Richard Henderson
1 sibling, 0 replies; 25+ messages in thread
From: Richard Henderson @ 2022-09-25 10:18 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: f4bug, mads, Paolo Bonzini, open list:Overall KVM CPUs
On 9/22/22 14:58, Alex Bennée wrote:
> This removes the final hard coding of kvm_enabled() in gdbstub and
> moves the check to an AccelOps.
>
> 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: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 25+ messages in thread