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

[Philippe Mathieu-Daudé]

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

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