[PATCH 0/6] build contrib/plugins using meson

[PATCH 0/6] build contrib/plugins using meson

[Pierrick Bouvier]

Contrib plugins have been built out of tree so far, thanks to a Makefile.
However, it is quite inconvenient for maintenance, as we may break them,
especially for specific architectures.

First patches are fixing warnings for existing plugins, then we add meson
support, and finally, we remove Makefile for contrib/plugins.

Based on the proposal of Anton Kochkov on associated gitlab issue.
Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710

Pierrick Bouvier (6):
  contrib/plugins/execlog: fix warning
  contrib/plugins/cache: fix warning when compiling on 32bits host
  contrib/plugins/hwprofile: fix warning when compiling on 32bits host
  contrib/plugins/hotblocks: fix warning when compiling on 32bits host
  meson: build contrib/plugins with meson
  contrib/plugins: remove Makefile for contrib/plugins

 configure                   | 18 --------
 Makefile                    | 10 -----
 meson.build                 |  4 ++
 contrib/plugins/cache.c     |  6 +--
 contrib/plugins/execlog.c   |  4 +-
 contrib/plugins/hotblocks.c |  4 +-
 contrib/plugins/hwprofile.c |  9 ++--
 contrib/plugins/Makefile    | 85 -------------------------------------
 contrib/plugins/meson.build | 23 ++++++++++
 9 files changed, 39 insertions(+), 124 deletions(-)
 delete mode 100644 contrib/plugins/Makefile
 create mode 100644 contrib/plugins/meson.build

-- 
2.39.2

[PATCH 1/6] contrib/plugins/execlog: fix warning

[Pierrick Bouvier]

Found on debian stable.

