Re: [PATCH 1/2] gdbstub: Set current_cpu for memory read write

[Alex Bennée]

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.

-- 
Alex Bennée