[Qemu-devel] Use of TARGET_FMT_plx in hw/tpm/trace-events

[Qemu-devel] Use of TARGET_FMT_plx in hw/tpm/trace-events

[Markus Armbruster]

Consider hw/tpm/trace-events

    # tpm_crb.c
    tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
    tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32

Format is TARGET_FMT_plx formats a hwaddr, but the parameter type is
uint64_t.  They happen to be the same.  Is this kosher?

Re: [Qemu-devel] Use of TARGET_FMT_plx in hw/tpm/trace-events

[Philippe Mathieu-Daudé]

On 7/20/19 8:39 AM, Markus Armbruster wrote:
> Consider hw/tpm/trace-events
> 
>     # tpm_crb.c
>     tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>     tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
> 
> Format is TARGET_FMT_plx formats a hwaddr, but the parameter type is
> uint64_t.  They happen to be the same.  Is this kosher?
> 

Missed when converting from DPRINTF() to trace-events:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ec427498;hp=8cb340c613

PRIx64 certainly makes sense here.

Since it is the single use, once updated we can remote this hunk from
scripts/tracetool/format/log_stap.py:

    if macro == "TARGET_FMT_plx":
        return "%016x"

I guess remember a thread with Thomas talking about TARGET_FMT_plx but I
can't find it, maybe I dreamed about it...

The idea was, we poison TARGET_FMT_l[udx] but not TARGET_FMT_plx, we
could, but then we have to fix few formats, once done TARGET_FMT_plx
existence is almost irrelevant.

Re: [Qemu-devel] Use of TARGET_FMT_plx in hw/tpm/trace-events

[Thomas Huth]

On 20/07/2019 11.42, Philippe Mathieu-Daudé wrote:
> On 7/20/19 8:39 AM, Markus Armbruster wrote:
>> Consider hw/tpm/trace-events
>>
>>     # tpm_crb.c
>>     tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>>     tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>>
>> Format is TARGET_FMT_plx formats a hwaddr, but the parameter type is
>> uint64_t.  They happen to be the same.  Is this kosher?
>>
> 
> Missed when converting from DPRINTF() to trace-events:
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ec427498;hp=8cb340c613
> 
> PRIx64 certainly makes sense here.
> 
> Since it is the single use, once updated we can remote this hunk from
> scripts/tracetool/format/log_stap.py:
> 
>     if macro == "TARGET_FMT_plx":
>         return "%016x"
> 
> I guess remember a thread with Thomas talking about TARGET_FMT_plx but I
> can't find it, maybe I dreamed about it...

That was:

 https://patchwork.kernel.org/patch/10930327/

... I think we should rename it to HWADDR_PRI0x or so.

 Thomas

Re: [Qemu-devel] Use of TARGET_FMT_plx in hw/tpm/trace-events

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 20/07/2019 11.42, Philippe Mathieu-Daudé wrote:
>> On 7/20/19 8:39 AM, Markus Armbruster wrote:
>>> Consider hw/tpm/trace-events
>>>
>>>     # tpm_crb.c
>>>     tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>>>     tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>>>
>>> Format is TARGET_FMT_plx formats a hwaddr, but the parameter type is
>>> uint64_t.  They happen to be the same.  Is this kosher?
>>>
>> 
>> Missed when converting from DPRINTF() to trace-events:
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ec427498;hp=8cb340c613
>> 
>> PRIx64 certainly makes sense here.
>> 
>> Since it is the single use, once updated we can remote this hunk from
>> scripts/tracetool/format/log_stap.py:
>> 
>>     if macro == "TARGET_FMT_plx":
>>         return "%016x"
>> 
>> I guess remember a thread with Thomas talking about TARGET_FMT_plx but I
>> can't find it, maybe I dreamed about it...
>
> That was:
>
>  https://patchwork.kernel.org/patch/10930327/
>
> ... I think we should rename it to HWADDR_PRI0x or so.

I'd expect "PRI" macros to expand into just a format specifier.
"%016" PRIx64 is a full conversion specification.  Something like
HWADDR_FMT0x would work for me.

"Use HWADDR_PRIx and slowly kill TARGET_FMT_plx" (suggested in the
thread you linked) would also work for me.

Re: [Qemu-devel] Use of TARGET_FMT_plx in hw/tpm/trace-events

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/20/19 8:39 AM, Markus Armbruster wrote:
>> Consider hw/tpm/trace-events
>> 
>>     # tpm_crb.c
>>     tpm_crb_mmio_read(uint64_t addr, unsigned size, uint32_t val) "CRB read 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>>     tpm_crb_mmio_write(uint64_t addr, unsigned size, uint32_t val) "CRB write 0x" TARGET_FMT_plx " len:%u val: 0x%" PRIx32
>> 
>> Format is TARGET_FMT_plx formats a hwaddr, but the parameter type is
>> uint64_t.  They happen to be the same.  Is this kosher?
>> 
>
> Missed when converting from DPRINTF() to trace-events:
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ec427498;hp=8cb340c613
>
> PRIx64 certainly makes sense here.
>
> Since it is the single use, once updated we can remote this hunk from
> scripts/tracetool/format/log_stap.py:
>
>     if macro == "TARGET_FMT_plx":
>         return "%016x"

Good point, thanks!

[...]