Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
On Tue, Sep 8, 2020 at 6:32 PM Havard Skinnemoen
wrote:
> On Tue, Sep 8, 2020 at 12:52 PM Havard Skinnemoen
> wrote:
> >
> > On Tue, Sep 8, 2020 at 9:58 AM Philippe Mathieu-Daudé
> 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(>qtimer)) {
> > > +return;
> > > +}
> > > timer_del(>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.
>
> Hmm, that didn't help, though it might make sense to keep it there anyway.
>
> What the test case does is:
>
> 1. Enable the timer (with zero expiration time) and reset it at the same
> time.
> 2. Disable the timer zero cycles after it was enabled.
>
> It also touches a bunch of other bits (including reserved bits), but
> they should be irrelevant.
>
> I think there are two issues here.
>
> When the Reset bit is set, the Enable bit should be forced to zero.
> This is easy to fix.
>
> If the timer is enabled with zero expiration time, and immediately
> disabled without advancing the virtual time, npcm7xx_timer_pause() is
> called while the timer is active, but t->expires_ns ==
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL). So t->remaining_ns becomes zero
> and triggers the assertion.
>
> If I revert a change that Philippe asked me to do earlier:
>
> timer_del(>qtimer);
> now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> t->remaining_ns = t->expires_ns - now;
> -g_assert(t->remaining_ns > 0);
> +if (t->remaining_ns <= 0) {
> +npcm7xx_timer_reached_zero(t);
> +}
> }
>
> it doesn't crash:
>
> $ cat << EOF | ./qemu-system-arm -machine npcm750-evb -m 128M -qtest
> stdio --trace npcm7xx_timer*
> write 0xf0009040 0x4 0xc4c4c4c4
> write 0xf0009040 0x4 0x4
> EOF
> [I 1599613445.620379] OPENED
> [R +0.180771] write 0xf0009040 0x4 0xc4c4c4c4
> 1361079@1599613445.801182:npcm7xx_timer_write /machine/soc/tim[1]
> offset: 0x0040 value 0xc4c4c4c4
> OK
> [S +0.180816] OK
> [R +0.180833] write 0xf0009040 0x4 0x4
> 1361079@1599613445.801220:npcm7xx_timer_write /machine/soc/tim[1]
> offset: 0x0040 value 0x
> 1361079@1599613445.801295:npcm7xx_timer_irq /machine/soc/tim[1] timer 4
> state 0
> OK
> [S +0.180927] OK
> [I +0.181319] CLOSED
> [I +4.003267] CLOSED
>
> Note that the npcm7xx_timer_irq trace event is a sign of the first
> bug, but fixing that might mask the second bug. If we write the same
> pattern, only without the Reset bit, this would be the correct
> behavior (and it still causes the v8 code to crash).
>
> I think this device deserves a qtest. I wonder if we'd trigger the
> assertion if we set a nonzero expiration time, but happen to clear the
> Enable bit on the exact cycle it's supposed to expire. That would be a
> more realistic scenario, as it wouldn't require multiple register
> writes in the same virtual clock cycle.
>
I wrote some qtests, and found several more bugs, but I wasn't able to
trigger this particular failure mode. I was able to reproduce both of the
bugs found by the fuzzer though.
I'll refresh the patch series tonight or tomorrow, and also send the qtest
to Nuvoton (and probably send it to the list within the next few weeks or
so).
> I probably won't add the qtest to the same series, as I'd like someone
> from Nuvoton to get a chance to review it first.
>
> Havard
>
> >
> > 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
> > > >>
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
On Tue, Sep 8, 2020 at 12:52 PM Havard Skinnemoen
wrote:
>
> On Tue, Sep 8, 2020 at 9:58 AM Philippe Mathieu-Daudé 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(>qtimer)) {
> > +return;
> > +}
> > timer_del(>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.
Hmm, that didn't help, though it might make sense to keep it there anyway.
What the test case does is:
1. Enable the timer (with zero expiration time) and reset it at the same time.
2. Disable the timer zero cycles after it was enabled.
It also touches a bunch of other bits (including reserved bits), but
they should be irrelevant.
I think there are two issues here.
When the Reset bit is set, the Enable bit should be forced to zero.
This is easy to fix.
If the timer is enabled with zero expiration time, and immediately
disabled without advancing the virtual time, npcm7xx_timer_pause() is
called while the timer is active, but t->expires_ns ==
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL). So t->remaining_ns becomes zero
and triggers the assertion.
If I revert a change that Philippe asked me to do earlier:
timer_del(>qtimer);
now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
t->remaining_ns = t->expires_ns - now;
-g_assert(t->remaining_ns > 0);
+if (t->remaining_ns <= 0) {
+npcm7xx_timer_reached_zero(t);
+}
}
it doesn't crash:
$ cat << EOF | ./qemu-system-arm -machine npcm750-evb -m 128M -qtest
stdio --trace npcm7xx_timer*
write 0xf0009040 0x4 0xc4c4c4c4
write 0xf0009040 0x4 0x4
EOF
[I 1599613445.620379] OPENED
[R +0.180771] write 0xf0009040 0x4 0xc4c4c4c4
1361079@1599613445.801182:npcm7xx_timer_write /machine/soc/tim[1]
offset: 0x0040 value 0xc4c4c4c4
OK
[S +0.180816] OK
[R +0.180833] write 0xf0009040 0x4 0x4
1361079@1599613445.801220:npcm7xx_timer_write /machine/soc/tim[1]
offset: 0x0040 value 0x
1361079@1599613445.801295:npcm7xx_timer_irq /machine/soc/tim[1] timer 4 state 0
OK
[S +0.180927] OK
[I +0.181319] CLOSED
[I +4.003267] CLOSED
Note that the npcm7xx_timer_irq trace event is a sign of the first
bug, but fixing that might mask the second bug. If we write the same
pattern, only without the Reset bit, this would be the correct
behavior (and it still causes the v8 code to crash).
I think this device deserves a qtest. I wonder if we'd trigger the
assertion if we set a nonzero expiration time, but happen to clear the
Enable bit on the exact cycle it's supposed to expire. That would be a
more realistic scenario, as it wouldn't require multiple register
writes in the same virtual clock cycle.
I probably won't add the qtest to the same series, as I'd like someone
from Nuvoton to get a chance to review it first.
Havard
>
> 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
> > >>
> > >> 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
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
On Tue, Sep 8, 2020 at 9:58 AM Philippe Mathieu-Daudé 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(>qtimer)) {
> +return;
> +}
> timer_del(>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
> >>
> >> 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
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
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(>qtimer)) {
+return;
+}
timer_del(>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
>>
>> 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.
>>> -
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
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 > > 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,
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
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
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
for the patches that add new virtual-device code (1-5, 7-12 ?)
-Alex
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
Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
On 8/25/20 2:16 AM, 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. Series: Tested-by: Philippe Mathieu-Daudé
[PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
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
