* [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
@ 2022-09-26 13:38 ` Alex Bennée
2022-09-26 14:34 ` Peter Maydell
2022-09-26 20:15 ` Alexander Graf
2022-09-26 13:38 ` [PATCH v2 02/11] target/arm: enable tracking of " Alex Bennée
` (9 subsequent siblings)
10 siblings, 2 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:38 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, mads, qemu-arm, Alex Bennée
We currently have hacks across the hw/ to reference current_cpu to
work out what the current accessing CPU is. This breaks in some cases
including using gdbstub to access HW state. As we have MemTxAttrs to
describe details about the access lets extend it to mention if this is
a CPU access and which one it is.
There are a number of places we need to fix up including:
CPU helpers directly calling address_space_*() fns
models in hw/ fishing the data out of current_cpu
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
---
include/exec/memattrs.h | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..0fb5f29d25 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -14,6 +14,15 @@
#ifndef MEMATTRS_H
#define MEMATTRS_H
+/*
+ * Where the memory transaction comes from
+ */
+typedef enum MemTxRequesterType {
+ MEMTXATTRS_CPU,
+ MEMTXATTRS_MSI,
+} 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
@@ -43,7 +52,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;
@@ -66,6 +77,10 @@ typedef struct MemTxAttrs {
*/
#define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
+/* Helper for setting a basic CPU sourced transaction */
+#define MEMTXATTRS_CPU(id) ((MemTxAttrs) \
+ {.requester_type = MEMTXATTRS_CPU, .requester_id = id})
+
/* New-style MMIO accessors can indicate that the transaction failed.
* A zero (MEMTX_OK) response means success; anything else is a failure
* of some kind. The memory subsystem will bitwise-OR together results
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs
2022-09-26 13:38 ` [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
@ 2022-09-26 14:34 ` Peter Maydell
2022-09-26 15:09 ` Alex Bennée
2022-09-26 20:15 ` Alexander Graf
1 sibling, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 14:34 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm, Edgar E. Iglesias
On Mon, 26 Sept 2022 at 15:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> We currently have hacks across the hw/ to reference current_cpu to
> work out what the current accessing CPU is. This breaks in some cases
> including using gdbstub to access HW state. As we have MemTxAttrs to
> describe details about the access lets extend it to mention if this is
> a CPU access and which one it is.
>
> There are a number of places we need to fix up including:
>
> CPU helpers directly calling address_space_*() fns
> models in hw/ fishing the data out of current_cpu
> 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
I still don't see anything in this patchset that updates
the code which currently assumes requester_id to be a PCI
index to check that it hasn't been handed a MemTxAttrs
that uses requester_id as a CPU number.
I also still need to go and look up how hardware does this,
so please don't queue this patchset yet. In particular, we
should think about whether we want this to be:
* a CPU number, but only set opt-in by some target archs
* a CPU number, valid for all target archs
* a unique-within-the-machine identifier of the transaction
master (i.e. which can be set by DMA controllers, etc,
not just CPUs)
I would also like some input from Edgar since I know Xilinx
have some more extensive out-of-tree uses of requester_id.
We aren't obligated to not break out-of-tree code, but that
seems like a bunch of experience and knowledge about how
real hardware works that would be useful for informing
how we design this.
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs
2022-09-26 14:34 ` Peter Maydell
@ 2022-09-26 15:09 ` Alex Bennée
2022-09-26 15:30 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 15:09 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, f4bug, mads, qemu-arm, Edgar E. Iglesias
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 26 Sept 2022 at 15:13, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> We currently have hacks across the hw/ to reference current_cpu to
>> work out what the current accessing CPU is. This breaks in some cases
>> including using gdbstub to access HW state. As we have MemTxAttrs to
>> describe details about the access lets extend it to mention if this is
>> a CPU access and which one it is.
>>
>> There are a number of places we need to fix up including:
>>
>> CPU helpers directly calling address_space_*() fns
>> models in hw/ fishing the data out of current_cpu
>> 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
>
> I still don't see anything in this patchset that updates
> the code which currently assumes requester_id to be a PCI
> index to check that it hasn't been handed a MemTxAttrs
> that uses requester_id as a CPU number.
OK I'll update so all the existing cases setting requester_id also set
the type to MEMTXATTRS_MSI.
I also noticed the GIC ITS code checks requester ID. Should we assert
(or hw_error?) if it's not the case?
> I also still need to go and look up how hardware does this,
> so please don't queue this patchset yet. In particular, we
> should think about whether we want this to be:
> * a CPU number, but only set opt-in by some target archs
Given a whole bunch of arches currently use MEMTXATTRS_UNSPECIFIED I
think for now it's worth confining to just ARM where we know we have
devices that care about the cpu_index and have tagged the various paths
with the correct data.
> * a CPU number, valid for all target archs
> * a unique-within-the-machine identifier of the transaction
> master (i.e. which can be set by DMA controllers, etc,
> not just CPUs)
That would require something to keep a map of requester_id's to
source/index right?
> I would also like some input from Edgar since I know Xilinx
> have some more extensive out-of-tree uses of requester_id.
> We aren't obligated to not break out-of-tree code, but that
> seems like a bunch of experience and knowledge about how
> real hardware works that would be useful for informing
> how we design this.
His comment against the last iteration was:
"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)...
IIRC a real GIC differentiates between the connected CPU's through
different ports, not by looking at master-ids but I'm not 100% sure..."
at the same time Richard's not keen about adding extra fields
(especially as some arches have INT32_MAX bounds for cpu_index). However
one approach would be to expand the requester_id field and you could
then expand MemTxRequesterType to and have a multiplexed type although I
admit it's hard to imagine HW that cares about both the CPU and bus id
at the same time.
>
> thanks
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs
2022-09-26 15:09 ` Alex Bennée
@ 2022-09-26 15:30 ` Peter Maydell
0 siblings, 0 replies; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 15:30 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm, Edgar E. Iglesias
On Mon, 26 Sept 2022 at 16:24, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > I still don't see anything in this patchset that updates
> > the code which currently assumes requester_id to be a PCI
> > index to check that it hasn't been handed a MemTxAttrs
> > that uses requester_id as a CPU number.
>
> OK I'll update so all the existing cases setting requester_id also set
> the type to MEMTXATTRS_MSI.
The problem is not the places that set requester_id (you can
arrange for the default to be MSI for back-compat if you don't
want to explicitly set it), but the places that *read* it.
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs
2022-09-26 13:38 ` [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
2022-09-26 14:34 ` Peter Maydell
@ 2022-09-26 20:15 ` Alexander Graf
1 sibling, 0 replies; 28+ messages in thread
From: Alexander Graf @ 2022-09-26 20:15 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: f4bug, mads, qemu-arm
On 26.09.22 15:38, 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
> 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
> ---
> include/exec/memattrs.h | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..0fb5f29d25 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -14,6 +14,15 @@
> #ifndef MEMATTRS_H
> #define MEMATTRS_H
>
> +/*
> + * Where the memory transaction comes from
> + */
> +typedef enum MemTxRequesterType {
> + MEMTXATTRS_CPU,
> + MEMTXATTRS_MSI,
I don't think MSI is any sensible identifier here. What you actually are
trying to describe are bus messages that may carry additional source
information - and what you describe as "MSI" here most likely means "no
further information".
How about you just call it "BUS", "DMA" or literally "UNSPECIFIED" for
now? Also, why is unspecified not part of the requester type enum?
Alex
> +} 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
> @@ -43,7 +52,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;
> @@ -66,6 +77,10 @@ typedef struct MemTxAttrs {
> */
> #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
>
> +/* Helper for setting a basic CPU sourced transaction */
> +#define MEMTXATTRS_CPU(id) ((MemTxAttrs) \
> + {.requester_type = MEMTXATTRS_CPU, .requester_id = id})
> +
> /* New-style MMIO accessors can indicate that the transaction failed.
> * A zero (MEMTX_OK) response means success; anything else is a failure
> * of some kind. The memory subsystem will bitwise-OR together results
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 02/11] target/arm: enable tracking of CPU index in MemTxAttrs
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
2022-09-26 13:38 ` [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
@ 2022-09-26 13:38 ` Alex Bennée
2022-09-26 14:12 ` Peter Maydell
2022-09-26 13:38 ` [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs Alex Bennée
` (8 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:38 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, mads, 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>
---
target/arm/ptw.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 3261039d93..644d450662 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2315,6 +2315,9 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
{
ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(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] 28+ messages in thread* Re: [PATCH v2 02/11] target/arm: enable tracking of CPU index in MemTxAttrs
2022-09-26 13:38 ` [PATCH v2 02/11] target/arm: enable tracking of " Alex Bennée
@ 2022-09-26 14:12 ` Peter Maydell
2022-09-26 15:05 ` Alex Bennée
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 14:12 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm
On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> 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>
> ---
> target/arm/ptw.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 3261039d93..644d450662 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2315,6 +2315,9 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
> {
> ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
>
> + attrs->requester_type = MEMTXATTRS_CPU;
> + attrs->requester_id = env_cpu(env)->cpu_index;
> +
This only catches the case where the memory access is
done via something that does a virtual-to-physical translation.
It misses memory accesses done directly on physical addresses,
such as those in arm_ldl_ptw() and arm_ldq_ptw(), plus various
M-profile specific ones.
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 02/11] target/arm: enable tracking of CPU index in MemTxAttrs
2022-09-26 14:12 ` Peter Maydell
@ 2022-09-26 15:05 ` Alex Bennée
2022-09-26 15:07 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 15:05 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, f4bug, mads, qemu-arm
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> 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>
>> ---
>> target/arm/ptw.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
>> index 3261039d93..644d450662 100644
>> --- a/target/arm/ptw.c
>> +++ b/target/arm/ptw.c
>> @@ -2315,6 +2315,9 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
>> {
>> ARMMMUIdx s1_mmu_idx = stage_1_mmu_idx(mmu_idx);
>>
>> + attrs->requester_type = MEMTXATTRS_CPU;
>> + attrs->requester_id = env_cpu(env)->cpu_index;
>> +
>
> This only catches the case where the memory access is
> done via something that does a virtual-to-physical translation.
> It misses memory accesses done directly on physical addresses,
> such as those in arm_ldl_ptw() and arm_ldq_ptw(), plus various
> M-profile specific ones.
I thought they were just used for the page table walk. Can you place
your page tables aliases with a piece of HW?
>
> thanks
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 02/11] target/arm: enable tracking of CPU index in MemTxAttrs
2022-09-26 15:05 ` Alex Bennée
@ 2022-09-26 15:07 ` Peter Maydell
0 siblings, 0 replies; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 15:07 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm
On Mon, 26 Sept 2022 at 16:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> wrote:
> > This only catches the case where the memory access is
> > done via something that does a virtual-to-physical translation.
> > It misses memory accesses done directly on physical addresses,
> > such as those in arm_ldl_ptw() and arm_ldq_ptw(), plus various
> > M-profile specific ones.
>
> I thought they were just used for the page table walk. Can you place
> your page tables aliases with a piece of HW?
They are just used for the page table walk, but they are
nonetheless still memory transactions initiated by the CPU,
so if we're saying those should be marked up in the
transaction-attributes struct then they should count.
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
2022-09-26 13:38 ` [PATCH v2 01/11] hw: encode accessing CPU index in MemTxAttrs Alex Bennée
2022-09-26 13:38 ` [PATCH v2 02/11] target/arm: enable tracking of " Alex Bennée
@ 2022-09-26 13:38 ` Alex Bennée
2022-09-26 14:10 ` Peter Maydell
2022-09-26 20:19 ` Alexander Graf
2022-09-26 13:38 ` [PATCH v2 04/11] qtest: make read/write operation appear to be from CPU Alex Bennée
` (7 subsequent siblings)
10 siblings, 2 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:38 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, qemu-arm, Alex Bennée, 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).
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mads Ynddal <mads@ynddal.dk>
Cc: Alexander Graf <agraf@csgraf.de>
---
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..13b7971560 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->cpu_index), &val, len);
} else {
address_space_read(&address_space_memory,
hvf_exit->exception.physical_address,
- MEMTXATTRS_UNSPECIFIED, &val, len);
+ MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
hvf_set_reg(cpu, srt, val);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-26 13:38 ` [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs Alex Bennée
@ 2022-09-26 14:10 ` Peter Maydell
2022-09-26 15:46 ` Alex Bennée
2022-09-26 20:19 ` Alexander Graf
1 sibling, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 14:10 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm, Alexander Graf
On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> 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).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> Cc: Alexander Graf <agraf@csgraf.de>
> ---
> 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..13b7971560 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->cpu_index), &val, len);
> } else {
> address_space_read(&address_space_memory,
> hvf_exit->exception.physical_address,
> - MEMTXATTRS_UNSPECIFIED, &val, len);
> + MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
> hvf_set_reg(cpu, srt, val);
> }
Don't we need a similar thing for KVM ? (In that case it's in
the generic code in accel/kvm/kvm-all.c, for the KVM_EXIT_MMIO
handling.)
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-26 14:10 ` Peter Maydell
@ 2022-09-26 15:46 ` Alex Bennée
0 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 15:46 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, f4bug, mads, qemu-arm, Alexander Graf
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> 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).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Mads Ynddal <mads@ynddal.dk>
>> Cc: Alexander Graf <agraf@csgraf.de>
>> ---
>> 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..13b7971560 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->cpu_index), &val, len);
>> } else {
>> address_space_read(&address_space_memory,
>> hvf_exit->exception.physical_address,
>> - MEMTXATTRS_UNSPECIFIED, &val, len);
>> + MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
>> hvf_set_reg(cpu, srt, val);
>> }
>
> Don't we need a similar thing for KVM ? (In that case it's in
> the generic code in accel/kvm/kvm-all.c, for the KVM_EXIT_MMIO
> handling.)
Added to MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
which at least anticipates an out-of-kernel GIC.
>
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs
2022-09-26 13:38 ` [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs Alex Bennée
2022-09-26 14:10 ` Peter Maydell
@ 2022-09-26 20:19 ` Alexander Graf
1 sibling, 0 replies; 28+ messages in thread
From: Alexander Graf @ 2022-09-26 20:19 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: f4bug, mads, qemu-arm, Peter Maydell
On 26.09.22 15:38, 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).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> Cc: Alexander Graf <agraf@csgraf.de>
> ---
> 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..13b7971560 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->cpu_index), &val, len);
I think it would make a safer API if MEMTXATTRS_CPU() would take
CPUState * as argument so you can just pass in cpu here.
For the HVF part however,
Acked-by: Alexander Graf <agraf@csgraf.de>
Alex
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 04/11] qtest: make read/write operation appear to be from CPU
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (2 preceding siblings ...)
2022-09-26 13:38 ` [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs Alex Bennée
@ 2022-09-26 13:38 ` Alex Bennée
2022-09-27 9:45 ` Thomas Huth
2022-09-26 13:38 ` [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
` (6 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:38 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, 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.
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
---
softmmu/qtest.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
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] 28+ messages in thread* Re: [PATCH v2 04/11] qtest: make read/write operation appear to be from CPU
2022-09-26 13:38 ` [PATCH v2 04/11] qtest: make read/write operation appear to be from CPU Alex Bennée
@ 2022-09-27 9:45 ` Thomas Huth
0 siblings, 0 replies; 28+ messages in thread
From: Thomas Huth @ 2022-09-27 9:45 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: f4bug, mads, qemu-arm, Laurent Vivier, Paolo Bonzini
On 26/09/2022 15.38, 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.
s/lets/let's/
> 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
> ---
> softmmu/qtest.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (3 preceding siblings ...)
2022-09-26 13:38 ` [PATCH v2 04/11] qtest: make read/write operation appear to be from CPU Alex Bennée
@ 2022-09-26 13:38 ` Alex Bennée
2022-09-26 14:14 ` Peter Maydell
2022-09-26 13:38 ` [PATCH v2 06/11] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
` (5 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:38 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, 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.
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
---
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..d907df3884 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_type == MEMTXATTRS_CPU);
+ g_assert(attrs.requester_id < s->num_cpu);
+
+ return attrs.requester_id;
}
-static inline int gic_get_current_vcpu(GICState *s)
+static inline int gic_get_current_vcpu(GICState *s, MemTxAttrs attrs)
{
- return gic_get_current_cpu(s) + GIC_NCPU;
+ return gic_get_current_cpu(s, attrs) + GIC_NCPU;
}
/* Return true if this GIC config has interrupt groups, which is
@@ -951,7 +956,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
int cm;
int mask;
- cpu = gic_get_current_cpu(s);
+ cpu = gic_get_current_cpu(s, attrs);
cm = 1 << cpu;
if (offset < 0x100) {
if (offset == 0) { /* GICD_CTLR */
@@ -1182,7 +1187,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
int i;
int cpu;
- cpu = gic_get_current_cpu(s);
+ cpu = gic_get_current_cpu(s, attrs);
if (offset < 0x100) {
if (offset == 0) {
if (s->security_extn && !attrs.secure) {
@@ -1476,7 +1481,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
int mask;
int target_cpu;
- cpu = gic_get_current_cpu(s);
+ cpu = gic_get_current_cpu(s, attrs);
irq = value & 0xf;
switch ((value >> 24) & 3) {
case 0:
@@ -1780,7 +1785,7 @@ static MemTxResult gic_thiscpu_read(void *opaque, hwaddr addr, uint64_t *data,
unsigned size, MemTxAttrs attrs)
{
GICState *s = (GICState *)opaque;
- return gic_cpu_read(s, gic_get_current_cpu(s), addr, data, attrs);
+ return gic_cpu_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
}
static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
@@ -1788,7 +1793,7 @@ static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
MemTxAttrs attrs)
{
GICState *s = (GICState *)opaque;
- return gic_cpu_write(s, gic_get_current_cpu(s), addr, value, attrs);
+ return gic_cpu_write(s, gic_get_current_cpu(s, attrs), addr, value, attrs);
}
/* Wrappers to read/write the GIC CPU interface for a specific CPU.
@@ -1818,7 +1823,7 @@ static MemTxResult gic_thisvcpu_read(void *opaque, hwaddr addr, uint64_t *data,
{
GICState *s = (GICState *)opaque;
- return gic_cpu_read(s, gic_get_current_vcpu(s), addr, data, attrs);
+ return gic_cpu_read(s, gic_get_current_vcpu(s, attrs), addr, data, attrs);
}
static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
@@ -1827,7 +1832,7 @@ static MemTxResult gic_thisvcpu_write(void *opaque, hwaddr addr,
{
GICState *s = (GICState *)opaque;
- return gic_cpu_write(s, gic_get_current_vcpu(s), addr, value, attrs);
+ return gic_cpu_write(s, gic_get_current_vcpu(s, attrs), addr, value, attrs);
}
static uint32_t gic_compute_eisr(GICState *s, int cpu, int lr_start)
@@ -1860,7 +1865,7 @@ static uint32_t gic_compute_elrsr(GICState *s, int cpu, int lr_start)
static void gic_vmcr_write(GICState *s, uint32_t value, MemTxAttrs attrs)
{
- int vcpu = gic_get_current_vcpu(s);
+ int vcpu = gic_get_current_vcpu(s, attrs);
uint32_t ctlr;
uint32_t abpr;
uint32_t bpr;
@@ -1995,7 +2000,7 @@ static MemTxResult gic_thiscpu_hyp_read(void *opaque, hwaddr addr, uint64_t *dat
{
GICState *s = (GICState *)opaque;
- return gic_hyp_read(s, gic_get_current_cpu(s), addr, data, attrs);
+ return gic_hyp_read(s, gic_get_current_cpu(s, attrs), addr, data, attrs);
}
static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
@@ -2004,7 +2009,7 @@ static MemTxResult gic_thiscpu_hyp_write(void *opaque, hwaddr addr,
{
GICState *s = (GICState *)opaque;
- return gic_hyp_write(s, gic_get_current_cpu(s), addr, value, attrs);
+ return gic_hyp_write(s, gic_get_current_cpu(s, attrs), addr, value, attrs);
}
static MemTxResult gic_do_hyp_read(void *opaque, hwaddr addr, uint64_t *data,
--
2.34.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 13:38 ` [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
@ 2022-09-26 14:14 ` Peter Maydell
2022-09-26 15:06 ` Alex Bennée
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 14:14 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm, Richard Henderson
On Mon, 26 Sept 2022 at 14:39, 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.
>
> 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
> ---
> 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..d907df3884 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_type == MEMTXATTRS_CPU);
> + g_assert(attrs.requester_id < s->num_cpu);
Would it be a QEMU bug, or a guest code bug ? If it's possible
for the guest to mis-program a DMA controller to do a read that
goes through this function, we shouldn't assert. (Whether that
can happen will depend on how the board/SoC code puts together
the MemoryRegion hierarchy, I think.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 14:14 ` Peter Maydell
@ 2022-09-26 15:06 ` Alex Bennée
2022-09-26 15:18 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 15:06 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, f4bug, mads, qemu-arm, Richard Henderson
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 26 Sept 2022 at 14:39, 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.
>>
>> 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
>> ---
>> 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..d907df3884 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_type == MEMTXATTRS_CPU);
>> + g_assert(attrs.requester_id < s->num_cpu);
>
> Would it be a QEMU bug, or a guest code bug ? If it's possible
> for the guest to mis-program a DMA controller to do a read that
> goes through this function, we shouldn't assert. (Whether that
> can happen will depend on how the board/SoC code puts together
> the MemoryRegion hierarchy, I think.)
Most likely a QEMU bug - how would a DMA master even access the GIC?
>
> thanks
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 15:06 ` Alex Bennée
@ 2022-09-26 15:18 ` Peter Maydell
2022-09-26 15:41 ` Alex Bennée
0 siblings, 1 reply; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 15:18 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm, Richard Henderson
On Mon, 26 Sept 2022 at 16:08, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> -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_type == MEMTXATTRS_CPU);
> >> + g_assert(attrs.requester_id < s->num_cpu);
> >
> > Would it be a QEMU bug, or a guest code bug ? If it's possible
> > for the guest to mis-program a DMA controller to do a read that
> > goes through this function, we shouldn't assert. (Whether that
> > can happen will depend on how the board/SoC code puts together
> > the MemoryRegion hierarchy, I think.)
>
> Most likely a QEMU bug - how would a DMA master even access the GIC?
If it's mapped into the system address space, the same way
as it does any memory access. For instance on the virt board
we just map the distributor MemoryRegion straight into the
system address space, and any DMA master can talk to it.
This is of course not how the hardware really works (where
the GIC is part of the CPU itself), but, as noted in previous
threads, up-ending the MemoryRegion handling in order to be
able to put the GIC only in the address space(s) that the CPU
sees would be a lot of work, which is why we didn't try to
solve the "how do you figure out which CPU is writing to the
GIC" problem that way.
thanks
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 15:18 ` Peter Maydell
@ 2022-09-26 15:41 ` Alex Bennée
2022-09-26 15:45 ` Peter Maydell
0 siblings, 1 reply; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 15:41 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, f4bug, mads, qemu-arm, Richard Henderson
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 26 Sept 2022 at 16:08, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >> -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_type == MEMTXATTRS_CPU);
>> >> + g_assert(attrs.requester_id < s->num_cpu);
>> >
>> > Would it be a QEMU bug, or a guest code bug ? If it's possible
>> > for the guest to mis-program a DMA controller to do a read that
>> > goes through this function, we shouldn't assert. (Whether that
>> > can happen will depend on how the board/SoC code puts together
>> > the MemoryRegion hierarchy, I think.)
>>
>> Most likely a QEMU bug - how would a DMA master even access the GIC?
>
> If it's mapped into the system address space, the same way
> as it does any memory access. For instance on the virt board
> we just map the distributor MemoryRegion straight into the
> system address space, and any DMA master can talk to it.
> This is of course not how the hardware really works (where
> the GIC is part of the CPU itself), but, as noted in previous
> threads, up-ending the MemoryRegion handling in order to be
> able to put the GIC only in the address space(s) that the CPU
> sees would be a lot of work, which is why we didn't try to
> solve the "how do you figure out which CPU is writing to the
> GIC" problem that way.
So hw_error?
I don't think there is a way we can safely continue unless we just want
to fallback to "it was vCPU 0 what did it".
>
> thanks
> -- PMM
--
Alex Bennée
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU
2022-09-26 15:41 ` Alex Bennée
@ 2022-09-26 15:45 ` Peter Maydell
0 siblings, 0 replies; 28+ messages in thread
From: Peter Maydell @ 2022-09-26 15:45 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, f4bug, mads, qemu-arm, Richard Henderson
On Mon, 26 Sept 2022 at 16:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 26 Sept 2022 at 16:08, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >> > On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> wrote:
> >> >> -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_type == MEMTXATTRS_CPU);
> >> >> + g_assert(attrs.requester_id < s->num_cpu);
> >> >
> >> > Would it be a QEMU bug, or a guest code bug ? If it's possible
> >> > for the guest to mis-program a DMA controller to do a read that
> >> > goes through this function, we shouldn't assert. (Whether that
> >> > can happen will depend on how the board/SoC code puts together
> >> > the MemoryRegion hierarchy, I think.)
> >>
> >> Most likely a QEMU bug - how would a DMA master even access the GIC?
> >
> > If it's mapped into the system address space, the same way
> > as it does any memory access. For instance on the virt board
> > we just map the distributor MemoryRegion straight into the
> > system address space, and any DMA master can talk to it.
> > This is of course not how the hardware really works (where
> > the GIC is part of the CPU itself), but, as noted in previous
> > threads, up-ending the MemoryRegion handling in order to be
> > able to put the GIC only in the address space(s) that the CPU
> > sees would be a lot of work, which is why we didn't try to
> > solve the "how do you figure out which CPU is writing to the
> > GIC" problem that way.
>
> So hw_error?
That's just an assert by another name, and isn't any better.
> I don't think there is a way we can safely continue unless we just want
> to fallback to "it was vCPU 0 what did it".
You can do that, or just make the whole memory transaction
return 0, or return a suitable memtx error.
-- PMM
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 06/11] hw/timer: convert mptimer access to attrs to derive cpu index
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (4 preceding siblings ...)
2022-09-26 13:38 ` [PATCH v2 05/11] hw/intc/gic: use MxTxAttrs to divine accessing CPU Alex Bennée
@ 2022-09-26 13:38 ` Alex Bennée
2022-09-26 13:39 ` [PATCH v2 07/11] configure: move detected gdb to TCG's config-host.mak Alex Bennée
` (4 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:38 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, 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..813ba3b7e8 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 == MEMTXATTRS_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] 28+ messages in thread* [PATCH v2 07/11] configure: move detected gdb to TCG's config-host.mak
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (5 preceding siblings ...)
2022-09-26 13:38 ` [PATCH v2 06/11] hw/timer: convert mptimer access to attrs to derive cpu index Alex Bennée
@ 2022-09-26 13:39 ` Alex Bennée
2022-09-26 13:39 ` [PATCH v2 08/11] gdbstub: move into its own sub directory Alex Bennée
` (3 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:39 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, mads, 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 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] 28+ messages in thread* [PATCH v2 08/11] gdbstub: move into its own sub directory
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (6 preceding siblings ...)
2022-09-26 13:39 ` [PATCH v2 07/11] configure: move detected gdb to TCG's config-host.mak Alex Bennée
@ 2022-09-26 13:39 ` Alex Bennée
2022-09-26 13:39 ` [PATCH v2 09/11] gdbstub: move sstep flags probing into AccelClass Alex Bennée
` (2 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, qemu-arm, Alex Bennée, Richard Henderson,
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] 28+ messages in thread* [PATCH v2 09/11] gdbstub: move sstep flags probing into AccelClass
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (7 preceding siblings ...)
2022-09-26 13:39 ` [PATCH v2 08/11] gdbstub: move into its own sub directory Alex Bennée
@ 2022-09-26 13:39 ` Alex Bennée
2022-09-26 13:39 ` [PATCH v2 10/11] gdbstub: move breakpoint logic to accel ops Alex Bennée
2022-09-26 13:39 ` [PATCH v2 11/11] gdbstub: move guest debug support check to ops Alex Bennée
10 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, qemu-arm, 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.
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] 28+ messages in thread* [PATCH v2 10/11] gdbstub: move breakpoint logic to accel ops
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (8 preceding siblings ...)
2022-09-26 13:39 ` [PATCH v2 09/11] gdbstub: move sstep flags probing into AccelClass Alex Bennée
@ 2022-09-26 13:39 ` Alex Bennée
2022-09-26 13:39 ` [PATCH v2 11/11] gdbstub: move guest debug support check to ops Alex Bennée
10 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, qemu-arm, Alex Bennée, Richard Henderson,
Paolo Bonzini, 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] 28+ messages in thread* [PATCH v2 11/11] gdbstub: move guest debug support check to ops
2022-09-26 13:38 [PATCH v2 00/11] gdbstub/next (MemTxAttrs and re-factoring) Alex Bennée
` (9 preceding siblings ...)
2022-09-26 13:39 ` [PATCH v2 10/11] gdbstub: move breakpoint logic to accel ops Alex Bennée
@ 2022-09-26 13:39 ` Alex Bennée
10 siblings, 0 replies; 28+ messages in thread
From: Alex Bennée @ 2022-09-26 13:39 UTC (permalink / raw)
To: qemu-devel
Cc: f4bug, mads, qemu-arm, 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.
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] 28+ messages in thread