[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machin
|
From: |
Havard Skinnemoen |
|
Subject: |
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines |
|
Date: |
Tue, 8 Sep 2020 12:52:23 -0700 |
On Tue, Sep 8, 2020 at 9:58 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 9/8/20 5:52 PM, Philippe Mathieu-Daudé wrote:
> > On 9/8/20 5:02 PM, Alexander Bulekov wrote:
> >> Hi Havard,
> >> I fuzzed the npcm750-evb machine until I hit over 85% coverage over all
> >> the new npcm.*\.c files. The only thing I found specific to the new
> >> code, so far:
> >>
> >> cat << EOF | ./qemu-system-arm -machine npcm750-evb -m 128M -qtest stdio
> >> write 0xf0009040 0x4 0xc4c4c4c4
> >> write 0xf0009040 0x4 0x4
> >> EOF
> >
> > This is an odd test because with -qtest the timer is not running,
> > so this can not really happen on real hw.
> >
> > The fix is:
> >
> > - g_assert(t->remaining_ns > 0);
> > + g_assert(qtest_enabled() || t->remaining_ns > 0);
>
> Alex corrected me on IRC, qtest is irrelevant here.
> The problem is he disables the timer twice.
>
> So maybe something like:
>
> static void npcm7xx_timer_pause(NPCM7xxTimer *t)
> {
> int64_t now;
>
> + if (!timer_pending(&t->qtimer)) {
> + return;
> + }
> timer_del(&t->qtimer);
> now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> t->remaining_ns = t->expires_ns - now;
> g_assert(t->remaining_ns > 0);
> }
Thanks, that makes sense. I was worried that making the assert
conditional on qtest_enabled() might hide real issues.
This fuzz testing is great, it would have been hard to find this bug
without it. Thanks a lot Alex for running it.
Havard
> >
> >>
> >> ERROR:../hw/timer/npcm7xx_timer.c:160:npcm7xx_timer_pause: assertion
> >> failed: (t->remaining_ns > 0)
> >> Bail out! ERROR:../hw/timer/npcm7xx_timer.c:160:npcm7xx_timer_pause:
> >> assertion failed: (t->remaining_ns > 0)
> >> Aborted
> >>
> >> I'm doing the same for the quanta-gsj machine, but I'm not sure whether
> >> it will cover more code, so I'm happy to leave a:
> >>
> >> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> >>
> >> for the patches that add new virtual-device code (1-5, 7-12 ?)
> >> -Alex
> >
> > Very nice from you for testing running the fuzzer!
> >
> > Regards,
> >
> > Phil.
> >
> >>
> >>
> >> On 200824 1716, Havard Skinnemoen via wrote:
> >>> I also pushed this and the previous patchsets to my qemu fork on github.
> >>> The branches are named npcm7xx-v[1-8].
> >>>
> >>> https://github.com/hskinnemoen/qemu
> >>>
> >>> This patch series models enough of the Nuvoton NPCM730 and NPCM750 SoCs
> >>> to boot
> >>> an OpenBMC image built for quanta-gsj. This includes device models for:
> >>>
> >>> - Global Configuration Registers
> >>> - Clock Control
> >>> - Timers
> >>> - Fuses
> >>> - Memory Controller
> >>> - Flash Controller
> >>>
> >>> These modules, along with the existing Cortex A9 CPU cores and built-in
> >>> peripherals, are integrated into a NPCM730 or NPCM750 SoC, which in turn
> >>> form
> >>> the foundation for the quanta-gsj and npcm750-evb machines, respectively.
> >>> The
> >>> two SoCs are very similar; the only difference is that NPCM730 is missing
> >>> some
> >>> peripherals that NPCM750 has, and which are not considered essential for
> >>> datacenter use (e.g. graphics controllers). For more information, see
> >>>
> >>> https://www.nuvoton.com/products/cloud-computing/ibmc/
> >>>
> >>> Both quanta-gsj and npcm750-evb correspond to real boards supported by
> >>> OpenBMC.
> >>> At the end of the series, qemu can boot an OpenBMC image built for one of
> >>> these
> >>> boards with some minor modifications.
> >>>
> >>> The patches in this series were developed by Google and reviewed by
> >>> Nuvoton. We
> >>> will be maintaining the machine and peripheral support together.
> >>>
> >>> The data sheet for these SoCs is not generally available. Please let me
> >>> know if
> >>> more comments are needed to understand the device behavior.
> >>>
> >>> Changes since v7:
> >>>
> >>> - Move register enums to .c files throughout, leaving a single
> >>> NPCM7XX_FOO_NR_REGS definition behind in the .h file. A
> >>> QEMU_BUILD_BUG_ON
> >>> should alert anyone accidentally expanding the register enum that
> >>> they need
> >>> to update the corresponding NR_REGS define, which in turn has a
> >>> comment
> >>> reminding them to update the vmstate version_id as well.
> >>> - Skip loading the bootrom if a kernel filename is provided by the user.
> >>> - New patch adding a board setup stub to tweak clocks before booting
> >>> directly
> >>> into the kernel.
> >>> - Add stuff to meson files instead of Makefiles.
> >>> - Try to disable the slowest drivers and services to speed up the flash
> >>> boot
> >>> acceptance test a bit. This is somewhat based on the following
> >>> systemd-analyze blame report:
> >>> https://gist.github.com/hskinnemoen/475cb0676530cd2cebaa1754cf16ca97
> >>>
> >>> Changes since v6:
> >>>
> >>> - Use size_to_str to report DRAM sizes in npcm7xx_gcr.
> >>> - Simplify the interrupt logic in npcm7xx_timer.
> >>> - Update global bios_name instead of temporary.
> >>> - Add npcm7xx_bootrom to MAINTAINERS and pc-bios/README.
> >>> - Use a predefined name for the gsj boot image in the acceptance test.
> >>>
> >>> Changes since v5:
> >>>
> >>> - Boot ROM included, as a git submodule and a binary blob, and loaded by
> >>> default, so the -bios option is usually not necessary anymore.
> >>> - Two acceptance tests added (openbmc image boot, and direct kernel
> >>> boot).
> >>> - npcm7xx_load_kernel() moved to SoC code.
> >>> - NPCM7XX_TIMER_REF_HZ definition moved to CLK header.
> >>> - Comments added clarifying available SPI flash chip selects.
> >>> - Error handling adjustments:
> >>> - Errors from CPU and GCR realization are propagated through the SoC
> >>> since they may be triggered by user-configurable parameters.
> >>> - Machine init uses error_fatal instead of error_abort for SoC
> >>> realization flash init. This makes error messages more helpful.
> >>> - Comments added to indicate whether peripherals may fail to
> >>> realize.
> >>> - Use ERRP_GUARD() instead of Error *err when possible.
> >>> - Default CPU type is now set, and attempting to set it to anything else
> >>> will fail.
> >>> - Format string fixes (use HWADDR_PRIx, etc.)
> >>> - Simplified memory size encoding and error checking in npcm7xx_gcr.
> >>> - Encapsulate non-obvious pointer subtraction into helper functions in
> >>> the
> >>> FIU and TIMER modules.
> >>> - Incorporate review feedback into the FIU module:
> >>> - Add select/deselect trace events.
> >>> - Use npcm7xx_fiu_{de,}select() consistently.
> >>> - Use extract/deposit in more places for consistency.
> >>> - Use -Wimplicit-fallthrough compatible fallthrough comments.
> >>> - Use qdev_init_gpio_out_named instead of sysbus_init_irq for chip
> >>> selects.
> >>> - Incorporate review feedback into the TIMER module:
> >>> - Assert that we never pause a timer that has already expired,
> >>> instead of
> >>> trying to handle it. This should be safe since QEMU_CLOCK_VIRTUAL
> >>> is
> >>> stopped while this code is running.
> >>> - Simplify the switch blocks in the read and write handlers.
> >>>
> >>> I made a change to error out if a flash drive was not specified, but
> >>> reverted
> >>> it because it caused make check to fail (qom-test). When specifying a NULL
> >>> block device, the m25p flash device initializes its in-memory storage
> >>> with 0xff
> >>> and doesn't attempt to write anything back. This seems correct to me.
> >>>
> >>> Changes since v4:
> >>>
> >>> - OTP cleanups suggested by Philippe Mathieu-Daudé.
> >>> - Added fuse array definitions based on public Nuvoton bootblock
> >>> code.
> >>> - Moved class structure to .c file since it's only used internally.
> >>> - Readability improvements.
> >>> - Split the first patch and folded parts of it into three other patches
> >>> so
> >>> that CONFIG_NPCM7XX is only enabled after the initial NPCM7xx machine
> >>> support is added.
> >>> - DRAM init moved to machine init code.
> >>> - Consistently use lower-case hex literals.
> >>> - Switched to fine-grained unimplemented devices, based on public
> >>> bootblock
> >>> source code. Added a tiny SRAM that got left out previously.
> >>> - Simplified error handling in npcm7xx_realize() since the board code
> >>> will
> >>> abort anyway, and SoCs are not hot-pluggable.
> >>>
> >>> Changes since v3:
> >>>
> >>> - License headers are now GPL v2-or-later throughout.
> >>> - Added vmstate throughout (except the memory controller, which doesn't
> >>> really have any state worth saving). Successfully booted a gsj image
> >>> with two stop/savevm/quit/loadvm cycles along the way.
> >>> - JFFS2 really doesn't like it if I let qemu keep running after
> >>> savevm,
> >>> and then jump back in time with loadvm. I assume this is expected.
> >>> - Fixed an error API violation in npcm7xx_realize, removed pointless
> >>> error
> >>> check after object_property_set_link().
> >>> - Switched the OTP device to use an embedded array instead of a
> >>> g_malloc0'd
> >>> one because I couldn't figure out how to set up vmstate for the
> >>> latter.
> >>>
> >>> Changes since v2:
> >>>
> >>> - Simplified the MAINTAINERS entry.
> >>> - Added link to OpenPOWER jenkins for gsj BMC images.
> >>> - Reverted the smpboot change, back to byte swapping.
> >>> - Adapted to upstream API changes:
> >>> - sysbus_init_child_obj -> object_initialize_child
> >>> - object_property_set_bool -> qdev_realize / sysbus_realize
> >>> - ssi_create_slave_no_init -> qdev_new
> >>> - qdev_init_nofail -> qdev_realize_and_unref
> >>> - ssi_auto_connect_slaves removed
> >>> - Moved Boot ROM loading from soc to machine init.
> >>> - Plumbed power-on-straps property from GCR to the machine init code so
> >>> it
> >>> can be properly initialized. Turns out npcm750 memory init doesn't
> >>> work
> >>> without this. npcm730 is fine either way, though I'm not sure why.
> >>> - Reworked the flash init code so it looks more like aspeed (i.e. the
> >>> flash
> >>> device gets added even if there's no drive).
> >>>
> >>> Changes since v1 (requested by reviewers):
> >>>
> >>> - Clarify the source of CLK reset values.
> >>> - Made smpboot a constant byte array, eliinated byte swapping.
> >>> - NPCM7xxState now stores an array of ARMCPUs, not pointers to ARMCPUs.
> >>> - Clarify why EL3 is disabled.
> >>> - Introduce NPCM7XX_NUM_IRQ constant.
> >>> - Set the number of CPUs according to SoC variant, and disallow command
> >>> line
> >>> overrides (i.e. you can no longer override the number of CPUs with
> >>> the -smp
> >>> parameter). This is trying to follow the spirit of
> >>> https://patchwork.kernel.org/patch/11595407/.
> >>> - Switch register operations to DEVICE_LITTLE_ENDIAN throughout.
> >>> - Machine documentation added (new patch).
> >>>
> >>> Changes since v1 to support flash booting:
> >>>
> >>> - GCR reset value changes to get past memory initialization when booting
> >>> from flash (patches 2 and 5):
> >>> - INTCR2 now indicates that the DDR controller is initialized.
> >>> - INTCR3 is initialized according to DDR memory size. A realize()
> >>> method was implemented to achieve this.
> >>> - Refactor the machine initialization a bit to make it easier to drop in
> >>> machine-specific flash initialization (patch 6).
> >>> - Extend the series with additional patches to enable booting from
> >>> flash:
> >>> - Boot ROM (through the -bios option).
> >>> - OTP (fuse) controller.
> >>> - Memory Controller stub (just enough to skip memory training).
> >>> - Flash controller.
> >>> - Board-specific flash initialization.
> >>>
> >>> Thanks for reviewing,
> >>>
> >>> Havard
> >>>
> >>> Havard Skinnemoen (14):
> >>> hw/misc: Add NPCM7xx System Global Control Registers device model
> >>> hw/misc: Add NPCM7xx Clock Controller device model
> >>> hw/timer: Add NPCM7xx Timer device model
> >>> hw/arm: Add NPCM730 and NPCM750 SoC models
> >>> hw/arm: Add two NPCM7xx-based machines
> >>> roms: Add virtual Boot ROM for NPCM7xx SoCs
> >>> hw/arm: Load -bios image as a boot ROM for npcm7xx
> >>> hw/nvram: NPCM7xx OTP device model
> >>> hw/mem: Stubbed out NPCM7xx Memory Controller model
> >>> hw/ssi: NPCM7xx Flash Interface Unit device model
> >>> hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj
> >>> hw/arm/npcm7xx: add board setup stub for CPU and UART clocks
> >>> docs/system: Add Nuvoton machine documentation
> >>> tests/acceptance: console boot tests for quanta-gsj
> >>>
> >>> docs/system/arm/nuvoton.rst | 90 ++++
> >>> docs/system/target-arm.rst | 1 +
> >>> Makefile | 1 +
> >>> default-configs/arm-softmmu.mak | 1 +
> >>> include/hw/arm/npcm7xx.h | 112 +++++
> >>> include/hw/mem/npcm7xx_mc.h | 36 ++
> >>> include/hw/misc/npcm7xx_clk.h | 48 +++
> >>> include/hw/misc/npcm7xx_gcr.h | 43 ++
> >>> include/hw/nvram/npcm7xx_otp.h | 79 ++++
> >>> include/hw/ssi/npcm7xx_fiu.h | 73 ++++
> >>> include/hw/timer/npcm7xx_timer.h | 78 ++++
> >>> hw/arm/npcm7xx.c | 532 +++++++++++++++++++++++
> >>> hw/arm/npcm7xx_boards.c | 197 +++++++++
> >>> hw/mem/npcm7xx_mc.c | 84 ++++
> >>> hw/misc/npcm7xx_clk.c | 266 ++++++++++++
> >>> hw/misc/npcm7xx_gcr.c | 269 ++++++++++++
> >>> hw/nvram/npcm7xx_otp.c | 439 +++++++++++++++++++
> >>> hw/ssi/npcm7xx_fiu.c | 572 +++++++++++++++++++++++++
> >>> hw/timer/npcm7xx_timer.c | 509 ++++++++++++++++++++++
> >>> .gitmodules | 3 +
> >>> MAINTAINERS | 10 +
> >>> hw/arm/Kconfig | 9 +
> >>> hw/arm/meson.build | 1 +
> >>> hw/mem/meson.build | 1 +
> >>> hw/misc/meson.build | 4 +
> >>> hw/misc/trace-events | 8 +
> >>> hw/nvram/meson.build | 1 +
> >>> hw/ssi/meson.build | 1 +
> >>> hw/ssi/trace-events | 11 +
> >>> hw/timer/meson.build | 1 +
> >>> hw/timer/trace-events | 5 +
> >>> pc-bios/README | 6 +
> >>> pc-bios/npcm7xx_bootrom.bin | Bin 0 -> 768 bytes
> >>> roms/Makefile | 7 +
> >>> roms/vbootrom | 1 +
> >>> tests/acceptance/boot_linux_console.py | 83 ++++
> >>> 36 files changed, 3582 insertions(+)
> >>> create mode 100644 docs/system/arm/nuvoton.rst
> >>> create mode 100644 include/hw/arm/npcm7xx.h
> >>> create mode 100644 include/hw/mem/npcm7xx_mc.h
> >>> create mode 100644 include/hw/misc/npcm7xx_clk.h
> >>> create mode 100644 include/hw/misc/npcm7xx_gcr.h
> >>> create mode 100644 include/hw/nvram/npcm7xx_otp.h
> >>> create mode 100644 include/hw/ssi/npcm7xx_fiu.h
> >>> create mode 100644 include/hw/timer/npcm7xx_timer.h
> >>> create mode 100644 hw/arm/npcm7xx.c
> >>> create mode 100644 hw/arm/npcm7xx_boards.c
> >>> create mode 100644 hw/mem/npcm7xx_mc.c
> >>> create mode 100644 hw/misc/npcm7xx_clk.c
> >>> create mode 100644 hw/misc/npcm7xx_gcr.c
> >>> create mode 100644 hw/nvram/npcm7xx_otp.c
> >>> create mode 100644 hw/ssi/npcm7xx_fiu.c
> >>> create mode 100644 hw/timer/npcm7xx_timer.c
> >>> create mode 100644 pc-bios/npcm7xx_bootrom.bin
> >>> create mode 160000 roms/vbootrom
> >>>
> >>> --
> >>> 2.28.0.297.g1956fa8f8d-goog
> >>>
> >>>
> >>
> >