diff mbox series

[RFC] cputlb: use uint64_t for interim values for unaligned load

Message ID 20190603150120.29255-1-alex.bennee@linaro.org
State Superseded
Headers show
Series [RFC] cputlb: use uint64_t for interim values for unaligned load | expand

Commit Message

Alex Bennée June 3, 2019, 3:01 p.m. UTC
When running on 32 bit TCG backends a wide unaligned load ends up
truncating data before returning to the guest. We specifically have
the return type as uint64_t to avoid any premature truncation so we
should use the same for the interim types.

Hopefully fixes #1830872

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 accel/tcg/cputlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.20.1

Comments

Andrew Randrianasulu June 3, 2019, 3:35 p.m. UTC | #1
В сообщении от Monday 03 June 2019 18:01:20 Alex Bennée написал(а):
> When running on 32 bit TCG backends a wide unaligned load ends up

> truncating data before returning to the guest. We specifically have

> the return type as uint64_t to avoid any premature truncation so we

> should use the same for the interim types.

> 

> Hopefully fixes #1830872

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  accel/tcg/cputlb.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c

> index cdcc3771020..b796ab1cbea 100644

> --- a/accel/tcg/cputlb.c

> +++ b/accel/tcg/cputlb.c

> @@ -1303,7 +1303,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,

>          && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1

>                      >= TARGET_PAGE_SIZE)) {

>          target_ulong addr1, addr2;

> -        tcg_target_ulong r1, r2;

> +        uint64_t r1, r2;

>          unsigned shift;

>      do_unaligned_access:

>          addr1 = addr & ~(size - 1);


Unfortunatly, this doesn't fix 32-bit qemu-system-x86_64 .... so, my bug is separate from #1830872 ?

I also was unable to convince qemu to use my kernel-only x86_64 gcc 6.5.0 cross-compiler ..
probably x86-64 testing on i686 requires either docker (I don't have this
) or 'real' cross-compiler (build with glibc support).
Richard Henderson June 3, 2019, 10:01 p.m. UTC | #2
On 6/3/19 10:01 AM, Alex Bennée wrote:
> When running on 32 bit TCG backends a wide unaligned load ends up

> truncating data before returning to the guest. We specifically have

> the return type as uint64_t to avoid any premature truncation so we

> should use the same for the interim types.

> 

> Hopefully fixes #1830872

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  accel/tcg/cputlb.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>



r~
Philippe Mathieu-Daudé June 4, 2019, 6:52 a.m. UTC | #3
On 6/3/19 5:01 PM, Alex Bennée wrote:
> When running on 32 bit TCG backends a wide unaligned load ends up

> truncating data before returning to the guest. We specifically have

> the return type as uint64_t to avoid any premature truncation so we

> should use the same for the interim types.

> 

> Hopefully fixes #1830872


Maybe clearer as:

Fixes: https://bugs.launchpad.net/qemu/+bug/1830872
Fixes: eed5664238e

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  accel/tcg/cputlb.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c

> index cdcc3771020..b796ab1cbea 100644

> --- a/accel/tcg/cputlb.c

> +++ b/accel/tcg/cputlb.c

> @@ -1303,7 +1303,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,

>          && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1

>                      >= TARGET_PAGE_SIZE)) {

>          target_ulong addr1, addr2;

> -        tcg_target_ulong r1, r2;

> +        uint64_t r1, r2;

>          unsigned shift;

>      do_unaligned_access:

>          addr1 = addr & ~(size - 1);

> 


Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Alex Bennée June 4, 2019, 9:43 a.m. UTC | #4
Andrew Randrianasulu <randrianasulu@gmail.com> writes:

> В сообщении от Monday 03 June 2019 18:01:20 Alex Bennée написал(а):

>> When running on 32 bit TCG backends a wide unaligned load ends up

>> truncating data before returning to the guest. We specifically have

>> the return type as uint64_t to avoid any premature truncation so we

>> should use the same for the interim types.

>>

>> Hopefully fixes #1830872

>>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>  accel/tcg/cputlb.c | 2 +-

>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c

>> index cdcc3771020..b796ab1cbea 100644

>> --- a/accel/tcg/cputlb.c

>> +++ b/accel/tcg/cputlb.c

>> @@ -1303,7 +1303,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,

>>          && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1

>>                      >= TARGET_PAGE_SIZE)) {

>>          target_ulong addr1, addr2;

>> -        tcg_target_ulong r1, r2;

>> +        uint64_t r1, r2;

>>          unsigned shift;

>>      do_unaligned_access:

>>          addr1 = addr & ~(size - 1);

>

> Unfortunatly, this doesn't fix 32-bit qemu-system-x86_64 .... so, my

> bug is separate from #1830872 ?


I think you've hit two - one of which we have just fixed. With my
expanded memory test on i386 I'm seeing a hang but it's ok @
pull-demacro-softmmu-100519-1. Unfortunately bisecting through the slirp
move and other i386 Werror stuff is proving painful.

>

> I also was unable to convince qemu to use my kernel-only x86_64 gcc 6.5.0 cross-compiler ..

> probably x86-64 testing on i686 requires either docker (I don't have this

> ) or 'real' cross-compiler (build with glibc support).



--
Alex Bennée
Igor Mammedov June 4, 2019, 11:42 a.m. UTC | #5
On Mon,  3 Jun 2019 16:01:20 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> When running on 32 bit TCG backends a wide unaligned load ends up

> truncating data before returning to the guest. We specifically have

> the return type as uint64_t to avoid any premature truncation so we

> should use the same for the interim types.

> 

> Hopefully fixes #1830872

> 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


Fixes arm/virt bios-tables-test for me, so

Tested-by: Igor Mammedov <imammedo@redhat.com>


> ---

>  accel/tcg/cputlb.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c

> index cdcc3771020..b796ab1cbea 100644

> --- a/accel/tcg/cputlb.c

> +++ b/accel/tcg/cputlb.c

> @@ -1303,7 +1303,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,

>          && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1

>                      >= TARGET_PAGE_SIZE)) {  

>          target_ulong addr1, addr2;

> -        tcg_target_ulong r1, r2;

> +        uint64_t r1, r2;

>          unsigned shift;

>      do_unaligned_access:

>          addr1 = addr & ~(size - 1);
diff mbox series

Patch

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index cdcc3771020..b796ab1cbea 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1303,7 +1303,7 @@  load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
                     >= TARGET_PAGE_SIZE)) {
         target_ulong addr1, addr2;
-        tcg_target_ulong r1, r2;
+        uint64_t r1, r2;
         unsigned shift;
     do_unaligned_access:
         addr1 = addr & ~(size - 1);