On 3/30/20 6:41 PM, Peter Maydell wrote: > On Mon, 30 Mar 2020 at 17:21, Philippe Mathieu-Daudé <phi...@redhat.com> > wrote: >> On 3/30/20 6:08 PM, Peter Maydell wrote: >>> On Mon, 30 Mar 2020 at 16:30, Philippe Mathieu-Daudé <f4...@amsat.org> >>> 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é <f4...@amsat.org> >>>> --- >>>> 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...
Jon, do you have documentation on the Qqemu.PhyMemMode packet? > > thanks > -- PMM >