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

[Philippe Mathieu-Daudé]

On 3/30/20 6:41 PM, Peter Maydell wrote:
> On Mon, 30 Mar 2020 at 17:21, Philippe Mathieu-Daudé <philmd@redhat.com> 
> wrote:
>> On 3/30/20 6:08 PM, Peter Maydell wrote:
>>> On Mon, 30 Mar 2020 at 16:30, Philippe Mathieu-Daudé <f4bug@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é <f4bug@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
>