Re: [Qemu-devel] [PATCH v3 39/45] windbg: implemented kd_api_get_version

[Ladi Prosek]

On Wed, Dec 6, 2017 at 10:00 AM, Mihail Abakumov
<address@hidden> wrote:
> Ladi Prosek писал 2017-11-29 11:14:
>
>> On Tue, Nov 21, 2017 at 3:10 PM, Mihail Abakumov
>> <address@hidden> wrote:
>>>
>>> Signed-off-by: Mihail Abakumov <address@hidden>
>>> Signed-off-by: Pavel Dovgalyuk <address@hidden>
>>> Signed-off-by: Dmitriy Koltunov <address@hidden>
>>> ---
>>>  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;
>>>
>>>
>