| 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
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. List of people who help him (in chronological order): - Richard Henderson - Sarah Harris and Edward Robbins - Philippe Mathieu-Daud=C3=A9 and Aleksandar Markovic - Pavel Dovgalyuk - Thomas Huth [*] The oldest contribution I could find on the list is from 2016: https://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg02985.html Tests included: $ avocado --show=3Dapp run -t arch:avr tests/acceptance/ Fetching asset from tests/acceptance/machine_avr6.py:AVR6Machine.test_freertos (1/1) tests/acceptance/machine_avr6.py:AVR6Machine.test_freertos: PASS (2.13= s) RESULTS : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANC= EL 0 JOB TIME : 2.35 s $ make check-qtest-avr TEST check-qtest-avr: tests/qtest/boot-serial-test TEST check-qtest-avr: tests/qtest/cdrom-test TEST check-qtest-avr: tests/qtest/device-introspect-test TEST check-qtest-avr: tests/qtest/machine-none-test TEST check-qtest-avr: tests/qtest/qmp-test TEST check-qtest-avr: tests/qtest/qmp-cmd-test TEST check-qtest-avr: tests/qtest/qom-test TEST check-qtest-avr: tests/qtest/test-hmp TEST check-qtest-avr: tests/qtest/qos-test CI results: . https://cirrus-ci.com/build/5697049146425344 . https://gitlab.com/philmd/qemu/-/pipelines/163985815 . https://travis-ci.org/github/philmd/qemu/builds/705817933 . https://app.shippable.com/github/philmd/qemu/runs/822/summary/console ---------------------------------------------------------------- Michael Rolnik (25): target/avr: Add basic parameters of the new platform target/avr: Introduce basic CPU class object target/avr: CPU class: Add interrupt handling support target/avr: CPU class: Add memory menagement support target/avr: CPU class: Add migration support target/avr: CPU class: Add GDB support target/avr: Introduce enumeration AVRFeature target/avr: Add definitions of AVR core types target/avr: Add instruction helpers target/avr: Add instruction translation - Register definitions target/avr: Add instruction translation - Arithmetic and Logic Instructions target/avr: Add instruction translation - Branch Instructions target/avr: Add instruction translation - Data Transfer Instructions target/avr: Add instruction translation - Bit and Bit-test Instructions target/avr: Add instruction translation - MCU Control Instructions target/avr: Add instruction translation - CPU main translation function target/avr: Initialize TCG register variables target/avr: Add support for disassembling via option '-d in_asm' target/avr: Register AVR support with the rest of QEMU tests/machine-none: Add AVR support hw/char: avr: Add limited support for USART peripheral hw/timer: avr: Add limited support for 16-bit timer peripheral hw/misc: avr: Add limited support for power reduction device tests/boot-serial: Test some Arduino boards (AVR based) tests/acceptance: Test the Arduino MEGA2560 board Philippe Mathieu-Daud=C3=A9 (6): hw/avr: Add support for loading ELF/raw binaries hw/avr: Add some ATmega microcontrollers hw/avr: Add limited support for some Arduino boards target/avr/cpu: Drop tlb_flush() in avr_cpu_reset() target/avr/cpu: Fix $PC displayed address target/avr/disas: Fix store instructions display order Thomas Huth (1): target/avr: Add section into QEMU documentation docs/system/target-avr.rst | 37 + docs/system/targets.rst | 1 + configure | 7 + default-configs/avr-softmmu.mak | 5 + qapi/machine.json | 3 +- hw/avr/atmega.h | 48 + hw/avr/boot.h | 33 + include/disas/dis-asm.h | 19 + include/elf.h | 4 + include/hw/char/avr_usart.h | 93 + include/hw/misc/avr_power.h | 46 + include/hw/timer/avr_timer16.h | 94 + include/sysemu/arch_init.h | 1 + target/avr/cpu-param.h | 36 + target/avr/cpu-qom.h | 54 + target/avr/cpu.h | 256 +++ target/avr/helper.h | 29 + target/avr/insn.decode | 187 ++ arch_init.c | 2 + hw/avr/arduino.c | 149 ++ hw/avr/atmega.c | 458 +++++ hw/avr/boot.c | 115 ++ hw/char/avr_usart.c | 320 ++++ hw/misc/avr_power.c | 113 ++ hw/timer/avr_timer16.c | 621 ++++++ target/avr/cpu.c | 367 ++++ target/avr/disas.c | 246 +++ target/avr/gdbstub.c | 84 + target/avr/helper.c | 342 ++++ target/avr/machine.c | 121 ++ target/avr/translate.c | 3061 ++++++++++++++++++++++++++++++ tests/qtest/boot-serial-test.c | 11 + tests/qtest/machine-none-test.c | 1 + MAINTAINERS | 30 + gdb-xml/avr-cpu.xml | 49 + hw/Kconfig | 1 + hw/avr/Kconfig | 9 + hw/avr/Makefile.objs | 3 + hw/char/Kconfig | 3 + hw/char/Makefile.objs | 1 + hw/misc/Kconfig | 3 + hw/misc/Makefile.objs | 2 + hw/misc/trace-events | 4 + hw/timer/Kconfig | 3 + hw/timer/Makefile.objs | 2 + hw/timer/trace-events | 12 + target/avr/Makefile.objs | 34 + tests/acceptance/machine_avr6.py | 50 + tests/qtest/Makefile.include | 2 + 49 files changed, 7171 insertions(+), 1 deletion(-) create mode 100644 docs/system/target-avr.rst create mode 100644 default-configs/avr-softmmu.mak create mode 100644 hw/avr/atmega.h create mode 100644 hw/avr/boot.h create mode 100644 include/hw/char/avr_usart.h create mode 100644 include/hw/misc/avr_power.h create mode 100644 include/hw/timer/avr_timer16.h create mode 100644 target/avr/cpu-param.h create mode 100644 target/avr/cpu-qom.h create mode 100644 target/avr/cpu.h create mode 100644 target/avr/helper.h create mode 100644 target/avr/insn.decode create mode 100644 hw/avr/arduino.c create mode 100644 hw/avr/atmega.c create mode 100644 hw/avr/boot.c create mode 100644 hw/char/avr_usart.c create mode 100644 hw/misc/avr_power.c create mode 100644 hw/timer/avr_timer16.c create mode 100644 target/avr/cpu.c create mode 100644 target/avr/disas.c create mode 100644 target/avr/gdbstub.c create mode 100644 target/avr/helper.c create mode 100644 target/avr/machine.c create mode 100644 target/avr/translate.c create mode 100644 gdb-xml/avr-cpu.xml create mode 100644 hw/avr/Kconfig create mode 100644 hw/avr/Makefile.objs create mode 100644 target/avr/Makefile.objs create mode 100644 tests/acceptance/machine_avr6.py --=20 2.21.3