| Message ID | 20240814233645.944327-3-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | build contrib/plugins using meson | expand |
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
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
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 >
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~
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 >>
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~
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 >>
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
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~
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); } /*
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(-)