../contrib/plugins/execlog.c: In function ‘vcpu_tb_trans’:
../contrib/plugins/execlog.c:236:22: error: declaration of ‘n’ shadows a previous local [-Werror=shadow=local]
  236 |             for (int n = 0; n < all_reg_names->len; n++) {
      |                      ^
../contrib/plugins/execlog.c:184:12: note: shadowed declaration is here
  184 |     size_t n = qemu_plugin_tb_n_insns(tb);
      |

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/execlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index 1c1601cc0b4..d67d0107613 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -181,8 +181,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
     bool check_regs_this = rmatches;
     bool check_regs_next = false;
 
-    size_t n = qemu_plugin_tb_n_insns(tb);
-    for (size_t i = 0; i < n; i++) {
+    size_t n_insns = qemu_plugin_tb_n_insns(tb);
+    for (size_t i = 0; i < n_insns; i++) {
         char *insn_disas;
         uint64_t insn_vaddr;
 
-- 
2.39.2

Re: [PATCH 1/6] contrib/plugins/execlog: fix warning

[Thomas Huth]

On 15/08/2024 01.36, Pierrick Bouvier wrote:
> Found on debian stable.
> 
> ../contrib/plugins/execlog.c: In function ‘vcpu_tb_trans’:
> ../contrib/plugins/execlog.c:236:22: error: declaration of ‘n’ shadows a previous local [-Werror=shadow=local]
>    236 |             for (int n = 0; n < all_reg_names->len; n++) {
>        |                      ^
> ../contrib/plugins/execlog.c:184:12: note: shadowed declaration is here
>    184 |     size_t n = qemu_plugin_tb_n_insns(tb);
>        |
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   contrib/plugins/execlog.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> index 1c1601cc0b4..d67d0107613 100644
> --- a/contrib/plugins/execlog.c
> +++ b/contrib/plugins/execlog.c
> @@ -181,8 +181,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>       bool check_regs_this = rmatches;
>       bool check_regs_next = false;
>   
> -    size_t n = qemu_plugin_tb_n_insns(tb);
> -    for (size_t i = 0; i < n; i++) {
> +    size_t n_insns = qemu_plugin_tb_n_insns(tb);
> +    for (size_t i = 0; i < n_insns; i++) {
>           char *insn_disas;
>           uint64_t insn_vaddr;
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/6] contrib/plugins/execlog: fix warning

[Alexandre IOOSS]

On 8/15/24 01:36, Pierrick Bouvier wrote:
> Found on debian stable.
> 
> ../contrib/plugins/execlog.c: In function ‘vcpu_tb_trans’:
> ../contrib/plugins/execlog.c:236:22: error: declaration of ‘n’ shadows a previous local [-Werror=shadow=local]
>    236 |             for (int n = 0; n < all_reg_names->len; n++) {
>        |                      ^
> ../contrib/plugins/execlog.c:184:12: note: shadowed declaration is here
>    184 |     size_t n = qemu_plugin_tb_n_insns(tb);
>        |
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   contrib/plugins/execlog.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> index 1c1601cc0b4..d67d0107613 100644
> --- a/contrib/plugins/execlog.c
> +++ b/contrib/plugins/execlog.c
> @@ -181,8 +181,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>       bool check_regs_this = rmatches;
>       bool check_regs_next = false;
>   
> -    size_t n = qemu_plugin_tb_n_insns(tb);
> -    for (size_t i = 0; i < n; i++) {
> +    size_t n_insns = qemu_plugin_tb_n_insns(tb);
> +    for (size_t i = 0; i < n_insns; i++) {
>           char *insn_disas;
>           uint64_t insn_vaddr;
>   

Reviewed-by: Alexandre Iooss <erdnaxe@crans.org>

[PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Pierrick Bouvier]

Found on debian stable (i386).

../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
      |

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/cache.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
index 512ef6776b7..82ed734d6d4 100644
--- a/contrib/plugins/cache.c
+++ b/contrib/plugins/cache.c
@@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
     n_insns = qemu_plugin_tb_n_insns(tb);
     for (i = 0; i < n_insns; i++) {
         struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
-        uint64_t effective_addr;
+        uintptr_t effective_addr;
 
         if (sys) {
-            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
+            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
         } else {
-            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
+            effective_addr = (uintptr_t) qemu_plugin_insn_vaddr(insn);
         }
 
         /*
-- 
2.39.2

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Thomas Huth]

On 15/08/2024 01.36, Pierrick Bouvier wrote:
> Found on debian stable (i386).
> 
> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>    477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>        |
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   contrib/plugins/cache.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> index 512ef6776b7..82ed734d6d4 100644
> --- a/contrib/plugins/cache.c
> +++ b/contrib/plugins/cache.c
> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>       n_insns = qemu_plugin_tb_n_insns(tb);
>       for (i = 0; i < n_insns; i++) {
>           struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> -        uint64_t effective_addr;
> +        uintptr_t effective_addr;
>   
>           if (sys) {
> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>           } else {
> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
> +            effective_addr = (uintptr_t) qemu_plugin_insn_vaddr(insn);
>           }

Is this the right fix? I assume effective_addr stores an address of the 
guest, so if the guest is 64-bit and the host is 32-bit, you now lose the 
upper bits of the address...?

The casting for qemu_plugin_insn_vaddr is not required at all since it 
already returns an uint64_t, so you can remoe that one. For the haddr part, 
maybe do a double-cast:

   effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)

?

  Thomas

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>> Found on debian stable (i386).
>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>    477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>        |
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   contrib/plugins/cache.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>> index 512ef6776b7..82ed734d6d4 100644
>> --- a/contrib/plugins/cache.c
>> +++ b/contrib/plugins/cache.c
>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>       n_insns = qemu_plugin_tb_n_insns(tb);
>>       for (i = 0; i < n_insns; i++) {
>>           struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>> -        uint64_t effective_addr;
>> +        uintptr_t effective_addr;
>>             if (sys) {
>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>           } else {
>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>> +            effective_addr = (uintptr_t)
>> qemu_plugin_insn_vaddr(insn);
>>           }
>
> Is this the right fix? I assume effective_addr stores an address of
> the guest, so if the guest is 64-bit and the host is 32-bit, you now
> lose the upper bits of the address...?

I think the problem is higher up, it was a mistake to have:

  void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);

return *void, at least vaddr returns an explicit 64 bit value which can
hold everything (at a slight expense to 32bit emulation hosts, but
seriously stop doing that we've been in the 64bit world for some time
now).

>
> The casting for qemu_plugin_insn_vaddr is not required at all since it
> already returns an uint64_t, so you can remoe that one. For the haddr
> part, maybe do a double-cast:
>
>   effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)
>
> ?
>
>  Thomas

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Pierrick Bouvier]

On 8/15/24 04:46, Alex Bennée wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>> Found on debian stable (i386).
>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>     477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>         |
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    contrib/plugins/cache.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>>> index 512ef6776b7..82ed734d6d4 100644
>>> --- a/contrib/plugins/cache.c
>>> +++ b/contrib/plugins/cache.c
>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>        n_insns = qemu_plugin_tb_n_insns(tb);
>>>        for (i = 0; i < n_insns; i++) {
>>>            struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>>> -        uint64_t effective_addr;
>>> +        uintptr_t effective_addr;
>>>              if (sys) {
>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>>            } else {
>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>>> +            effective_addr = (uintptr_t)
>>> qemu_plugin_insn_vaddr(insn);
>>>            }
>>
>> Is this the right fix? I assume effective_addr stores an address of
>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
>> lose the upper bits of the address...?
> 
> I think the problem is higher up, it was a mistake to have:
> 
>    void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
> 
> return *void, at least vaddr returns an explicit 64 bit value which can
> hold everything (at a slight expense to 32bit emulation hosts, but
> seriously stop doing that we've been in the 64bit world for some time
> now).
> 

It's an open question I had. When executing 64 bits binaries on a 32 
bits host, are we emulating the full 64 bits address space, or do we 
restrict to 32 bits? For user mode, I don't see how it could be possible 
to have address space beyond the 32 bits range, but system mode is 
probably different.

The real proper fix is to not encode directly value under udata for 
callbacks, but allocate this and pass a pointer instead.

>>
>> The casting for qemu_plugin_insn_vaddr is not required at all since it
>> already returns an uint64_t, so you can remoe that one. For the haddr
>> part, maybe do a double-cast:
>>
>>    effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)
>>
>> ?
>>
>>   Thomas
> 

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Pierrick Bouvier]

On 8/15/24 10:38, Pierrick Bouvier wrote:
> On 8/15/24 04:46, Alex Bennée wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>>> Found on debian stable (i386).
>>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>>      477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>>          |
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     contrib/plugins/cache.c | 6 +++---
>>>>     1 file changed, 3 insertions(+), 3 deletions(-)
>>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>>>> index 512ef6776b7..82ed734d6d4 100644
>>>> --- a/contrib/plugins/cache.c
>>>> +++ b/contrib/plugins/cache.c
>>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>>         n_insns = qemu_plugin_tb_n_insns(tb);
>>>>         for (i = 0; i < n_insns; i++) {
>>>>             struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>>>> -        uint64_t effective_addr;
>>>> +        uintptr_t effective_addr;
>>>>               if (sys) {
>>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>>>             } else {
>>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>>>> +            effective_addr = (uintptr_t)
>>>> qemu_plugin_insn_vaddr(insn);
>>>>             }
>>>
>>> Is this the right fix? I assume effective_addr stores an address of
>>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
>>> lose the upper bits of the address...?
>>
>> I think the problem is higher up, it was a mistake to have:
>>
>>     void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
>>
>> return *void, at least vaddr returns an explicit 64 bit value which can
>> hold everything (at a slight expense to 32bit emulation hosts, but
>> seriously stop doing that we've been in the 64bit world for some time
>> now).
>>
> 
> It's an open question I had. When executing 64 bits binaries on a 32
> bits host, are we emulating the full 64 bits address space, or do we
> restrict to 32 bits? For user mode, I don't see how it could be possible
> to have address space beyond the 32 bits range, but system mode is
> probably different.
>

After trying to boot an x64 system on 32 bits host, I can confirm the 
address returned by qemu_plugin_tb_vaddr is effectively on 64 bits 
(using upper 32 bits).

Thus, I'll respin this series with a correct fix instead of the bad 
casts I used.

> The real proper fix is to not encode directly value under udata for
> callbacks, but allocate this and pass a pointer instead.
> 
>>>
>>> The casting for qemu_plugin_insn_vaddr is not required at all since it
>>> already returns an uint64_t, so you can remoe that one. For the haddr
>>> part, maybe do a double-cast:
>>>
>>>     effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)
>>>
>>> ?
>>>
>>>    Thomas
>>

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 8/15/24 04:46, Alex Bennée wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>>> Found on debian stable (i386).
>>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>>     477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>>         |
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>    contrib/plugins/cache.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>>>> index 512ef6776b7..82ed734d6d4 100644
>>>> --- a/contrib/plugins/cache.c
>>>> +++ b/contrib/plugins/cache.c
>>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>>        n_insns = qemu_plugin_tb_n_insns(tb);
>>>>        for (i = 0; i < n_insns; i++) {
>>>>            struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>>>> -        uint64_t effective_addr;
>>>> +        uintptr_t effective_addr;
>>>>              if (sys) {
>>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>>>            } else {
>>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>>>> +            effective_addr = (uintptr_t)
>>>> qemu_plugin_insn_vaddr(insn);
>>>>            }
>>>
>>> Is this the right fix? I assume effective_addr stores an address of
>>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
>>> lose the upper bits of the address...?
>> I think the problem is higher up, it was a mistake to have:
>>    void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn
>> *insn);
>> return *void, at least vaddr returns an explicit 64 bit value which
>> can
>> hold everything (at a slight expense to 32bit emulation hosts, but
>> seriously stop doing that we've been in the 64bit world for some time
>> now).
>> 
>
> It's an open question I had. When executing 64 bits binaries on a 32
> bits host, are we emulating the full 64 bits address space, or do we
> restrict to 32 bits?

Yes - and having to jump through a few extra hoops to do it.

> For user mode, I don't see how it could be
> possible to have address space beyond the 32 bits range, but system
> mode is probably different.

For user-modes very simple base + addr calculation yes it is tricky and
we often fail to find a gap big enough to map the guest address space
into it. If we implement softmmu for linux-user we could get around this
difficulty for a cost.

>
> The real proper fix is to not encode directly value under udata for
> callbacks, but allocate this and pass a pointer instead.
>
>>>
>>> The casting for qemu_plugin_insn_vaddr is not required at all since it
>>> already returns an uint64_t, so you can remoe that one. For the haddr
>>> part, maybe do a double-cast:
>>>
>>>    effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)
>>>
>>> ?
>>>
>>>   Thomas
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Richard Henderson]

On 8/15/24 21:46, Alex Bennée wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>> Found on debian stable (i386).
>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>     477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>         |
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    contrib/plugins/cache.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>>> index 512ef6776b7..82ed734d6d4 100644
>>> --- a/contrib/plugins/cache.c
>>> +++ b/contrib/plugins/cache.c
>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>        n_insns = qemu_plugin_tb_n_insns(tb);
>>>        for (i = 0; i < n_insns; i++) {
>>>            struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>>> -        uint64_t effective_addr;
>>> +        uintptr_t effective_addr;
>>>              if (sys) {
>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>>            } else {
>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>>> +            effective_addr = (uintptr_t)
>>> qemu_plugin_insn_vaddr(insn);
>>>            }
>>
>> Is this the right fix? I assume effective_addr stores an address of
>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
>> lose the upper bits of the address...?
> 
> I think the problem is higher up, it was a mistake to have:
> 
>    void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
> 
> return *void.

No, not a bug.  This is a host addr, right there in the name.
Returning uint64_t would be a bug.


r~

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 8/15/24 21:46, Alex Bennée wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>>> Found on debian stable (i386).
>>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>>     477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>>         |
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>    contrib/plugins/cache.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>>>> index 512ef6776b7..82ed734d6d4 100644
>>>> --- a/contrib/plugins/cache.c
>>>> +++ b/contrib/plugins/cache.c
>>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>>        n_insns = qemu_plugin_tb_n_insns(tb);
>>>>        for (i = 0; i < n_insns; i++) {
>>>>            struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>>>> -        uint64_t effective_addr;
>>>> +        uintptr_t effective_addr;
>>>>              if (sys) {
>>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>>>            } else {
>>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>>>> +            effective_addr = (uintptr_t)
>>>> qemu_plugin_insn_vaddr(insn);
>>>>            }
>>>
>>> Is this the right fix? I assume effective_addr stores an address of
>>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
>>> lose the upper bits of the address...?
>> I think the problem is higher up, it was a mistake to have:
>>    void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn
>> *insn);
>> return *void.
>
> No, not a bug.  This is a host addr, right there in the name.
> Returning uint64_t would be a bug.

No it's:

 * Returns: hardware (physical) target address of instruction

I was kinda assuming that was what the underlying host_addr[] fields in
DisasContextDB are. Are we just saying its QEMU's vaddr of where the
guest physical address is mapped into QEMU?

>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Peter Maydell]

On Fri, 16 Aug 2024 at 13:48, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
> > On 8/15/24 21:46, Alex Bennée wrote:
> >> Thomas Huth <thuth@redhat.com> writes:
> >>
> >>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
> >>>> Found on debian stable (i386).
> >>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
> >>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> >>>>     477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
> >>>>         |
> >>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> >>>> ---
> >>>>    contrib/plugins/cache.c | 6 +++---
> >>>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> >>>> index 512ef6776b7..82ed734d6d4 100644
> >>>> --- a/contrib/plugins/cache.c
> >>>> +++ b/contrib/plugins/cache.c
> >>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> >>>>        n_insns = qemu_plugin_tb_n_insns(tb);
> >>>>        for (i = 0; i < n_insns; i++) {
> >>>>            struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> >>>> -        uint64_t effective_addr;
> >>>> +        uintptr_t effective_addr;
> >>>>              if (sys) {
> >>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
> >>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
> >>>>            } else {
> >>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
> >>>> +            effective_addr = (uintptr_t)
> >>>> qemu_plugin_insn_vaddr(insn);
> >>>>            }
> >>>
> >>> Is this the right fix? I assume effective_addr stores an address of
> >>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
> >>> lose the upper bits of the address...?
> >> I think the problem is higher up, it was a mistake to have:
> >>    void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn
> >> *insn);
> >> return *void.
> >
> > No, not a bug.  This is a host addr, right there in the name.
> > Returning uint64_t would be a bug.
>
> No it's:
>
>  * Returns: hardware (physical) target address of instruction
>
> I was kinda assuming that was what the underlying host_addr[] fields in
> DisasContextDB are. Are we just saying its QEMU's vaddr of where the
> guest physical address is mapped into QEMU?

DisasContextBase::host_addr[] are host (virtual) addresses,
i.e. pointers you can dereference in C code in QEMU.
The comment in qemu_plugin_insn_haddr() says:

     * ??? The return value is not intended for use of host memory,
     * but as a proxy for address space and physical address.

This seems pretty QEMU-implementation-internal to me and
not something we should be allowing to escape into the
plugin API. What, per the comment, we ought to be exposing is
the (address space, guest physaddr) tuple, which we
presumably don't conveniently have to hand here.

thanks
-- PMM

Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Richard Henderson]

On 8/16/24 22:47, Alex Bennée wrote:
>> No, not a bug.  This is a host addr, right there in the name.
>> Returning uint64_t would be a bug.
> 
> No it's:
> 
>   * Returns: hardware (physical) target address of instruction
> 
> I was kinda assuming that was what the underlying host_addr[] fields in
> DisasContextDB are. Are we just saying its QEMU's vaddr of where the
> guest physical address is mapped into QEMU?
It's QEMU's host address of where the guest physical address is mapped.
That's why is says host_addr, and has pointer type.


r~

[PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host

[Pierrick Bouvier]

Found on debian stable (i386).

../contrib/plugins/hwprofile.c: In function 'new_location':
../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
      |                                ^
../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  227 |             off = (uint64_t) udata;
      |                   ^
../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  232 |                                                              (gpointer) off);
      |                                                              ^
../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
      |

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/hwprofile.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c
index 739ac0c66b5..ee94a74ad94 100644
--- a/contrib/plugins/hwprofile.c
+++ b/contrib/plugins/hwprofile.c
@@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
     return count;
 }
 
-static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc)
+static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
 {
     IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
     loc->off_or_pc = off_or_pc;
@@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
         return;
     } else {
         const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
-        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
+        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
         bool is_write = qemu_plugin_mem_is_store(meminfo);
         DeviceCounts *counts;
 
@@ -224,7 +224,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
 
         /* either track offsets or source of access */
         if (source) {
-            off = (uint64_t) udata;
+            off = (uintptr_t) udata;
         }
 
         if (pattern || source) {
@@ -247,7 +247,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 
     for (i = 0; i < n; i++) {
         struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
-        gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
+        gpointer udata = (gpointer) (
+                source ? (uintptr_t) qemu_plugin_insn_vaddr(insn) : 0);
         qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
                                          QEMU_PLUGIN_CB_NO_REGS,
                                          rw, udata);
-- 
2.39.2

Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host

[Thomas Huth]

On 15/08/2024 01.36, Pierrick Bouvier wrote:
> Found on debian stable (i386).
> 
> ../contrib/plugins/hwprofile.c: In function 'new_location':
> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>        |                                ^
> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>    227 |             off = (uint64_t) udata;
>        |                   ^
> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    232 |                                                              (gpointer) off);
>        |                                                              ^
> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>        |
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   contrib/plugins/hwprofile.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c
> index 739ac0c66b5..ee94a74ad94 100644
> --- a/contrib/plugins/hwprofile.c
> +++ b/contrib/plugins/hwprofile.c
> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>       return count;
>   }
>   
> -static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc)
> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>   {
>       IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>       loc->off_or_pc = off_or_pc;
> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>           return;
>       } else {
>           const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);

qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks wrong to me.

  Thomas

Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>> Found on debian stable (i386).
>> ../contrib/plugins/hwprofile.c: In function 'new_location':
>> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>    172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>>        |                                ^
>> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
>> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>    227 |             off = (uint64_t) udata;
>>        |                   ^
>> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>    232 |                                                              (gpointer) off);
>>        |                                                              ^
>> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
>> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>    250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>>        |
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   contrib/plugins/hwprofile.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>> diff --git a/contrib/plugins/hwprofile.c
>> b/contrib/plugins/hwprofile.c
>> index 739ac0c66b5..ee94a74ad94 100644
>> --- a/contrib/plugins/hwprofile.c
>> +++ b/contrib/plugins/hwprofile.c
>> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>>       return count;
>>   }
>>   -static IOLocationCounts *new_location(GHashTable *table, uint64_t
>> off_or_pc)
>> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>>   {
>>       IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>>       loc->off_or_pc = off_or_pc;
>> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>>           return;
>>       } else {
>>           const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>
> qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks
> wrong to me.

It is. However it just goes to show you should be expecting to
instrument 64 bit code with a 32 bit host because you can't do pointer
stuffing tricks like this.

Maybe we could just disable plugins on 32 bit hosts?

>
>  Thomas

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host

[Pierrick Bouvier]

On 8/15/24 05:03, Alex Bennée wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>> Found on debian stable (i386).
>>> ../contrib/plugins/hwprofile.c: In function 'new_location':
>>> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>     172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>>>         |                                ^
>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
>>> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>     227 |             off = (uint64_t) udata;
>>>         |                   ^
>>> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>     232 |                                                              (gpointer) off);
>>>         |                                                              ^
>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
>>> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>     250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>>>         |
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    contrib/plugins/hwprofile.c | 9 +++++----
>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>> diff --git a/contrib/plugins/hwprofile.c
>>> b/contrib/plugins/hwprofile.c
>>> index 739ac0c66b5..ee94a74ad94 100644
>>> --- a/contrib/plugins/hwprofile.c
>>> +++ b/contrib/plugins/hwprofile.c
>>> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>>>        return count;
>>>    }
>>>    -static IOLocationCounts *new_location(GHashTable *table, uint64_t
>>> off_or_pc)
>>> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>>>    {
>>>        IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>>>        loc->off_or_pc = off_or_pc;
>>> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>>>            return;
>>>        } else {
>>>            const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>>> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>
>> qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks
>> wrong to me.
> 
> It is. However it just goes to show you should be expecting to
> instrument 64 bit code with a 32 bit host because you can't do pointer
> stuffing tricks like this.
> 
> Maybe we could just disable plugins on 32 bit hosts?
> 

Only two plugins are concerned by this problem, it's worth fixing them 
correctly (i.e. not use 64 bits data directly as a pointer, but allocate 
memory and pass pointer instead).

>>
>>   Thomas
> 

Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host

[Pierrick Bouvier]

On 8/15/24 10:40, Pierrick Bouvier wrote:
> On 8/15/24 05:03, Alex Bennée wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>>> Found on debian stable (i386).
>>>> ../contrib/plugins/hwprofile.c: In function 'new_location':
>>>> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>      172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>>>>          |                                ^
>>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
>>>> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>>      227 |             off = (uint64_t) udata;
>>>>          |                   ^
>>>> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>      232 |                                                              (gpointer) off);
>>>>          |                                                              ^
>>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
>>>> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>      250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>>>>          |
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     contrib/plugins/hwprofile.c | 9 +++++----
>>>>     1 file changed, 5 insertions(+), 4 deletions(-)
>>>> diff --git a/contrib/plugins/hwprofile.c
>>>> b/contrib/plugins/hwprofile.c
>>>> index 739ac0c66b5..ee94a74ad94 100644
>>>> --- a/contrib/plugins/hwprofile.c
>>>> +++ b/contrib/plugins/hwprofile.c
>>>> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>>>>         return count;
>>>>     }
>>>>     -static IOLocationCounts *new_location(GHashTable *table, uint64_t
>>>> off_or_pc)
>>>> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>>>>     {
>>>>         IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>>>>         loc->off_or_pc = off_or_pc;
>>>> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>>>>             return;
>>>>         } else {
>>>>             const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>>>> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>>> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>>
>>> qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks
>>> wrong to me.
>>
>> It is. However it just goes to show you should be expecting to
>> instrument 64 bit code with a 32 bit host because you can't do pointer
>> stuffing tricks like this.
>>
>> Maybe we could just disable plugins on 32 bit hosts?
>>
> 
> Only two plugins are concerned by this problem, it's worth fixing them
> correctly (i.e. not use 64 bits data directly as a pointer, but allocate
> memory and pass pointer instead).
> 

After looking more closely at existing plugins, the problem is wider 
than expected (and build warnings).
The GLib macro (GUINT_TO_POINTER) silently transform an int64 value to a 
pointer, including on 32 bits platform. It performs a double case 
(unsigned long -> pointer).

Thus, even if there is no warning, some plugins are broken because of 
data (either because they pass data in callback, or they have an address 
based hashtable).

It could be fixable, but the wiser solution is probably to disable 
plugins on 32 bits platform, like Alex proposed.

>>>
>>>    Thomas
>>

[PATCH 4/6] contrib/plugins/hotblocks: fix warning when compiling on 32bits host

[Pierrick Bouvier]

Found on debian stable (i386).

../contrib/plugins/hotblocks.c: In function 'vcpu_tb_trans':
../contrib/plugins/hotblocks.c:117:56: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  117 |     cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash);
      |                                                        ^
../contrib/plugins/hotblocks.c:126:40: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  126 |         g_hash_table_insert(hotblocks, (gpointer) hash, (gpointer) cnt);
      |

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/hotblocks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index 02bc5078bdd..d540f1b7f0b 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -109,9 +109,9 @@ static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
 static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 {
     ExecCount *cnt;
-    uint64_t pc = qemu_plugin_tb_vaddr(tb);
+    uintptr_t pc = qemu_plugin_tb_vaddr(tb);
     size_t insns = qemu_plugin_tb_n_insns(tb);
-    uint64_t hash = pc ^ insns;
+    uintptr_t hash = pc ^ insns;
 
     g_mutex_lock(&lock);
     cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash);
-- 
2.39.2

Re: [PATCH 4/6] contrib/plugins/hotblocks: fix warning when compiling on 32bits host

[Thomas Huth]

On 15/08/2024 01.36, Pierrick Bouvier wrote:
> Found on debian stable (i386).
> 
> ../contrib/plugins/hotblocks.c: In function 'vcpu_tb_trans':
> ../contrib/plugins/hotblocks.c:117:56: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    117 |     cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash);
>        |                                                        ^
> ../contrib/plugins/hotblocks.c:126:40: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    126 |         g_hash_table_insert(hotblocks, (gpointer) hash, (gpointer) cnt);
>        |
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   contrib/plugins/hotblocks.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
> index 02bc5078bdd..d540f1b7f0b 100644
> --- a/contrib/plugins/hotblocks.c
> +++ b/contrib/plugins/hotblocks.c
> @@ -109,9 +109,9 @@ static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
>   static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>   {
>       ExecCount *cnt;
> -    uint64_t pc = qemu_plugin_tb_vaddr(tb);
> +    uintptr_t pc = qemu_plugin_tb_vaddr(tb);

qemu_plugin_tb_vaddr() return uint64_t, so this looks wrong to me, too.

  Thomas

[PATCH 5/6] meson: build contrib/plugins with meson

[Pierrick Bouvier]

Tried to unify this meson.build with tests/tcg/plugins/meson.build but
the resulting modules are not output in the right directory.

Originally proposed by Anton Kochkov, thank you!

Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 meson.build                 |  4 ++++
 contrib/plugins/meson.build | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 contrib/plugins/meson.build

diff --git a/meson.build b/meson.build
index 52e5aa95cc0..4efe1e782ba 100644
--- a/meson.build
+++ b/meson.build
@@ -3627,6 +3627,10 @@ subdir('accel')
 subdir('plugins')
 subdir('ebpf')
 
+if 'CONFIG_TCG' in config_all_accel
+  subdir('contrib/plugins')
+endif
+
 common_user_inc = []
 
 subdir('common-user')
diff --git a/contrib/plugins/meson.build b/contrib/plugins/meson.build
new file mode 100644
index 00000000000..a0e026d25e2
--- /dev/null
+++ b/contrib/plugins/meson.build
@@ -0,0 +1,23 @@
+t = []
+if get_option('plugins')
+  foreach i : ['cache', 'drcov', 'execlog', 'hotblocks', 'hotpages', 'howvec',
+               'hwprofile', 'ips', 'lockstep', 'stoptrigger']
+    if host_os == 'windows'
+      t += shared_module(i, files(i + '.c') + 'win32_linker.c',
+                        include_directories: '../../include/qemu',
+                        link_depends: [win32_qemu_plugin_api_lib],
+                        link_args: ['-Lplugins', '-lqemu_plugin_api'],
+                        dependencies: glib)
+
+    else
+      t += shared_module(i, files(i + '.c'),
+                        include_directories: '../../include/qemu',
+                        dependencies: glib)
+    endif
+  endforeach
+endif
+if t.length() > 0
+  alias_target('contrib-plugins', t)
+else
+  run_target('contrib-plugins', command: find_program('true'))
+endif
-- 
2.39.2

[PATCH 6/6] contrib/plugins: remove Makefile for contrib/plugins

[Pierrick Bouvier]

Now replaced by meson build.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 configure                | 18 ---------
 Makefile                 | 10 -----
 contrib/plugins/Makefile | 85 ----------------------------------------
 3 files changed, 113 deletions(-)
 delete mode 100644 contrib/plugins/Makefile

diff --git a/configure b/configure
index 019fcbd0ef7..2c0f708cd0a 100755
--- a/configure
+++ b/configure
@@ -1030,7 +1030,6 @@ if test "$static" = "yes" ; then
 fi
 if test "$plugins" != "no"; then
   plugins=yes
-  subdirs="$subdirs contrib/plugins"
 fi
 
 cat > $TMPC << EOF
@@ -1531,7 +1530,6 @@ LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
 LINKS="$LINKS tests/avocado tests/data"
 LINKS="$LINKS tests/qemu-iotests/check tests/qemu-iotests/Makefile"
 LINKS="$LINKS python"
-LINKS="$LINKS contrib/plugins/Makefile "
 for f in $LINKS ; do
     if [ -e "$source_path/$f" ]; then
         symlink "$source_path/$f" "$f"
@@ -1614,22 +1612,6 @@ if test "$default_targets" = "yes"; then
   echo "CONFIG_DEFAULT_TARGETS=y" >> $config_host_mak
 fi
 
-# contrib/plugins configuration
-echo "# Automatically generated by configure - do not modify" > contrib/plugins/$config_host_mak
-echo "SRC_PATH=$source_path/contrib/plugins" >> contrib/plugins/$config_host_mak
-echo "PKG_CONFIG=${pkg_config}" >> contrib/plugins/$config_host_mak
-echo "CC=$cc $CPU_CFLAGS" >> contrib/plugins/$config_host_mak
-echo "CFLAGS=${CFLAGS-$default_cflags} $EXTRA_CFLAGS" >> contrib/plugins/$config_host_mak
-if test "$host_os" = windows; then
-  echo "DLLTOOL=$dlltool" >> contrib/plugins/$config_host_mak
-fi
-if test "$host_os" = darwin; then
-  echo "CONFIG_DARWIN=y" >> contrib/plugins/$config_host_mak
-fi
-if test "$host_os" = windows; then
-  echo "CONFIG_WIN32=y" >> contrib/plugins/$config_host_mak
-fi
-
 # tests/tcg configuration
 mkdir -p tests/tcg
 echo "# Automatically generated by configure - do not modify" > tests/tcg/$config_host_mak
diff --git a/Makefile b/Makefile
index 02a257584ba..e474fbe50de 100644
--- a/Makefile
+++ b/Makefile
@@ -186,11 +186,6 @@ SUBDIR_RULES=$(foreach t, all clean distclean, $(addsuffix /$(t), $(SUBDIRS)))
 $(SUBDIR_RULES):
 	$(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) V="$(V)" TARGET_DIR="$(dir $@)" $(notdir $@),)
 
-ifneq ($(filter contrib/plugins, $(SUBDIRS)),)
-.PHONY: plugins
-plugins: contrib/plugins/all
-endif
-
 .PHONY: recurse-all recurse-clean
 recurse-all: $(addsuffix /all, $(SUBDIRS))
 recurse-clean: $(addsuffix /clean, $(SUBDIRS))
@@ -306,11 +301,6 @@ help:
 	$(call print-help,cscope,Generate cscope index)
 	$(call print-help,sparse,Run sparse on the QEMU source)
 	@echo  ''
-ifneq ($(filter contrib/plugins, $(SUBDIRS)),)
-	@echo  'Plugin targets:'
-	$(call print-help,plugins,Build the example TCG plugins)
-	@echo  ''
-endif
 	@echo  'Cleaning targets:'
 	$(call print-help,clean,Remove most generated files but keep the config)
 	$(call print-help,distclean,Remove all generated files)
diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
deleted file mode 100644
index edf256cd9d1..00000000000
--- a/contrib/plugins/Makefile
+++ /dev/null
@@ -1,85 +0,0 @@
-# -*- Mode: makefile -*-
-#
-# This Makefile example is fairly independent from the main makefile
-# so users can take and adapt it for their build. We only really
-# include config-host.mak so we don't have to repeat probing for
-# programs that the main configure has already done for us.
-#
-
-include config-host.mak
-
-TOP_SRC_PATH = $(SRC_PATH)/../..
-
-VPATH += $(SRC_PATH)
-
-NAMES :=
-NAMES += execlog
-NAMES += hotblocks
-NAMES += hotpages
-NAMES += howvec
-
-# The lockstep example communicates using unix sockets,
-# and can't be easily made to work on windows.
-ifneq ($(CONFIG_WIN32),y)
-NAMES += lockstep
-endif
-
-NAMES += hwprofile
-NAMES += cache
-NAMES += drcov
-NAMES += ips
-NAMES += stoptrigger
-
-ifeq ($(CONFIG_WIN32),y)
-SO_SUFFIX := .dll
-LDLIBS += $(shell $(PKG_CONFIG) --libs glib-2.0)
-else
-SO_SUFFIX := .so
-endif
-
-SONAMES := $(addsuffix $(SO_SUFFIX),$(addprefix lib,$(NAMES)))
-
-# The main QEMU uses Glib extensively so it is perfectly fine to use it
-# in plugins (which many example do).
-PLUGIN_CFLAGS := $(shell $(PKG_CONFIG) --cflags glib-2.0)
-PLUGIN_CFLAGS += -fPIC -Wall
-PLUGIN_CFLAGS += -I$(TOP_SRC_PATH)/include/qemu
-
-# Helper that honours V=1 so we get some output when compiling
-quiet-@ = $(if $(V),,@$(if $1,printf "  %-7s %s\n" "$(strip $1)" "$(strip $2)" && ))
-quiet-command = $(call quiet-@,$2,$3)$1
-
-# for including , in command strings
-COMMA := ,
-
-all: $(SONAMES)
-
-%.o: %.c
-	$(call quiet-command, \
-		$(CC) $(CFLAGS) $(PLUGIN_CFLAGS) -c -o $@ $<, \
-	        BUILD, plugin $@)
-
-ifeq ($(CONFIG_WIN32),y)
-lib%$(SO_SUFFIX): %.o win32_linker.o ../../plugins/libqemu_plugin_api.a
-	$(call quiet-command, \
-		$(CC) -shared -o $@ $^ $(LDLIBS), \
-		LINK, plugin $@)
-else ifeq ($(CONFIG_DARWIN),y)
-lib%$(SO_SUFFIX): %.o
-	$(call quiet-command, \
-		$(CC) -bundle -Wl$(COMMA)-undefined$(COMMA)dynamic_lookup -o $@ $^ $(LDLIBS), \
-		LINK, plugin $@)
-else
-lib%$(SO_SUFFIX): %.o
-	$(call quiet-command, \
-		$(CC) -shared -o $@ $^ $(LDLIBS), \
-		LINK, plugin $@)
-endif
-
-
-clean:
-	rm -f *.o *$(SO_SUFFIX) *.d
-	rm -Rf .libs
-
-.PHONY: all clean
-.SECONDARY:
-- 
2.39.2

Re: [PATCH 0/6] build contrib/plugins using meson

[Paolo Bonzini]

On Thu, Aug 15, 2024 at 1:37 AM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
> Contrib plugins have been built out of tree so far, thanks to a Makefile.
> However, it is quite inconvenient for maintenance, as we may break them,
> especially for specific architectures.
>
> First patches are fixing warnings for existing plugins, then we add meson
> support, and finally, we remove Makefile for contrib/plugins.
>
> Based on the proposal of Anton Kochkov on associated gitlab issue.
> Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710

Is the bug actually still there?

The Makefile explains why it was done this way:

# This Makefile example is fairly independent from the main makefile
# so users can take and adapt it for their build. We only really
# include config-host.mak so we don't have to repeat probing for
# programs that the main configure has already done for us.

In other words we should also take into account that there is a
documentation benefit to having a Makefile that works across Windows,
Darwin and generic ELF Unices. Anyway Philippe, Akihiko and Alex are
the best people to decide.

One argument from moving contrib/plugins to meson is that the Windows
case depends on libqemu_plugin_api.a which is built with meson(*);
that said, libqemu_plugin_api.a should be installed - which would
justify it being used from an "external" makefile.

Paolo

(*) by the way,

Re: [PATCH 0/6] build contrib/plugins using meson

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Thu, Aug 15, 2024 at 1:37 AM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>> Contrib plugins have been built out of tree so far, thanks to a Makefile.
>> However, it is quite inconvenient for maintenance, as we may break them,
>> especially for specific architectures.
>>
>> First patches are fixing warnings for existing plugins, then we add meson
>> support, and finally, we remove Makefile for contrib/plugins.
>>
>> Based on the proposal of Anton Kochkov on associated gitlab issue.
>> Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710
>
> Is the bug actually still there?
>
> The Makefile explains why it was done this way:
>
> # This Makefile example is fairly independent from the main makefile
> # so users can take and adapt it for their build. We only really
> # include config-host.mak so we don't have to repeat probing for
> # programs that the main configure has already done for us.
>
> In other words we should also take into account that there is a
> documentation benefit to having a Makefile that works across Windows,
> Darwin and generic ELF Unices. Anyway Philippe, Akihiko and Alex are
> the best people to decide.

We could keep the Makefile as an example but the meson file looks fairly
easy to read. However it keeps growing warts to adapt to the fact its
not integrated with the wider project.

> One argument from moving contrib/plugins to meson is that the Windows
> case depends on libqemu_plugin_api.a which is built with meson(*);
> that said, libqemu_plugin_api.a should be installed - which would
> justify it being used from an "external" makefile.
>
> Paolo
>
> (*) by the way,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/6] build contrib/plugins using meson

[Pierrick Bouvier]

On 8/15/24 04:42, Alex Bennée wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On Thu, Aug 15, 2024 at 1:37 AM Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>>> Contrib plugins have been built out of tree so far, thanks to a Makefile.
>>> However, it is quite inconvenient for maintenance, as we may break them,
>>> especially for specific architectures.
>>>
>>> First patches are fixing warnings for existing plugins, then we add meson
>>> support, and finally, we remove Makefile for contrib/plugins.
>>>
>>> Based on the proposal of Anton Kochkov on associated gitlab issue.
>>> Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710
>>
>> Is the bug actually still there?
>>
>> The Makefile explains why it was done this way:
>>
>> # This Makefile example is fairly independent from the main makefile
>> # so users can take and adapt it for their build. We only really
>> # include config-host.mak so we don't have to repeat probing for
>> # programs that the main configure has already done for us.
>>
>> In other words we should also take into account that there is a
>> documentation benefit to having a Makefile that works across Windows,
>> Darwin and generic ELF Unices. Anyway Philippe, Akihiko and Alex are
>> the best people to decide.
> 
> We could keep the Makefile as an example but the meson file looks fairly
> easy to read. However it keeps growing warts to adapt to the fact its
> not integrated with the wider project.
> 

As you wish, but I think it's very confusing to have both.

In more, you *need* meson to build the lib to which the plugin is 
linked, so it's not possible to compile a plugin with only a Makefile 
from scratch.

>> One argument from moving contrib/plugins to meson is that the Windows
>> case depends on libqemu_plugin_api.a which is built with meson(*);
>> that said, libqemu_plugin_api.a should be installed - which would
>> justify it being used from an "external" makefile.
>>
>> Paolo
>>
>> (*) by the way,
> 

Re: [PATCH 0/6] build contrib/plugins using meson

[Pierrick Bouvier]

On 8/14/24 23:00, Paolo Bonzini wrote:
> On Thu, Aug 15, 2024 at 1:37 AM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>> Contrib plugins have been built out of tree so far, thanks to a Makefile.
>> However, it is quite inconvenient for maintenance, as we may break them,
>> especially for specific architectures.
>>
>> First patches are fixing warnings for existing plugins, then we add meson
>> support, and finally, we remove Makefile for contrib/plugins.
>>
>> Based on the proposal of Anton Kochkov on associated gitlab issue.
>> Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710
> 
> Is the bug actually still there?
> 

Maybe the changes you did fixed the portability issue. I just added this 
"solves" because it's where the idea to compile with meson was presented 
initially.

> The Makefile explains why it was done this way:
> 
> # This Makefile example is fairly independent from the main makefile
> # so users can take and adapt it for their build. We only really
> # include config-host.mak so we don't have to repeat probing for
> # programs that the main configure has already done for us.
> 
> In other words we should also take into account that there is a
> documentation benefit to having a Makefile that works across Windows,
> Darwin and generic ELF Unices. Anyway Philippe, Akihiko and Alex are
> the best people to decide.
>  > One argument from moving contrib/plugins to meson is that the Windows
> case depends on libqemu_plugin_api.a which is built with meson(*);
> that said, libqemu_plugin_api.a should be installed - which would
> justify it being used from an "external" makefile.
>

You need meson to build this lib in the first place, so I guess that 
99.9% of the people writing a plugin will have a qemu source tree (with 
access to plugin headers), and first compile the lib.

I am not convinced by the scenario where people build this out of tree 
to be honest, but I may be wrong.

> Paolo
> 
> (*) by the way,
> 

Re: [PATCH 0/6] build contrib/plugins using meson

[Paolo Bonzini]

On Thu, Aug 15, 2024 at 8:04 PM Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
> > One argument from moving contrib/plugins to meson is that the Windows
> > case depends on libqemu_plugin_api.a which is built with meson(*);
> > that said, libqemu_plugin_api.a should be installed - which would
> > justify it being used from an "external" makefile.
>
> You need meson to build this lib in the first place, so I guess that
> 99.9% of the people writing a plugin will have a qemu source tree (with
> access to plugin headers), and first compile the lib.

Note that the lib is built at configure time, not Make time.  It's not
a normal library.

> I am not convinced by the scenario where people build this out of tree
> to be honest, but I may be wrong.

The idea is that people copy the Makefile, and yeah I'm not sure
either if they do it but it can be useful.

Paolo

Re: [PATCH 0/6] build contrib/plugins using meson

[Peter Maydell]

On Thu, 15 Aug 2024 at 19:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Thu, Aug 15, 2024 at 8:04 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> > > One argument from moving contrib/plugins to meson is that the Windows
> > > case depends on libqemu_plugin_api.a which is built with meson(*);
> > > that said, libqemu_plugin_api.a should be installed - which would
> > > justify it being used from an "external" makefile.
> >
> > You need meson to build this lib in the first place, so I guess that
> > 99.9% of the people writing a plugin will have a qemu source tree (with
> > access to plugin headers), and first compile the lib.
>
> Note that the lib is built at configure time, not Make time.  It's not
> a normal library.
>
> > I am not convinced by the scenario where people build this out of tree
> > to be honest, but I may be wrong.
>
> The idea is that people copy the Makefile, and yeah I'm not sure
> either if they do it but it can be useful.

The original theory was that since plugins are supposed to
be independent of QEMU version that we should have the
build system also be independent of QEMU (and perhaps that
plugins might not be in the QEMU source tree at all some day).
That's why they're in contrib/ (though I am not a fan of
contrib/ as an organizational category in the filesystem...)

-- PMM

Re: [PATCH 0/6] build contrib/plugins using meson

[Philippe Mathieu-Daudé]

On 15/8/24 20:04, Pierrick Bouvier wrote:
> On 8/14/24 23:00, Paolo Bonzini wrote:
>> On Thu, Aug 15, 2024 at 1:37 AM Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>>> Contrib plugins have been built out of tree so far, thanks to a 
>>> Makefile.
>>> However, it is quite inconvenient for maintenance, as we may break them,
>>> especially for specific architectures.
>>>
>>> First patches are fixing warnings for existing plugins, then we add 
>>> meson
>>> support, and finally, we remove Makefile for contrib/plugins.
>>>
>>> Based on the proposal of Anton Kochkov on associated gitlab issue.
>>> Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710
>>
>> Is the bug actually still there?
>>
> 
> Maybe the changes you did fixed the portability issue. I just added this 
> "solves" because it's where the idea to compile with meson was presented 
> initially.
> 
>> The Makefile explains why it was done this way:
>>
>> # This Makefile example is fairly independent from the main makefile
>> # so users can take and adapt it for their build. We only really
>> # include config-host.mak so we don't have to repeat probing for
>> # programs that the main configure has already done for us.
>>
>> In other words we should also take into account that there is a
>> documentation benefit to having a Makefile that works across Windows,
>> Darwin and generic ELF Unices. Anyway Philippe, Akihiko and Alex are
>> the best people to decide.
>>  > One argument from moving contrib/plugins to meson is that the Windows
>> case depends on libqemu_plugin_api.a which is built with meson(*);
>> that said, libqemu_plugin_api.a should be installed - which would
>> justify it being used from an "external" makefile.
>>
> 
> You need meson to build this lib in the first place, so I guess that 
> 99.9% of the people writing a plugin will have a qemu source tree (with 
> access to plugin headers), and first compile the lib.
> 
> I am not convinced by the scenario where people build this out of tree 
> to be honest, but I may be wrong.

Consider users interested to use plugins (not really develop them).
They should be able to build the plugins with their distrib QEMU,
not cloning the whole git tree. (At least this is how I understood
when Alex explained me the plugins power and simplicity for users.)

Re: [PATCH 0/6] build contrib/plugins using meson

[Pierrick Bouvier]

I don't have strong opinion in the end, I'm just looking for building 
contrib plugins like the rest of the codebase, to reduce maintenance burden.

Initially, when this idea was implemented, it was suggested by Alex to 
remove the Makefile [1].
Now it seems to be suggested to keep it as an example. So I'll let you 
decide what's the best, I'll follow that.

[1] 
https://lore.kernel.org/qemu-devel/20230615141315.961315-1-anton.kochkov@proton.me/T/#m2237fb20b9a53bd2ddec221831692090cda14c6f 


On 8/15/24 23:50, Philippe Mathieu-Daudé wrote:
> On 15/8/24 20:04, Pierrick Bouvier wrote:
>> On 8/14/24 23:00, Paolo Bonzini wrote:
>>> On Thu, Aug 15, 2024 at 1:37 AM Pierrick Bouvier
>>> <pierrick.bouvier@linaro.org> wrote:
>>>> Contrib plugins have been built out of tree so far, thanks to a
>>>> Makefile.
>>>> However, it is quite inconvenient for maintenance, as we may break them,
>>>> especially for specific architectures.
>>>>
>>>> First patches are fixing warnings for existing plugins, then we add
>>>> meson
>>>> support, and finally, we remove Makefile for contrib/plugins.
>>>>
>>>> Based on the proposal of Anton Kochkov on associated gitlab issue.
>>>> Solves: https://gitlab.com/qemu-project/qemu/-/issues/1710
>>>
>>> Is the bug actually still there?
>>>
>>
>> Maybe the changes you did fixed the portability issue. I just added this
>> "solves" because it's where the idea to compile with meson was presented
>> initially.
>>
>>> The Makefile explains why it was done this way:
>>>
>>> # This Makefile example is fairly independent from the main makefile
>>> # so users can take and adapt it for their build. We only really
>>> # include config-host.mak so we don't have to repeat probing for
>>> # programs that the main configure has already done for us.
>>>
>>> In other words we should also take into account that there is a
>>> documentation benefit to having a Makefile that works across Windows,
>>> Darwin and generic ELF Unices. Anyway Philippe, Akihiko and Alex are
>>> the best people to decide.
>>>   > One argument from moving contrib/plugins to meson is that the Windows
>>> case depends on libqemu_plugin_api.a which is built with meson(*);
>>> that said, libqemu_plugin_api.a should be installed - which would
>>> justify it being used from an "external" makefile.
>>>
>>
>> You need meson to build this lib in the first place, so I guess that
>> 99.9% of the people writing a plugin will have a qemu source tree (with
>> access to plugin headers), and first compile the lib.
>>
>> I am not convinced by the scenario where people build this out of tree
>> to be honest, but I may be wrong.
> 
> Consider users interested to use plugins (not really develop them).
> They should be able to build the plugins with their distrib QEMU,
> not cloning the whole git tree. (At least this is how I understood
> when Alex explained me the plugins power and simplicity for users.)