Re: [Qemu-arm] [PATCH v4 4/4] target/arm: Add arm_gdb_set_sysreg() callback

[Peter Maydell]

On 12 March 2018 at 10:31, Abdallah Bouassida
<address@hidden> wrote:
> This is a callback to set the cp-regs registered by the dynamic XML.
>
> Signed-off-by: Abdallah Bouassida <address@hidden>
> ---
>>> Adding to that our customers may need this write access, our tool TRACE32®
>>> needs this also in some particular cases. For example: temporary disabling 
>>> MMU
>>> to do a physical memory access.
>
>> By clearing the SCTLR bit? That's a good example of a case that
>> won't work reliably. If you clear the SCTLR.M bit via raw_write
>> this will not perform the tlb_flush() that it needs to, which
>> means that if anything does a memory access via the QEMU TLB
>> it may get the wrong cached results. If you always clear the
>> bit, do one gdb memory access then set the bit then it will
>> probably not run into problems but you're walking on thin ice.
>
> Does adding tlb_flush() before or after write_raw_cp_reg()
> could solve the reliability issue for other particular cases?
> Or is there any improvement that could be done for this write
> callback in order to get more reliable results for other
> particular cases?

(We kind of discussed some of this on IRC, so this is just writing
that conversation down as I understand the situation. I think we
can get the first 3 patches in to shape to apply without having
to resolve the writing-registers part first, anyway.)

I think there's an underlying unaddressed design question here,
which is: what should the semantics of gdbstub writes to system
registers be, and how can we best implement that? At the moment
system registers have basically two access methods:
 * real guest reads and writes, which we open-code in the
   code-generation parts of translate.c and translate-a64.c.
   This includes access-permission checks that don't allow a
   guest running at a lower exception level to access higher EL
   registers, and is also the code path that uses the CPRegInfo
   accessfn, readfn and writefn functions if a register spec
   defines them.
 * "raw" reads and writes. These were designed for VM migration
   and for synchronization of state with the kernel when running
   under KVM. That means they're not expected to actually be
   changing the state of the CPU, and they just update the
   underlying CPU state data structure fields for the most part.

Neither of those is necessarily the right thing for a write from GDB.

We get away with using 'raw read' for reads from GDB because
generally that gives us the right value and if we do happen
to return a wrong value that's pretty harmless: it won't have
upset the state of QEMU. But raw writes are potentially dangerous
if we let the user make them. SCTLR.M is one example discussed
above. Another is if you allow the user to write SCR_EL3 directly
when the CPU isn't at EL3 then you might be allowing them to set
SCR_EL3.NS == 0 with a PSTATE indicating EL2; QEMU will likely
assert() shortly after because that's not architecturally valid.

Using an equivalent of the "real guest write" path would at
least be a bit safer than a raw write, but I'm not sure that
"sorry, can't do a physical memory access because the CPU's
at EL0 and we can't write SCTLR" is the behaviour you want...

So my feeling is that to safely support guest sysreg writes
from the gdbstub we need to:
 * define what the semantics of doing that are (preferably
   by reference to something architectural, eg what you get
   if you try it via halting debug mode)
 * provide some way of getting those semantics, which is probably
   another function pointer in the regdef struct for a gdb write
 * go through the various regdefs and update them to be able
   to handle gdb writes (either by providing a function to do
   the job, or by being flagged as "raw write is ok for gdb
   write" or something. Will depend on the individual register.)

The really important part here is 1, I think. Part 3 will be
a bit tedious, but I don't think there is any way to avoid
it. There's no "works for all registers" shortcut.

thanks
-- PMM