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

[Philippe Mathieu-Daudé]

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?

Instead of MEMTXATTRS_UNSPECIFIED I should use a crafted MemTxAttrs with 
.secure = 1, .unspecified = 1? The idea of this command is to use the 
CPU AS but not the MMU/MPU, maybe it doesn't make sense...