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

[Jon Doron <arilou@gmail.com>]

On 31/05/2020, Philippe Mathieu-Daudé wrote:
>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
>>

Hi, there is no documentation for this mode, but in general the idea was 
very simple.

I want to have GDB the option to see the physical memory and examine it 
and have this option toggled.

This was useful to me when I was working on nested virtual machine and I 
wanted to examine different states of the VMCS12 and EPTs.

I used this in the following commands:
// Enable
maint packet Qqemu.PhyMemMode:1

// Disable
maint packet Qqemu.PhyMemMode:0

It was mostly used part of a GDB script I played with to help me find 
the VMCS12 and EPTs.

The architecture was x86_64.

Let me know if you have any further questions.

Cheers,
-- Jon.