Re: [PATCH v7 13/13] tests/acceptance: console boot tests for quanta-gsj

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 8/20/20 7:29 AM, Philippe Mathieu-Daudé wrote:
> +Eric / Richard for compiler optimizations.
> 
> On 8/20/20 3:53 AM, Havard Skinnemoen wrote:
>> On Tue, Aug 11, 2020 at 8:26 PM Havard Skinnemoen
>> <hskinnemoen@google.com> wrote:
>>>
>>> On Tue, Aug 11, 2020 at 1:48 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> INTERRUPTED: Test interrupted by SIGTERM
>>>> Runner error occurred: Timeout reached
>>>> (240.45 s)
>>>>
>>>> Is that expected?
>>>
>>> I'm not sure why it only happens when running direct kernel boot with
>>> unoptimized qemu, but it seems a little happier if I enable a few more
>>> peripherals that I have queued up (sd, ehci, ohci and rng), though not
>>> enough.
>>>
>>> It still stalls for an awfully long time on "console: Run /init as
>>> init process" though. I'm not sure what it's doing there. With -O2 it
>>> only takes a couple of seconds to move on.
>>
>> So it turns out that the kernel gets _really_ sluggish when skipping
>> the clock initialization normally done by the boot loader.

Hmm IIRC other boards are affected (raspi and orange-pi).

Maybe it is time to define some static inlined boolean function
in "qemu/compiler.h", maybe qemu_build_optimized()? Or not inlined
function but simply expand to true/false:

 /**
  * qemu_build_not_reached()
  *
  * The compiler, during optimization, is expected to prove that a call
  * to this function cannot be reached and remove it.  If the compiler
  * supports QEMU_ERROR, this will be reported at compile time; o therwise
  * this will be reported at link time due to the missing symbol.
  */
 #if defined(__OPTIMIZE__) && !defined(__NO_INLINE__)
 extern void QEMU_NORETURN QEMU_ERROR("code path is reachable")
     qemu_build_not_reached(void);
 #else
 #define qemu_build_not_reached()  g_assert_not_reached()
 #endif
+
+#if defined(__OPTIMIZE__)
+#define qemu_build_optimized() true
+#else
+#define qemu_build_optimized() false
+#endif

>>
>> I changed the reset value of CLKSEL like this:
>>
>> diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
>> index 21ab4200d1..5e9849410f 100644
>> --- a/hw/misc/npcm7xx_clk.c
>> +++ b/hw/misc/npcm7xx_clk.c
>> @@ -67,7 +67,7 @@ enum NPCM7xxCLKRegisters {
>>   */
>>  static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
>>      [NPCM7XX_CLK_CLKEN1]        = 0xffffffff,
>> -    [NPCM7XX_CLK_CLKSEL]        = 0x004aaaaa,
>> +    [NPCM7XX_CLK_CLKSEL]        = 0x004aaba9,
>>      [NPCM7XX_CLK_CLKDIV1]       = 0x5413f855,
>>      [NPCM7XX_CLK_PLLCON0]       = 0x00222101 | PLLCON_LOKI,
>>      [NPCM7XX_CLK_PLLCON1]       = 0x00202101 | PLLCON_LOKI,
>>
>> which switches the CPU core and UART to run from PLL2 instead of
>> CLKREF (25 MHz).
>>
>> With this change, the test passes without optimization:
>>
>>  (02/19) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd:
>> PASS (39.62 s)
>>
>> It doesn't look like this change hurts booting from the bootrom (IIUC
>> the nuvoton bootblock overwrites CLKSEL anyway), but it's not super
>> clean.
>>
>> Perhaps I should make it conditional on kernel_filename being set? Or
>> would it be better to provide a write_board_setup hook for this?
> 
> QEMU prefers to avoid ifdef'ry at all cost. However I find this
> approach acceptable (anyway up to the maintainer):
> 
> +static void npcm7xx_clk_cold_reset_fixup(NPCM7xxCLKState *s)
> +{
> +#ifndef __OPTIMIZE__
> +    /*
> +     * When built without optimization, ...
> +     * so run CPU core and UART from PLL2 instead of CLKREF.
> +     */
> +    s->regs[NPCM7XX_CLK_CLKSEL] |= 0x103,
> +#endif
> +}
> 
>  static void npcm7xx_clk_enter_reset(Object *obj, ResetType type)
>  {
>      NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> 
>      QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> 
>      switch (type) {
>      case RESET_TYPE_COLD:
>          memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
> +        npcm7xx_clk_cold_reset_fixup(s);
>          s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>          return;
>      }
>      ...
> 
> Regards,
> 
> Phil.
>