Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines

[Philippe Mathieu-Daudé]

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);

> 
> 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
>>
>>
>