Re: [PATCH-for-5.0] gdbstub: Use correct address space with Qqemu.PhyMemMode packet

[Peter Maydell]

On Mon, 30 Mar 2020 at 17:21, Philippe Mathieu-Daudé <address@hidden> wrote:
>
> On 3/30/20 6:08 PM, Peter Maydell wrote:
> > On Mon, 30 Mar 2020 at 16:30, Philippe Mathieu-Daudé <address@hidden> wrote:
> >>
> >> Since commit 3f940dc98, we added support for vAttach packet
> >> to select a particular thread/cpu/core. However when using
> >> the GDB physical memory mode, it is not clear which CPU
> >> address space is used.
> >> Since the CPU address space is stored in CPUState::as, use
> >> address_space_rw() instead of cpu_physical_memory_rw().
> >>
> >> Fixes: ab4752ec8d9
> >> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> >> ---
> >>   gdbstub.c | 7 ++-----
> >>   1 file changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/gdbstub.c b/gdbstub.c
> >> index 013fb1ac0f..3baaef50e3 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -69,11 +69,8 @@ static inline int target_memory_rw_debug(CPUState *cpu, 
> >> target_ulong addr,
> >>
> >>   #ifndef CONFIG_USER_ONLY
> >>       if (phy_memory_mode) {
> >> -        if (is_write) {
> >> -            cpu_physical_memory_write(addr, buf, len);
> >> -        } else {
> >> -            cpu_physical_memory_read(addr, buf, len);
> >> -        }
> >> +        address_space_rw(cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
> >> +                         buf, len, is_write);
> >>           return 0;
> >
> > There's an argument here for using
> >     int asidx = cpu_asidx_from_attrs(cpu, MEMTXATTRS_UNSPECIFIED);
> >     AddressSpace *as = cpu_get_address_space(cpu, asidx);
> >
> > though it will effectively boil down to the same thing in the end
> > as there's no way for the gdbstub to specify whether it wanted
> > eg the Arm secure or non-secure physical address space.
>
> https://static.docs.arm.com/ihi0074/a/debug_interface_v6_0_architecture_specification_IHI0074A.pdf
>
> * Configuration of hypervisor noninvasive debug.
>
> This field can have one of the following values:
>
> - 0b00
> Separate controls for hypervisor noninvasive debug are not implemented,
> or no hypervisor is implemented. For ARMv7 PEs that implement the
> Virtualization Extensions, and for ARMv8 PEs that implement EL2, if
> separate controls for hypervisor debug visibility are not implemented,
> the hypervisor debug visibility is indicated by the relevant Non-secure
> debug visibility fields NSNID and NSID.
>
> OK so for ARM "noninvasive debug is not implemented" and we would use
> the core secure address space?

I'm not very familiar with the debug interface (we don't model
it in QEMU), but I think that is the wrong end of it. These
are bits in AUTHSTATUS, which is a read-only register provided
by the CPU to the debugger. It basically says "am I, the CPU,
going to permit you, the debugger, to debug code running
in secure mode, or in EL2". (The CPU gets to decide this:
for security some h/w will not want any random with access
to the jtag debug port to be able to just read out code from
the secure world, for instance.)

What the debugger gets to control is bits in the CSW register
in the "MEM-AP"; it can use these to specify the size of
a memory access it wants to make to the guest, and also
the 'type', which is IMPDEF but typically lets the debugger
say "code access vs data access", "privileged vs usermode"
and "secure vs non-secure".

The equivalent in the QEMU world is that the debugger can
specify the memory transaction attributes. The question is
whether the gdb protocol provides any equivalent of that:
if it doesn't then gdbstub.c has to make up an attribute and
use that.

> Instead of MEMTXATTRS_UNSPECIFIED I should use a crafted MemTxAttrs with
> .secure = 1, .unspecified = 1?

You shouldn't set 'unspecified = 1', because that indicates
"this is MEMTXATTRS_UNSPECIFIED". The default set of
unspecified-attributes are probably good enough,
though, so you can just use MEMTXATTRS_UNSPECIFIED.

> The idea of this command is to use the
> CPU AS but not the MMU/MPU, maybe it doesn't make sense...

The trouble is that the command isn't precise enough.
"read/write to physical memory" is fine if the CPU has
exactly one physical address space, but it's ambiguous if the CPU
has more than one physical address space. Either we need the
user to be able to tell us which one they wanted somehow
(which for QEMU more or less means that they should tell
us what tx attributes they wanted), or we need to make an
arbitrary decision.

PS: do we have any documentation of this new command ?
ab4752ec8d9 has the implementation but no documentation...

thanks
-- PMM