| Message ID | 20220322154213.86475-1-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] gdbstub: Set current_cpu for memory read write | expand |
On Tue, 22 Mar 2022 at 15:43, Bin Meng <bmeng.cn@gmail.com> wrote: > > When accessing the per-CPU register bank of some devices (e.g.: GIC) > from the GDB stub context, a segfault occurs. This is due to current_cpu > is not set, as the contect is not a guest CPU. > > Let's set current_cpu before doing the acutal memory read write. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124 > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- This works, but I worry a bit that it might have unexpected side effects, and setting globals (even if thread-local) to cause side-effects elsewhere isn't ideal... -- PMM
+Thomas On 22/3/22 16:56, Peter Maydell wrote: > On Tue, 22 Mar 2022 at 15:43, Bin Meng <bmeng.cn@gmail.com> wrote: >> >> When accessing the per-CPU register bank of some devices (e.g.: GIC) >> from the GDB stub context, a segfault occurs. This is due to current_cpu >> is not set, as the contect is not a guest CPU. >> >> Let's set current_cpu before doing the acutal memory read write. >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124 >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- > > This works, but I worry a bit that it might have unexpected > side effects, and setting globals (even if thread-local) to > cause side-effects elsewhere isn't ideal... Yeah, gdbstub is like a JTAG probe, CPU accessors/views shouldn't be involved. Having current_cpu==NULL seems the correct behavior. There was a thread few years ago about an issue similar to this one. IIRC it was about how to have qtest commands select a different address space instead of the 'current cpu' one. I wonder why target_memory_rw_debug() involves CPU at all. Maybe it is simply not using the correct API?
On Tue, 22 Mar 2022 at 18:59, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > On 22/3/22 16:56, Peter Maydell wrote: > > This works, but I worry a bit that it might have unexpected > > side effects, and setting globals (even if thread-local) to > > cause side-effects elsewhere isn't ideal... > > Yeah, gdbstub is like a JTAG probe, CPU accessors/views shouldn't be > involved. Having current_cpu==NULL seems the correct behavior. > > There was a thread few years ago about an issue similar to this one. > IIRC it was about how to have qtest commands select a different address > space instead of the 'current cpu' one. > > I wonder why target_memory_rw_debug() involves CPU at all. Maybe it is > simply not using the correct API? gdb lets you look at specific threads (CPUs). When you're looking at a given CPU you want that CPU's view of the world. Most importantly, you want the virtual-to-physical mapping that that CPU currently has set up... -- PMM
On Tue, Mar 22, 2022 at 11:56 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 22 Mar 2022 at 15:43, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > When accessing the per-CPU register bank of some devices (e.g.: GIC) > > from the GDB stub context, a segfault occurs. This is due to current_cpu > > is not set, as the contect is not a guest CPU. > > > > Let's set current_cpu before doing the acutal memory read write. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124 > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > This works, but I worry a bit that it might have unexpected > side effects, and setting globals (even if thread-local) to > cause side-effects elsewhere isn't ideal... > The functions modified are local to the gdbstub or monitor thread, so modifying the thread-local variable should have no side-effects. Regards, Bin
Bin Meng <bmeng.cn@gmail.com> writes: > On Tue, Mar 22, 2022 at 11:56 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Tue, 22 Mar 2022 at 15:43, Bin Meng <bmeng.cn@gmail.com> wrote: >> > >> > When accessing the per-CPU register bank of some devices (e.g.: GIC) >> > from the GDB stub context, a segfault occurs. This is due to current_cpu >> > is not set, as the contect is not a guest CPU. >> > >> > Let's set current_cpu before doing the acutal memory read write. >> > >> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124 >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> > --- >> >> This works, but I worry a bit that it might have unexpected >> side effects, and setting globals (even if thread-local) to >> cause side-effects elsewhere isn't ideal... >> > > The functions modified are local to the gdbstub or monitor thread, so > modifying the thread-local variable should have no side-effects. The functions may be but current_cpu isn't as evidenced by the fact you set it despite passing cpu down the call chain. We have places in the code that assert(current_cpu) because they absolutely be only called in a real vCPU thread and not elsewhere. This loosens that guarantee. I think we need to not use cpu_physical_memory_write (which is explicitly the system address space) but have a function that takes cpu so it can work out the correct address space to you address_space_read/write. If null we could probably reasonably use first_cpu as an approximation.
On Thu, 24 Mar 2022 at 10:33, Alex Bennée <alex.bennee@linaro.org> wrote: > I think we need to not use cpu_physical_memory_write (which is > explicitly the system address space) but have a function that takes cpu > so it can work out the correct address space to you > address_space_read/write. If null we could probably reasonably use > first_cpu as an approximation. It's not sufficient to use address_space_read/write, because the devices in question are written to figure out the accessing CPU using current_cpu, not by having different MemoryRegions in the different per-CPU AddressSpaces. (You can see this because the bug is present in the common "gdb gives us a virtual address" case which goes via cpu_memory_rw_debug() and does thus use address_space_read(), not only in the oddball 'treat addresses from gdb as physaddrs' case.) If we want to fix this without setting current_cpu, then we need to rewrite these devices not to be accessing it, which we could do in one of two ways: * have the devices arrange to put different MRs in the ASes for each CPU (this is possible today but a massive pain as it would likely involve restructuring a lot of board and SoC level code) * have the MemTxAttrs tell the device what the accessing CPU is (probably by extending the requester_id field); this is somewhat analogous to how it happens in some cases on real hardware, where the AXI bus signals include an ID field indicating what initiated the transaction. This feels neater, but it's still quite a bit of work for such an unimportant corner case. Or we could work around things, by requiring that devices that access current_cpu cope with it being NULL in some vaguely plausible way. This means that even when you tell gdb or the monitor "access this address using this CPU" you'll get CPU0's view, of course. Some devices like hw/intc/openpic.c do this already. Or we could continue to ignore the bug, like we've done for the past six years, on the basis that you only hit it if you've done something slightly silly in gdb or the monitor in the first place :-) -- PMM
On Thu, Mar 24, 2022 at 7:52 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 24 Mar 2022 at 10:33, Alex Bennée <alex.bennee@linaro.org> wrote: > > I think we need to not use cpu_physical_memory_write (which is > > explicitly the system address space) but have a function that takes cpu > > so it can work out the correct address space to you > > address_space_read/write. If null we could probably reasonably use > > first_cpu as an approximation. > > It's not sufficient to use address_space_read/write, because the > devices in question are written to figure out the accessing CPU > using current_cpu, not by having different MemoryRegions in the > different per-CPU AddressSpaces. (You can see this because the bug > is present in the common "gdb gives us a virtual address" case which > goes via cpu_memory_rw_debug() and does thus use address_space_read(), > not only in the oddball 'treat addresses from gdb as physaddrs' case.) > > If we want to fix this without setting current_cpu, then we need to > rewrite these devices not to be accessing it, which we could do > in one of two ways: > * have the devices arrange to put different MRs in the ASes > for each CPU (this is possible today but a massive pain as > it would likely involve restructuring a lot of board and > SoC level code) > * have the MemTxAttrs tell the device what the accessing CPU is > (probably by extending the requester_id field); this is > somewhat analogous to how it happens in some cases on real > hardware, where the AXI bus signals include an ID field > indicating what initiated the transaction. This feels neater, > but it's still quite a bit of work for such an unimportant > corner case. > > Or we could work around things, by requiring that devices that > access current_cpu cope with it being NULL in some vaguely > plausible way. This means that even when you tell gdb or the > monitor "access this address using this CPU" you'll get CPU0's > view, of course. Some devices like hw/intc/openpic.c do this already. > > Or we could continue to ignore the bug, like we've done for the > past six years, on the basis that you only hit it if you've > done something slightly silly in gdb or the monitor in > the first place :-) IMHO it's too bad to just ignore this bug forever. This is a valid use case. It's not about whether we intentionally want to inspect the GIC register value from gdb. The case is that when single stepping the source codes it triggers the core dump for no reason if the instructions involved contain load/store to any of the GIC registers. Regards, Bin
On Mon, 28 Mar 2022 at 03:10, Bin Meng <bmeng.cn@gmail.com> wrote: > IMHO it's too bad to just ignore this bug forever. > > This is a valid use case. It's not about whether we intentionally want > to inspect the GIC register value from gdb. The case is that when > single stepping the source codes it triggers the core dump for no > reason if the instructions involved contain load/store to any of the > GIC registers. Huh? Single-stepping the instruction should execute it inside QEMU, which will do the load in the usual way. That should not be going via gdbstub reads and writes. -- PMM
On Mon, Mar 28, 2022 at 5:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 28 Mar 2022 at 03:10, Bin Meng <bmeng.cn@gmail.com> wrote: > > IMHO it's too bad to just ignore this bug forever. > > > > This is a valid use case. It's not about whether we intentionally want > > to inspect the GIC register value from gdb. The case is that when > > single stepping the source codes it triggers the core dump for no > > reason if the instructions involved contain load/store to any of the > > GIC registers. > > Huh? Single-stepping the instruction should execute it inside > QEMU, which will do the load in the usual way. That should not > be going via gdbstub reads and writes. Yes, single-stepping the instruction is executed in the vCPU context, but a gdb client sends additional commands, more than just telling QEMU to execute a single instruction. For example, the following is the sequence a gdb client sent when doing a "si": gdbstub_io_command Received: Z0,100000,4 gdbstub_io_reply Sent: OK gdbstub_io_got_ack Got ACK gdbstub_io_command Received: m18c430,4 gdbstub_io_reply Sent: ff430091 gdbstub_io_got_ack Got ACK gdbstub_io_command Received: vCont;s:p1.1;c:p1.-1 gdbstub_op_stepping Stepping CPU 0 gdbstub_op_continue_cpu Continuing CPU 1 gdbstub_op_continue_cpu Continuing CPU 2 gdbstub_op_continue_cpu Continuing CPU 3 gdbstub_hit_break RUN_STATE_DEBUG gdbstub_io_reply Sent: T05thread:p01.01; gdbstub_io_got_ack Got ACK gdbstub_io_command Received: g gdbstub_io_reply Sent: 3848ed0000000000f08fa610000000000300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001f90000000030a5ec000000000034c4180000000000c9030000 gdbstub_io_got_ack Got ACK gdbstub_io_command Received: m18c434,4 gdbstub_io_reply Sent: 00e004d1 gdbstub_io_got_ack Got ACK gdbstub_io_command Received: m18c430,4 gdbstub_io_reply Sent: ff430091 gdbstub_io_got_ack Got ACK gdbstub_io_command Received: m18c434,4 gdbstub_io_reply Sent: 00e004d1 gdbstub_io_got_ack Got ACK gdbstub_io_command Received: m18c400,40 gdbstub_io_reply Sent: ff4300d1e00300f980370058000040f900a00191000040f900b00091000040f900e004911e7800f9fe0340f91e0000f9ff43009100e004d174390094bb390094 gdbstub_io_got_ack Got ACK gdbstub_io_command Received: mf9010000,4 Here "mf9010000,4" triggers the bug where 0xf9010000 is the GIC register. This is not something QEMU can ignore or control. The logic is inside the gdb client. Regards, Bin
On Tue, Mar 29, 2022 at 12:43 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Mon, Mar 28, 2022 at 5:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Mon, 28 Mar 2022 at 03:10, Bin Meng <bmeng.cn@gmail.com> wrote: > > > IMHO it's too bad to just ignore this bug forever. > > > > > > This is a valid use case. It's not about whether we intentionally want > > > to inspect the GIC register value from gdb. The case is that when > > > single stepping the source codes it triggers the core dump for no > > > reason if the instructions involved contain load/store to any of the > > > GIC registers. > > > > Huh? Single-stepping the instruction should execute it inside > > QEMU, which will do the load in the usual way. That should not > > be going via gdbstub reads and writes. > > Yes, single-stepping the instruction is executed in the vCPU context, > but a gdb client sends additional commands, more than just telling > QEMU to execute a single instruction. > > For example, the following is the sequence a gdb client sent when doing a "si": > > gdbstub_io_command Received: Z0,100000,4 > gdbstub_io_reply Sent: OK > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: m18c430,4 > gdbstub_io_reply Sent: ff430091 > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: vCont;s:p1.1;c:p1.-1 > gdbstub_op_stepping Stepping CPU 0 > gdbstub_op_continue_cpu Continuing CPU 1 > gdbstub_op_continue_cpu Continuing CPU 2 > gdbstub_op_continue_cpu Continuing CPU 3 > gdbstub_hit_break RUN_STATE_DEBUG > gdbstub_io_reply Sent: T05thread:p01.01; > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: g > gdbstub_io_reply Sent: > 3848ed0000000000f08fa610000000000300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001f90000000030a5ec000000000034c4180000000000c9030000 > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: m18c434,4 > gdbstub_io_reply Sent: 00e004d1 > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: m18c430,4 > gdbstub_io_reply Sent: ff430091 > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: m18c434,4 > gdbstub_io_reply Sent: 00e004d1 > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: m18c400,40 > gdbstub_io_reply Sent: > ff4300d1e00300f980370058000040f900a00191000040f900b00091000040f900e004911e7800f9fe0340f91e0000f9ff43009100e004d174390094bb390094 > gdbstub_io_got_ack Got ACK > gdbstub_io_command Received: mf9010000,4 > > Here "mf9010000,4" triggers the bug where 0xf9010000 is the GIC register. > > This is not something QEMU can ignore or control. The logic is inside > the gdb client. > Ping for this series? Regards, Bin
On Sat, Apr 2, 2022 at 7:20 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Tue, Mar 29, 2022 at 12:43 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > On Mon, Mar 28, 2022 at 5:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Mon, 28 Mar 2022 at 03:10, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > IMHO it's too bad to just ignore this bug forever. > > > > > > > > This is a valid use case. It's not about whether we intentionally want > > > > to inspect the GIC register value from gdb. The case is that when > > > > single stepping the source codes it triggers the core dump for no > > > > reason if the instructions involved contain load/store to any of the > > > > GIC registers. > > > > > > Huh? Single-stepping the instruction should execute it inside > > > QEMU, which will do the load in the usual way. That should not > > > be going via gdbstub reads and writes. > > > > Yes, single-stepping the instruction is executed in the vCPU context, > > but a gdb client sends additional commands, more than just telling > > QEMU to execute a single instruction. > > > > For example, the following is the sequence a gdb client sent when doing a "si": > > > > gdbstub_io_command Received: Z0,100000,4 > > gdbstub_io_reply Sent: OK > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: m18c430,4 > > gdbstub_io_reply Sent: ff430091 > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: vCont;s:p1.1;c:p1.-1 > > gdbstub_op_stepping Stepping CPU 0 > > gdbstub_op_continue_cpu Continuing CPU 1 > > gdbstub_op_continue_cpu Continuing CPU 2 > > gdbstub_op_continue_cpu Continuing CPU 3 > > gdbstub_hit_break RUN_STATE_DEBUG > > gdbstub_io_reply Sent: T05thread:p01.01; > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: g > > gdbstub_io_reply Sent: > > 3848ed0000000000f08fa610000000000300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001f90000000030a5ec000000000034c4180000000000c9030000 > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: m18c434,4 > > gdbstub_io_reply Sent: 00e004d1 > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: m18c430,4 > > gdbstub_io_reply Sent: ff430091 > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: m18c434,4 > > gdbstub_io_reply Sent: 00e004d1 > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: m18c400,40 > > gdbstub_io_reply Sent: > > ff4300d1e00300f980370058000040f900a00191000040f900b00091000040f900e004911e7800f9fe0340f91e0000f9ff43009100e004d174390094bb390094 > > gdbstub_io_got_ack Got ACK > > gdbstub_io_command Received: mf9010000,4 > > > > Here "mf9010000,4" triggers the bug where 0xf9010000 is the GIC register. > > > > This is not something QEMU can ignore or control. The logic is inside > > the gdb client. > > > > Ping for this series? > Ping?
Bin Meng <bmeng.cn@gmail.com> writes: > On Sat, Apr 2, 2022 at 7:20 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> >> On Tue, Mar 29, 2022 at 12:43 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> > >> > On Mon, Mar 28, 2022 at 5:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> > > >> > > On Mon, 28 Mar 2022 at 03:10, Bin Meng <bmeng.cn@gmail.com> wrote: >> > > > IMHO it's too bad to just ignore this bug forever. >> > > > >> > > > This is a valid use case. It's not about whether we intentionally want >> > > > to inspect the GIC register value from gdb. The case is that when >> > > > single stepping the source codes it triggers the core dump for no >> > > > reason if the instructions involved contain load/store to any of the >> > > > GIC registers. >> > > >> > > Huh? Single-stepping the instruction should execute it inside >> > > QEMU, which will do the load in the usual way. That should not >> > > be going via gdbstub reads and writes. >> > >> > Yes, single-stepping the instruction is executed in the vCPU context, >> > but a gdb client sends additional commands, more than just telling >> > QEMU to execute a single instruction. >> > >> > For example, the following is the sequence a gdb client sent when doing a "si": >> > >> > gdbstub_io_command Received: Z0,100000,4 >> > gdbstub_io_reply Sent: OK >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c430,4 >> > gdbstub_io_reply Sent: ff430091 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: vCont;s:p1.1;c:p1.-1 >> > gdbstub_op_stepping Stepping CPU 0 >> > gdbstub_op_continue_cpu Continuing CPU 1 >> > gdbstub_op_continue_cpu Continuing CPU 2 >> > gdbstub_op_continue_cpu Continuing CPU 3 >> > gdbstub_hit_break RUN_STATE_DEBUG >> > gdbstub_io_reply Sent: T05thread:p01.01; >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: g >> > gdbstub_io_reply Sent: >> > 3848ed0000000000f08fa610000000000300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001f90000000030a5ec000000000034c4180000000000c9030000 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c434,4 >> > gdbstub_io_reply Sent: 00e004d1 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c430,4 >> > gdbstub_io_reply Sent: ff430091 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c434,4 >> > gdbstub_io_reply Sent: 00e004d1 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c400,40 >> > gdbstub_io_reply Sent: >> > ff4300d1e00300f980370058000040f900a00191000040f900b00091000040f900e004911e7800f9fe0340f91e0000f9ff43009100e004d174390094bb390094 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: mf9010000,4 >> > >> > Here "mf9010000,4" triggers the bug where 0xf9010000 is the GIC register. >> > >> > This is not something QEMU can ignore or control. The logic is inside >> > the gdb client. >> > >> >> Ping for this series? >> > > Ping? Re-reading the thread we seem to have two problems: - gdbstub is not explicitly passing the explicit CPU for its access - some devices use current_cpu to work out what AS they should be working in But I've already said just fudging current_cpu isn't the correct approach.
Bin Meng <bmeng.cn@gmail.com> writes: > On Sat, Apr 2, 2022 at 7:20 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> >> On Tue, Mar 29, 2022 at 12:43 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> > >> > On Mon, Mar 28, 2022 at 5:10 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> > > >> > > On Mon, 28 Mar 2022 at 03:10, Bin Meng <bmeng.cn@gmail.com> wrote: >> > > > IMHO it's too bad to just ignore this bug forever. >> > > > >> > > > This is a valid use case. It's not about whether we intentionally want >> > > > to inspect the GIC register value from gdb. The case is that when >> > > > single stepping the source codes it triggers the core dump for no >> > > > reason if the instructions involved contain load/store to any of the >> > > > GIC registers. >> > > >> > > Huh? Single-stepping the instruction should execute it inside >> > > QEMU, which will do the load in the usual way. That should not >> > > be going via gdbstub reads and writes. >> > >> > Yes, single-stepping the instruction is executed in the vCPU context, >> > but a gdb client sends additional commands, more than just telling >> > QEMU to execute a single instruction. >> > >> > For example, the following is the sequence a gdb client sent when doing a "si": >> > >> > gdbstub_io_command Received: Z0,100000,4 >> > gdbstub_io_reply Sent: OK >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c430,4 >> > gdbstub_io_reply Sent: ff430091 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: vCont;s:p1.1;c:p1.-1 >> > gdbstub_op_stepping Stepping CPU 0 >> > gdbstub_op_continue_cpu Continuing CPU 1 >> > gdbstub_op_continue_cpu Continuing CPU 2 >> > gdbstub_op_continue_cpu Continuing CPU 3 >> > gdbstub_hit_break RUN_STATE_DEBUG >> > gdbstub_io_reply Sent: T05thread:p01.01; >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: g >> > gdbstub_io_reply Sent: >> > 3848ed0000000000f08fa610000000000300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001f90000000030a5ec000000000034c4180000000000c9030000 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c434,4 >> > gdbstub_io_reply Sent: 00e004d1 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c430,4 >> > gdbstub_io_reply Sent: ff430091 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c434,4 >> > gdbstub_io_reply Sent: 00e004d1 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: m18c400,40 >> > gdbstub_io_reply Sent: >> > ff4300d1e00300f980370058000040f900a00191000040f900b00091000040f900e004911e7800f9fe0340f91e0000f9ff43009100e004d174390094bb390094 >> > gdbstub_io_got_ack Got ACK >> > gdbstub_io_command Received: mf9010000,4 >> > >> > Here "mf9010000,4" triggers the bug where 0xf9010000 is the GIC register. >> > >> > This is not something QEMU can ignore or control. The logic is inside >> > the gdb client. >> > >> >> Ping for this series? >> > > Ping? Can you have a look at: Subject: [RFC PATCH] gdb/gic: expose cpu_index via MxTxAttrs Date: Tue, 12 Apr 2022 11:45:19 +0100 Message-Id: <20220412104519.201655-1-alex.bennee@linaro.org> and let me know what you think?
diff --git a/gdbstub.c b/gdbstub.c index 3c14c6a038..0b12b98fbc 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -66,6 +66,9 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr, uint8_t *buf, int len, bool is_write) { CPUClass *cc; + int ret; + + current_cpu = cpu; #ifndef CONFIG_USER_ONLY if (phy_memory_mode) { @@ -74,15 +77,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr, } else { cpu_physical_memory_read(addr, buf, len); } - return 0; + ret = 0; + goto done; } #endif cc = CPU_GET_CLASS(cpu); if (cc->memory_rw_debug) { - return cc->memory_rw_debug(cpu, addr, buf, len, is_write); + ret = cc->memory_rw_debug(cpu, addr, buf, len, is_write); + goto done; } - return cpu_memory_rw_debug(cpu, addr, buf, len, is_write); + ret = cpu_memory_rw_debug(cpu, addr, buf, len, is_write); + +done: + current_cpu = NULL; + return ret; } /* Return the GDB index for a given vCPU state.
When accessing the per-CPU register bank of some devices (e.g.: GIC) from the GDB stub context, a segfault occurs. This is due to current_cpu is not set, as the contect is not a guest CPU. Let's set current_cpu before doing the acutal memory read write. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/124 Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- gdbstub.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)