diff mbox series

[v3,39/45] windbg: implemented kd_api_get_version

Message ID 151127345586.6888.4109654104815822158.stgit@Misha-PC.lan02.inno
State New
Headers show
Series Windbg supporting | expand

Commit Message

Mikhail Abakumov Nov. 21, 2017, 2:10 p.m. UTC 
Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
---
 include/exec/windbgstub-utils.h |    1 +
 windbgstub-utils.c              |   22 ++++++++++++++++++++++
 windbgstub.c                    |    4 ++++
 3 files changed, 27 insertions(+)

Comments

Ladi Prosek Nov. 29, 2017, 8:14 a.m. UTC | #1 
On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
<mikhail.abakumov@ispras.ru> wrote:
> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
> ---
>  include/exec/windbgstub-utils.h |    1 +
>  windbgstub-utils.c              |   22 ++++++++++++++++++++++
>  windbgstub.c                    |    4 ++++
>  3 files changed, 27 insertions(+)
>
> diff --git a/include/exec/windbgstub-utils.h b/include/exec/windbgstub-utils.h
> index be48f69f40..bc5b6a8468 100755
> --- a/include/exec/windbgstub-utils.h
> +++ b/include/exec/windbgstub-utils.h
> @@ -99,6 +99,7 @@ void kd_api_read_io_space(CPUState *cpu, PacketData *pd);
>  void kd_api_write_io_space(CPUState *cpu, PacketData *pd);
>  void kd_api_read_physical_memory(CPUState *cpu, PacketData *pd);
>  void kd_api_write_physical_memory(CPUState *cpu, PacketData *pd);
> +void kd_api_get_version(CPUState *cpu, PacketData *pd);
>  void kd_api_unsupported(CPUState *cpu, PacketData *pd);
>
>  SizedBuf kd_gen_exception_sc(CPUState *cpu);
> diff --git a/windbgstub-utils.c b/windbgstub-utils.c
> index 6708e62798..7ef301bac7 100755
> --- a/windbgstub-utils.c
> +++ b/windbgstub-utils.c
> @@ -239,6 +239,28 @@ void kd_api_write_physical_memory(CPUState *cpu, PacketData *pd)
>      stl_p(&mem->ActualBytesWritten, len);
>  }
>
> +void kd_api_get_version(CPUState *cpu, PacketData *pd)
> +{
> +    DBGKD_GET_VERSION64 *kdver;
> +    int err = cpu_memory_rw_debug(cpu, version.addr, PTR(pd->m64) + 0x10,
> +                                  sizeof(DBGKD_MANIPULATE_STATE64) - 0x10, 0);
> +    if (!err) {
> +        kdver = (DBGKD_GET_VERSION64 *) (PTR(pd->m64) + 0x10);
> +
> +        stw_p(&kdver->MajorVersion, kdver->MajorVersion);
> +        stw_p(&kdver->MinorVersion, kdver->MinorVersion);
> +        stw_p(&kdver->Flags, kdver->Flags);
> +        stw_p(&kdver->MachineType, kdver->MachineType);
> +        stw_p(&kdver->Unused[0], kdver->Unused[0]);
> +        sttul_p(&kdver->KernBase, kdver->KernBase);
> +        sttul_p(&kdver->PsLoadedModuleList, kdver->PsLoadedModuleList);
> +        sttul_p(&kdver->DebuggerDataList, kdver->DebuggerDataList);
> +    } else {
> +        pd->m64.ReturnStatus = STATUS_UNSUCCESSFUL;

The ReturnStatus field must be written using st* as well. You have
this direct write in many functions.

> +        WINDBG_ERROR("get_version: " FMT_ERR, err);
> +    }
> +}
> +
>  void kd_api_unsupported(CPUState *cpu, PacketData *pd)
>  {
>      WINDBG_ERROR("Caught unimplemented api %s",
> diff --git a/windbgstub.c b/windbgstub.c
> index 72324ae53d..ddca290694 100755
> --- a/windbgstub.c
> +++ b/windbgstub.c
> @@ -197,6 +197,10 @@ static void windbg_process_manipulate_packet(ParsingContext *ctx)
>          kd_api_write_physical_memory(cpu, &ctx->data);
>          break;
>
> +    case DbgKdGetVersionApi:
> +        kd_api_get_version(cpu, &ctx->data);
> +        break;
> +
>      case DbgKdClearAllInternalBreakpointsApi:
>          return;
>
>
Mikhail Abakumov Dec. 6, 2017, 9 a.m. UTC | #2 
Ladi Prosek писал 2017-11-29 11:14:
> On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
> <mikhail.abakumov@ispras.ru> wrote:
>> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
>> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
>> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
>> ---
>>  include/exec/windbgstub-utils.h |    1 +
>>  windbgstub-utils.c              |   22 ++++++++++++++++++++++
>>  windbgstub.c                    |    4 ++++
>>  3 files changed, 27 insertions(+)
>> 
>> diff --git a/include/exec/windbgstub-utils.h 
>> b/include/exec/windbgstub-utils.h
>> index be48f69f40..bc5b6a8468 100755
>> --- a/include/exec/windbgstub-utils.h
>> +++ b/include/exec/windbgstub-utils.h
>> @@ -99,6 +99,7 @@ void kd_api_read_io_space(CPUState *cpu, PacketData 
>> *pd);
>>  void kd_api_write_io_space(CPUState *cpu, PacketData *pd);
>>  void kd_api_read_physical_memory(CPUState *cpu, PacketData *pd);
>>  void kd_api_write_physical_memory(CPUState *cpu, PacketData *pd);
>> +void kd_api_get_version(CPUState *cpu, PacketData *pd);
>>  void kd_api_unsupported(CPUState *cpu, PacketData *pd);
>> 
>>  SizedBuf kd_gen_exception_sc(CPUState *cpu);
>> diff --git a/windbgstub-utils.c b/windbgstub-utils.c
>> index 6708e62798..7ef301bac7 100755
>> --- a/windbgstub-utils.c
>> +++ b/windbgstub-utils.c
>> @@ -239,6 +239,28 @@ void kd_api_write_physical_memory(CPUState *cpu, 
>> PacketData *pd)
>>      stl_p(&mem->ActualBytesWritten, len);
>>  }
>> 
>> +void kd_api_get_version(CPUState *cpu, PacketData *pd)
>> +{
>> +    DBGKD_GET_VERSION64 *kdver;
>> +    int err = cpu_memory_rw_debug(cpu, version.addr, PTR(pd->m64) + 
>> 0x10,
>> +                                  sizeof(DBGKD_MANIPULATE_STATE64) - 
>> 0x10, 0);
>> +    if (!err) {
>> +        kdver = (DBGKD_GET_VERSION64 *) (PTR(pd->m64) + 0x10);
>> +
>> +        stw_p(&kdver->MajorVersion, kdver->MajorVersion);
>> +        stw_p(&kdver->MinorVersion, kdver->MinorVersion);
>> +        stw_p(&kdver->Flags, kdver->Flags);
>> +        stw_p(&kdver->MachineType, kdver->MachineType);
>> +        stw_p(&kdver->Unused[0], kdver->Unused[0]);
>> +        sttul_p(&kdver->KernBase, kdver->KernBase);
>> +        sttul_p(&kdver->PsLoadedModuleList, 
>> kdver->PsLoadedModuleList);
>> +        sttul_p(&kdver->DebuggerDataList, kdver->DebuggerDataList);
>> +    } else {
>> +        pd->m64.ReturnStatus = STATUS_UNSUCCESSFUL;
> 
> The ReturnStatus field must be written using st* as well. You have
> this direct write in many functions.
> 

I'm doing stl_p for status before sending of packet in the 
'windbg_process_manipulate_packet',
because this is a global field of structure. Is that bad?

>> +        WINDBG_ERROR("get_version: " FMT_ERR, err);
>> +    }
>> +}
>> +
>>  void kd_api_unsupported(CPUState *cpu, PacketData *pd)
>>  {
>>      WINDBG_ERROR("Caught unimplemented api %s",
>> diff --git a/windbgstub.c b/windbgstub.c
>> index 72324ae53d..ddca290694 100755
>> --- a/windbgstub.c
>> +++ b/windbgstub.c
>> @@ -197,6 +197,10 @@ static void 
>> windbg_process_manipulate_packet(ParsingContext *ctx)
>>          kd_api_write_physical_memory(cpu, &ctx->data);
>>          break;
>> 
>> +    case DbgKdGetVersionApi:
>> +        kd_api_get_version(cpu, &ctx->data);
>> +        break;
>> +
>>      case DbgKdClearAllInternalBreakpointsApi:
>>          return;
>> 
>>
Ladi Prosek Dec. 6, 2017, 9:37 a.m. UTC | #3 
On Wed, Dec 6, 2017 at 10:00 AM, Mihail Abakumov
<mikhail.abakumov@ispras.ru> wrote:
> Ladi Prosek писал 2017-11-29 11:14:
>
>> On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
>> <mikhail.abakumov@ispras.ru> wrote:
>>>
>>> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
>>> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
>>> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
>>> ---
>>>  include/exec/windbgstub-utils.h |    1 +
>>>  windbgstub-utils.c              |   22 ++++++++++++++++++++++
>>>  windbgstub.c                    |    4 ++++
>>>  3 files changed, 27 insertions(+)
>>>
>>> diff --git a/include/exec/windbgstub-utils.h
>>> b/include/exec/windbgstub-utils.h
>>> index be48f69f40..bc5b6a8468 100755
>>> --- a/include/exec/windbgstub-utils.h
>>> +++ b/include/exec/windbgstub-utils.h
>>> @@ -99,6 +99,7 @@ void kd_api_read_io_space(CPUState *cpu, PacketData
>>> *pd);
>>>  void kd_api_write_io_space(CPUState *cpu, PacketData *pd);
>>>  void kd_api_read_physical_memory(CPUState *cpu, PacketData *pd);
>>>  void kd_api_write_physical_memory(CPUState *cpu, PacketData *pd);
>>> +void kd_api_get_version(CPUState *cpu, PacketData *pd);
>>>  void kd_api_unsupported(CPUState *cpu, PacketData *pd);
>>>
>>>  SizedBuf kd_gen_exception_sc(CPUState *cpu);
>>> diff --git a/windbgstub-utils.c b/windbgstub-utils.c
>>> index 6708e62798..7ef301bac7 100755
>>> --- a/windbgstub-utils.c
>>> +++ b/windbgstub-utils.c
>>> @@ -239,6 +239,28 @@ void kd_api_write_physical_memory(CPUState *cpu,
>>> PacketData *pd)
>>>      stl_p(&mem->ActualBytesWritten, len);
>>>  }
>>>
>>> +void kd_api_get_version(CPUState *cpu, PacketData *pd)
>>> +{
>>> +    DBGKD_GET_VERSION64 *kdver;
>>> +    int err = cpu_memory_rw_debug(cpu, version.addr, PTR(pd->m64) +
>>> 0x10,
>>> +                                  sizeof(DBGKD_MANIPULATE_STATE64) -
>>> 0x10, 0);
>>> +    if (!err) {
>>> +        kdver = (DBGKD_GET_VERSION64 *) (PTR(pd->m64) + 0x10);
>>> +
>>> +        stw_p(&kdver->MajorVersion, kdver->MajorVersion);
>>> +        stw_p(&kdver->MinorVersion, kdver->MinorVersion);
>>> +        stw_p(&kdver->Flags, kdver->Flags);
>>> +        stw_p(&kdver->MachineType, kdver->MachineType);
>>> +        stw_p(&kdver->Unused[0], kdver->Unused[0]);
>>> +        sttul_p(&kdver->KernBase, kdver->KernBase);
>>> +        sttul_p(&kdver->PsLoadedModuleList, kdver->PsLoadedModuleList);
>>> +        sttul_p(&kdver->DebuggerDataList, kdver->DebuggerDataList);
>>> +    } else {
>>> +        pd->m64.ReturnStatus = STATUS_UNSUCCESSFUL;
>>
>>
>> The ReturnStatus field must be written using st* as well. You have
>> this direct write in many functions.
>>
>
> I'm doing stl_p for status before sending of packet in the
> 'windbg_process_manipulate_packet',
> because this is a global field of structure. Is that bad?

I see. Technically it's probably ok, as long as you make sure that the
field is always aligned. But having the field hold two representations
of the value depending on where exactly in the program you are is not
good style. Imagine debugging it, looking at the contents of the
structure, and not being sure if you're looking at the host or guest
byte order. Plus everybody reading the code will be confused just like
me :)

>>> +        WINDBG_ERROR("get_version: " FMT_ERR, err);
>>> +    }
>>> +}
>>> +
>>>  void kd_api_unsupported(CPUState *cpu, PacketData *pd)
>>>  {
>>>      WINDBG_ERROR("Caught unimplemented api %s",
>>> diff --git a/windbgstub.c b/windbgstub.c
>>> index 72324ae53d..ddca290694 100755
>>> --- a/windbgstub.c
>>> +++ b/windbgstub.c
>>> @@ -197,6 +197,10 @@ static void
>>> windbg_process_manipulate_packet(ParsingContext *ctx)
>>>          kd_api_write_physical_memory(cpu, &ctx->data);
>>>          break;
>>>
>>> +    case DbgKdGetVersionApi:
>>> +        kd_api_get_version(cpu, &ctx->data);
>>> +        break;
>>> +
>>>      case DbgKdClearAllInternalBreakpointsApi:
>>>          return;
>>>
>>>
>
diff mbox series

Patch

diff --git a/include/exec/windbgstub-utils.h b/include/exec/windbgstub-utils.h
index be48f69f40..bc5b6a8468 100755
--- a/include/exec/windbgstub-utils.h
+++ b/include/exec/windbgstub-utils.h
@@ -99,6 +99,7 @@  void kd_api_read_io_space(CPUState *cpu, PacketData *pd);
 void kd_api_write_io_space(CPUState *cpu, PacketData *pd);
 void kd_api_read_physical_memory(CPUState *cpu, PacketData *pd);
 void kd_api_write_physical_memory(CPUState *cpu, PacketData *pd);
+void kd_api_get_version(CPUState *cpu, PacketData *pd);
 void kd_api_unsupported(CPUState *cpu, PacketData *pd);
 
 SizedBuf kd_gen_exception_sc(CPUState *cpu);
diff --git a/windbgstub-utils.c b/windbgstub-utils.c
index 6708e62798..7ef301bac7 100755
--- a/windbgstub-utils.c
+++ b/windbgstub-utils.c
@@ -239,6 +239,28 @@  void kd_api_write_physical_memory(CPUState *cpu, PacketData *pd)
     stl_p(&mem->ActualBytesWritten, len);
 }
 
+void kd_api_get_version(CPUState *cpu, PacketData *pd)
+{
+    DBGKD_GET_VERSION64 *kdver;
+    int err = cpu_memory_rw_debug(cpu, version.addr, PTR(pd->m64) + 0x10,
+                                  sizeof(DBGKD_MANIPULATE_STATE64) - 0x10, 0);
+    if (!err) {
+        kdver = (DBGKD_GET_VERSION64 *) (PTR(pd->m64) + 0x10);
+
+        stw_p(&kdver->MajorVersion, kdver->MajorVersion);
+        stw_p(&kdver->MinorVersion, kdver->MinorVersion);
+        stw_p(&kdver->Flags, kdver->Flags);
+        stw_p(&kdver->MachineType, kdver->MachineType);
+        stw_p(&kdver->Unused[0], kdver->Unused[0]);
+        sttul_p(&kdver->KernBase, kdver->KernBase);
+        sttul_p(&kdver->PsLoadedModuleList, kdver->PsLoadedModuleList);
+        sttul_p(&kdver->DebuggerDataList, kdver->DebuggerDataList);
+    } else {
+        pd->m64.ReturnStatus = STATUS_UNSUCCESSFUL;
+        WINDBG_ERROR("get_version: " FMT_ERR, err);
+    }
+}
+
 void kd_api_unsupported(CPUState *cpu, PacketData *pd)
 {
     WINDBG_ERROR("Caught unimplemented api %s",
diff --git a/windbgstub.c b/windbgstub.c
index 72324ae53d..ddca290694 100755
--- a/windbgstub.c
+++ b/windbgstub.c
@@ -197,6 +197,10 @@  static void windbg_process_manipulate_packet(ParsingContext *ctx)
         kd_api_write_physical_memory(cpu, &ctx->data);
         break;
 
+    case DbgKdGetVersionApi:
+        kd_api_get_version(cpu, &ctx->data);
+        break;
+
     case DbgKdClearAllInternalBreakpointsApi:
         return;