Message ID | 20200707181710.30950-1-f4bug@amsat.org |
---|---|
State | New |
Headers | show |
On Tue, 7 Jul 2020 at 19:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Possible false-positives from checkpatch: > > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > > The following changes since commit 7623b5ba017f61de5d7c2bba12c6feb3d55091b1: > > Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-5.1-pull-= > request' into staging (2020-07-06 11:40:10 +0100) > > are available in the Git repository at: > > https://gitlab.com/philmd/qemu.git tags/avr-port-20200707 > > for you to fetch changes up to 0cdaf2f343491f60dbf7dd2a912cd88b5f9f899c: > > target/avr/disas: Fix store instructions display order (2020-07-07 20:14:15= > +0200) > > ---------------------------------------------------------------- > 8bit AVR port from Michael Rolnik. > > Michael started to work on the AVR port few years ago [*] and kept > improving the code over various series. Hi; I'm afraid this fails "make check" on big-endian hosts (s390x, ppc64): MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} QTEST_QEMU_BINARY=avr-softmmu/qemu-system-avr QTEST_QEMU_IMG=qemu-img tests/qtest/boot-serial-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl --test-name="boot-serial-test" qemu-system-avr: execution left flash memory ** (tests/qtest/boot-serial-test:24048): ERROR **: 11:00:46.466: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sVGEXHI' ERROR - Bail out! FATAL-ERROR: Failed to find expected string. Please check '/tmp/qtest-boot-serial-sVGEXHI' thanks -- PMM
On 10/07/2020 13.41, Peter Maydell wrote: > On Tue, 7 Jul 2020 at 19:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> Possible false-positives from checkpatch: >> >> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? >> >> The following changes since commit 7623b5ba017f61de5d7c2bba12c6feb3d55091b1: >> >> Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-5.1-pull-= >> request' into staging (2020-07-06 11:40:10 +0100) >> >> are available in the Git repository at: >> >> https://gitlab.com/philmd/qemu.git tags/avr-port-20200707 >> >> for you to fetch changes up to 0cdaf2f343491f60dbf7dd2a912cd88b5f9f899c: >> >> target/avr/disas: Fix store instructions display order (2020-07-07 20:14:15= >> +0200) >> >> ---------------------------------------------------------------- >> 8bit AVR port from Michael Rolnik. >> >> Michael started to work on the AVR port few years ago [*] and kept >> improving the code over various series. > > Hi; I'm afraid this fails "make check" on big-endian hosts (s390x, ppc64): > > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > QTEST_QEMU_BINARY=avr-softmmu/qemu-system-avr QTEST_QEMU_IMG=qemu-img > tests/qtest/boot-serial-test -m=quick -k --tap < /dev/null | > ./scripts/tap-driver.pl --test-name="boot-serial-test" > qemu-system-avr: execution left flash memory > > ** (tests/qtest/boot-serial-test:24048): ERROR **: 11:00:46.466: > Failed to find expected string. Please check > '/tmp/qtest-boot-serial-sVGEXHI' > ERROR - Bail out! FATAL-ERROR: Failed to find expected string. Please > check '/tmp/qtest-boot-serial-sVGEXHI' Endianess bug ... this should fix it: diff --git a/target/avr/helper.c b/target/avr/helper.c --- a/target/avr/helper.c +++ b/target/avr/helper.c @@ -337,6 +337,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr) helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data); } else { /* memory */ - cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1); + uint8_t data8 = data; + cpu_physical_memory_write(OFFSET_DATA + addr, &data8, 1); } } Thomas
On Fri, 10 Jul 2020 at 16:03, Thomas Huth <thuth@redhat.com> wrote: > Endianess bug ... this should fix it: > > diff --git a/target/avr/helper.c b/target/avr/helper.c > --- a/target/avr/helper.c > +++ b/target/avr/helper.c > @@ -337,6 +337,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, > uint32_t addr) > helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data); > } else { > /* memory */ > - cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1); > + uint8_t data8 = data; > + cpu_physical_memory_write(OFFSET_DATA + addr, &data8, 1); > } Or equivalently address_space_stb(&address_space_memory, data, MEMTXATTRS_UNSPECIFIED, NULL); (better choices of address space may be available, but this is the exact-same-behaviour one). > } thanks -- PMM
On 7/10/20 5:12 PM, Peter Maydell wrote: > On Fri, 10 Jul 2020 at 16:03, Thomas Huth <thuth@redhat.com> wrote: >> Endianess bug ... this should fix it: >> >> diff --git a/target/avr/helper.c b/target/avr/helper.c >> --- a/target/avr/helper.c >> +++ b/target/avr/helper.c >> @@ -337,6 +337,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, >> uint32_t addr) >> helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data); >> } else { >> /* memory */ >> - cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1); >> + uint8_t data8 = data; >> + cpu_physical_memory_write(OFFSET_DATA + addr, &data8, 1); >> } > > Or equivalently > address_space_stb(&address_space_memory, data, MEMTXATTRS_UNSPECIFIED, NULL); > > (better choices of address space may be available, but this is > the exact-same-behaviour one). Ah, this is my stashed fix: -- >8 -- @@ -320,8 +320,10 @@ target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr) * this function implements ST instruction when there is a posibility to write * into a CPU register */ -void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr) +void helper_fullwr(CPUAVRState *env, uint32_t data32, uint32_t addr) { + uint8_t data = data32; + assert(data == data32); + env->fullacc = false; --- 3 ways to do the same :) The assert is probably superfluous. I don't like the fact that env->r[addr] (which is u8) is silently casted from u32.
On 7/10/20 5:17 PM, Philippe Mathieu-Daudé wrote: > On 7/10/20 5:12 PM, Peter Maydell wrote: >> On Fri, 10 Jul 2020 at 16:03, Thomas Huth <thuth@redhat.com> wrote: >>> Endianess bug ... this should fix it: >>> >>> diff --git a/target/avr/helper.c b/target/avr/helper.c >>> --- a/target/avr/helper.c >>> +++ b/target/avr/helper.c >>> @@ -337,6 +337,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, >>> uint32_t addr) >>> helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data); >>> } else { >>> /* memory */ >>> - cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1); >>> + uint8_t data8 = data; >>> + cpu_physical_memory_write(OFFSET_DATA + addr, &data8, 1); >>> } >> >> Or equivalently >> address_space_stb(&address_space_memory, data, MEMTXATTRS_UNSPECIFIED, NULL); >> >> (better choices of address space may be available, but this is >> the exact-same-behaviour one). > > Ah, this is my stashed fix: > > -- >8 -- > @@ -320,8 +320,10 @@ target_ulong helper_fullrd(CPUAVRState *env, > uint32_t addr) > * this function implements ST instruction when there is a posibility > to write > * into a CPU register > */ > -void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr) > +void helper_fullwr(CPUAVRState *env, uint32_t data32, uint32_t addr) > { > + uint8_t data = data32; > + assert(data == data32); > + > env->fullacc = false; > > --- > > 3 ways to do the same :) The assert is probably superfluous. > > I don't like the fact that env->r[addr] (which is u8) is silently casted > from u32. I'll squash Peter suggested fix: -- >8 -- --- a/target/avr/helper.c +++ b/target/avr/helper.c @@ -232,7 +232,9 @@ target_ulong helper_inb(CPUAVRState *env, uint32_t port) break; default: /* not a special register, pass to normal memory access */ - cpu_physical_memory_read(OFFSET_IO_REGISTERS + port, &data, 1); + data = address_space_ldub(&address_space_memory, + OFFSET_IO_REGISTERS + port, + MEMTXATTRS_UNSPECIFIED, NULL); } return data; @@ -289,7 +291,8 @@ void helper_outb(CPUAVRState *env, uint32_t port, uint32_t data) break; default: /* not a special register, pass to normal memory access */ - cpu_physical_memory_write(OFFSET_IO_REGISTERS + port, &data, 1); + address_space_stb(&address_space_memory, OFFSET_IO_REGISTERS + port, + data, MEMTXATTRS_UNSPECIFIED, NULL); } } @@ -305,13 +308,14 @@ target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr) if (addr < NUMBER_OF_CPU_REGISTERS) { /* CPU registers */ - data = env->r[addr]; + data = cpu_to_le32(env->r[addr]); } else if (addr < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) { /* IO registers */ data = helper_inb(env, addr - NUMBER_OF_CPU_REGISTERS); } else { /* memory */ - cpu_physical_memory_read(OFFSET_DATA + addr, &data, 1); + data = address_space_ldub(&address_space_memory, OFFSET_DATA + addr, + MEMTXATTRS_UNSPECIFIED, NULL); } return data; } @@ -337,6 +341,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr) helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data); } else { /* memory */ - cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1); + address_space_stb(&address_space_memory, OFFSET_DATA + addr, data, + MEMTXATTRS_UNSPECIFIED, NULL); } } ---
On 10/07/2020 17.32, Philippe Mathieu-Daudé wrote: > On 7/10/20 5:17 PM, Philippe Mathieu-Daudé wrote: >> On 7/10/20 5:12 PM, Peter Maydell wrote: >>> On Fri, 10 Jul 2020 at 16:03, Thomas Huth <thuth@redhat.com> wrote: >>>> Endianess bug ... this should fix it: >>>> >>>> diff --git a/target/avr/helper.c b/target/avr/helper.c >>>> --- a/target/avr/helper.c >>>> +++ b/target/avr/helper.c >>>> @@ -337,6 +337,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data, >>>> uint32_t addr) >>>> helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data); >>>> } else { >>>> /* memory */ >>>> - cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1); >>>> + uint8_t data8 = data; >>>> + cpu_physical_memory_write(OFFSET_DATA + addr, &data8, 1); >>>> } >>> >>> Or equivalently >>> address_space_stb(&address_space_memory, data, MEMTXATTRS_UNSPECIFIED, NULL); >>> >>> (better choices of address space may be available, but this is >>> the exact-same-behaviour one). >> >> Ah, this is my stashed fix: >> >> -- >8 -- >> @@ -320,8 +320,10 @@ target_ulong helper_fullrd(CPUAVRState *env, >> uint32_t addr) >> * this function implements ST instruction when there is a posibility >> to write >> * into a CPU register >> */ >> -void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr) >> +void helper_fullwr(CPUAVRState *env, uint32_t data32, uint32_t addr) >> { >> + uint8_t data = data32; >> + assert(data == data32); >> + >> env->fullacc = false; >> >> --- >> >> 3 ways to do the same :) The assert is probably superfluous. >> >> I don't like the fact that env->r[addr] (which is u8) is silently casted >> from u32. > > I'll squash Peter suggested fix: [...] > @@ -305,13 +308,14 @@ target_ulong helper_fullrd(CPUAVRState *env, > uint32_t addr) > > if (addr < NUMBER_OF_CPU_REGISTERS) { > /* CPU registers */ > - data = env->r[addr]; > + data = cpu_to_le32(env->r[addr]); That part looks wrong? Thomas
On 7/10/20 8:32 AM, Philippe Mathieu-Daudé wrote: > if (addr < NUMBER_OF_CPU_REGISTERS) { > /* CPU registers */ > - data = env->r[addr]; > + data = cpu_to_le32(env->r[addr]); This doesn't look right. Why? r~
On 7/10/20 5:54 PM, Richard Henderson wrote: > On 7/10/20 8:32 AM, Philippe Mathieu-Daudé wrote: >> if (addr < NUMBER_OF_CPU_REGISTERS) { >> /* CPU registers */ >> - data = env->r[addr]; >> + data = cpu_to_le32(env->r[addr]); > > This doesn't look right. Why? Sorry guys, false alarm, leftover from previous test but this is definitively not in the pull request :D