Re: [PATCH v5 04/11] hw/arm: Add NPCM730 and NPCM750 SoC models

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> +Markus
>
> On 7/14/20 2:44 AM, Havard Skinnemoen wrote:
>> On Mon, Jul 13, 2020 at 8:02 AM Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> On 7/9/20 2:36 AM, Havard Skinnemoen wrote:
>>>> The Nuvoton NPCM7xx SoC family are used to implement Baseboard
>>>> Management Controllers in servers. While the family includes four SoCs,
>>>> this patch implements limited support for two of them: NPCM730 (targeted
>>>> for Data Center applications) and NPCM750 (targeted for Enterprise
>>>> applications).
>>>>
>>>> This patch includes little more than the bare minimum needed to boot a
>>>> Linux kernel built with NPCM7xx support in direct-kernel mode:
>>>>
>>>>   - Two Cortex-A9 CPU cores with built-in periperhals.
>>>>   - Global Configuration Registers.
>>>>   - Clock Management.
>>>>   - 3 Timer Modules with 5 timers each.
>>>>   - 4 serial ports.
>>>>
>>>> The chips themselves have a lot more features, some of which will be
>>>> added to the model at a later stage.
>>>>
>>>> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
>>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
>>>> ---
> ...
>
>>>> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    NPCM7xxState *s = NPCM7XX(dev);
>>>> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
>>>> +    int i;
>>>> +
>>>> +    /* CPUs */
>>>> +    for (i = 0; i < nc->num_cpus; i++) {
>>>> +        object_property_set_int(OBJECT(&s->cpu[i]),
>>>> +                                arm_cpu_mp_affinity(i, 
>>>> NPCM7XX_MAX_NUM_CPUS),
>>>> +                                "mp-affinity", &error_abort);
>>>> +        object_property_set_int(OBJECT(&s->cpu[i]), 
>>>> NPCM7XX_GIC_CPU_IF_ADDR,
>>>> +                                "reset-cbar", &error_abort);
>>>> +        object_property_set_bool(OBJECT(&s->cpu[i]), true,
>>>> +                                 "reset-hivecs", &error_abort);
>>>> +
>>>> +        /* Disable security extensions. */
>>>> +        object_property_set_bool(OBJECT(&s->cpu[i]), false, "has_el3",
>>>> +                                 &error_abort);
>>>> +
>>>> +        qdev_realize(DEVICE(&s->cpu[i]), NULL, &error_abort);
>>>
>>> I would check the error:
>>>
>>>         if (!qdev_realize(DEVICE(&s->cpu[i]), NULL, errp)) {
>>>             return;
>>>         }
>>>
>>> same for the sysbus_realize() below.
>> 
>> Hmm, I used to propagate these errors until Philippe told me not to
>> (or at least that's how I understood it).
>
> It was before Markus simplification API were merged, you had to
> propagate after each call, since this is a non hot-pluggable SoC
> I suggested to use &error_abort to simplify.
>
>> I'll be happy to do it
>> either way (and the new API makes it really easy to propagate errors),
>> but I worry that I don't fully understand when to propagate errors and
>> when not to.
>
> Markus explained it on the mailing list recently (as I found the doc
> not obvious). I can't find the thread. I suppose once the work result
> after the "Questionable aspects of QEMU Error's design" discussion is
> merged, the documentation will be clarified.

The Error API evolved recently.  Please peruse the big comment in
include/qapi/error.h.  If still unsure, don't hesitate to ask here.

> My rule of thumb so far is:
> - programming error (can't happen) -> &error_abort

Correct.  Quote the big comment:

 * Call a function aborting on errors:
 *     foo(arg, &error_abort);
 * This is more concise and fails more nicely than
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     assert(!err); // don't do this

> - everything triggerable by user or management layer (via QMP command)
>   -> &error_fatal, as we can't risk loose the user data, we need to
>   shutdown gracefully.

Quote the big comment:

 * Call a function treating errors as fatal:
 *     foo(arg, &error_fatal);
 * This is more concise than
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) { // don't do this
 *         error_report_err(err);
 *         exit(1);
 *     }

Terminating the process is generally fine during initial startup,
i.e. before the guest runs.

It's generally not fine once the guest runs.  Errors need to be handled
more gracefully then.  A QMP command, for instance, should fail cleanly,
propagating the error to the monitor core, which then sends it to the
QMP client, and loops to process the next command.

>> It makes sense to me to propagate errors from *_realize() and
>> error_abort on failure to set simple properties, but I'd like to know
>> if Philippe is on board with that.

Realize methods must not use &error_fatal.  Instead, they should clean
up and fail.

"Clean up" is the part we often neglect.  The big advantage of
&error_fatal is that you don't have to bother :)

Questions?

[...]