diff mbox series

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

Message ID 20200330153016.2959-1-f4bug@amsat.org
State New
Headers show
Series [PATCH-for-5.0] gdbstub: Use correct address space with Qqemu.PhyMemMode packet | expand

Commit Message

Philippe Mathieu-Daudé March 30, 2020, 3:30 p.m. UTC 
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(-)

Comments

Peter Maydell March 30, 2020, 4:08 p.m. UTC | #1 
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.

thanks
-- PMM
Philippe Mathieu-Daudé March 30, 2020, 4:21 p.m. UTC | #2 
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?

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...
Peter Maydell March 30, 2020, 4:41 p.m. UTC | #3 
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...

thanks
-- PMM
Philippe Mathieu-Daudé May 31, 2020, 3:27 p.m. UTC | #4 
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
>
Jon Doron May 31, 2020, 4:42 p.m. UTC | #5 
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.
Peter Maydell May 31, 2020, 4:57 p.m. UTC | #6 
On Sun, 31 May 2020 at 17:42, Jon Doron <arilou@gmail.com> wrote:
>
> On 31/05/2020, Philippe Mathieu-Daudé wrote:
> >On 3/30/20 6:41 PM, Peter Maydell wrote:
> >> 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?

> 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

docs/system/gdb.rst would be the place to document QEMU-specific
extensions to the gdb protocol (there's an "advanced debugging
options" section where we document things like the single-step
stuff you can also change via 'maint packet').

thanks
-- PMM
Jon Doron June 1, 2020, 7:29 a.m. UTC | #7 
On 31/05/2020, Peter Maydell wrote:
>On Sun, 31 May 2020 at 17:42, Jon Doron <arilou@gmail.com> wrote:
>>
>> On 31/05/2020, Philippe Mathieu-Daudé wrote:
>> >On 3/30/20 6:41 PM, Peter Maydell wrote:
>> >> 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?
>
>> 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
>
>docs/system/gdb.rst would be the place to document QEMU-specific
>extensions to the gdb protocol (there's an "advanced debugging
>options" section where we document things like the single-step
>stuff you can also change via 'maint packet').
>
>thanks
>-- PMM

Thanks, I'll know for next time, when I did all that work and 
re-factored gdbstub, that doc did not exist.

Perhaps Peter can just add the documentation to this commit?

-- Jon.
Alex Bennée June 1, 2020, 10:39 a.m. UTC | #8 
Jon Doron <arilou@gmail.com> writes:

> On 31/05/2020, Peter Maydell wrote:
>>On Sun, 31 May 2020 at 17:42, Jon Doron <arilou@gmail.com> wrote:
>>>
>>> On 31/05/2020, Philippe Mathieu-Daudé wrote:
>>> >On 3/30/20 6:41 PM, Peter Maydell wrote:
>>> >> 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?
>>
>>> 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
>>
>>docs/system/gdb.rst would be the place to document QEMU-specific
>>extensions to the gdb protocol (there's an "advanced debugging
>>options" section where we document things like the single-step
>>stuff you can also change via 'maint packet').
>>
>>thanks
>>-- PMM
>
> Thanks, I'll know for next time, when I did all that work and
> re-factored gdbstub, that doc did not exist.

The documentation existed in our old texinfo docs, however they have
been recently updated to rst and are now considerably easier to find and
hack on now.

> Perhaps Peter can just add the documentation to this commit?

Please don't impose extra burden on our overworked maintainer when he's
already given review feedback. Generally maintainers have more work to
do than time to do it in so the easiest way to expedite patches with
features you want added is to send well formed complete patches which
can be easily merged. Otherwise patches tend to end up deep in a pile of
"must get around to that when I can".
Jon Doron June 1, 2020, 10:41 a.m. UTC | #9 
On 01/06/2020, Alex Bennée wrote:
>
>Jon Doron <arilou@gmail.com> writes:
>
>> On 31/05/2020, Peter Maydell wrote:
>>>On Sun, 31 May 2020 at 17:42, Jon Doron <arilou@gmail.com> wrote:
>>>>
>>>> On 31/05/2020, Philippe Mathieu-Daudé wrote:
>>>> >On 3/30/20 6:41 PM, Peter Maydell wrote:
>>>> >> 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?
>>>
>>>> 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
>>>
>>>docs/system/gdb.rst would be the place to document QEMU-specific
>>>extensions to the gdb protocol (there's an "advanced debugging
>>>options" section where we document things like the single-step
>>>stuff you can also change via 'maint packet').
>>>
>>>thanks
>>>-- PMM
>>
>> Thanks, I'll know for next time, when I did all that work and
>> re-factored gdbstub, that doc did not exist.
>
>The documentation existed in our old texinfo docs, however they have
>been recently updated to rst and are now considerably easier to find and
>hack on now.
>
>> Perhaps Peter can just add the documentation to this commit?
>
>Please don't impose extra burden on our overworked maintainer when he's
>already given review feedback. Generally maintainers have more work to
>do than time to do it in so the easiest way to expedite patches with
>features you want added is to send well formed complete patches which
>can be easily merged. Otherwise patches tend to end up deep in a pile of
>"must get around to that when I can".
>
>-- 
>Alex Bennée

No problem, I'll revise a patch to the rst and send it.

Thanks,
-- Jon.
diff mbox series

Patch

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;
     }
 #endif