Re: [PATCH v6 04/13] hw/arm: Add NPCM730 and NPCM750 SoC models

[Philippe Mathieu-Daudé]

On 7/17/20 6:59 PM, Havard Skinnemoen wrote:
> +Markus Armbruster
> 
> On Fri, Jul 17, 2020 at 5:20 AM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 7/17/20 8:02 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>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
>>> ---
>>>  include/hw/arm/npcm7xx.h |  85 ++++++++
>>>  hw/arm/npcm7xx.c         | 407 +++++++++++++++++++++++++++++++++++++++
>>>  hw/arm/Kconfig           |   5 +
>>>  hw/arm/Makefile.objs     |   1 +
>>>  4 files changed, 498 insertions(+)
>>>  create mode 100644 include/hw/arm/npcm7xx.h
>>>  create mode 100644 hw/arm/npcm7xx.c
...

>>> +static void npcm7xx_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    NPCM7xxState *s = NPCM7XX(dev);
>>> +    NPCM7xxClass *nc = NPCM7XX_GET_CLASS(s);
>>> +    int i;
>>> +
>>> +    if (memory_region_size(s->dram) > NPCM7XX_DRAM_SZ) {
>>> +        error_setg(errp, "%s: NPCM7xx cannot address more than %" PRIu64
>>> +                   " MiB of DRAM", __func__, NPCM7XX_DRAM_SZ / MiB);
>>> +        return;
>>> +    }
>>> +
>>> +    /* CPUs */
>>> +    for (i = 0; i < nc->num_cpus; i++) {
>>> +        object_property_set_int(OBJECT(&s->cpu[i]), "mp-affinity",
>>> +                                arm_cpu_mp_affinity(i, 
>>> NPCM7XX_MAX_NUM_CPUS),
>>> +                                &error_abort);
>>> +        object_property_set_int(OBJECT(&s->cpu[i]), "reset-cbar",
>>> +                                NPCM7XX_GIC_CPU_IF_ADDR, &error_abort);
>>> +        object_property_set_bool(OBJECT(&s->cpu[i]), "reset-hivecs", true,
>>> +                                 &error_abort);
>>> +
>>> +        /* Disable security extensions. */
>>> +        object_property_set_bool(OBJECT(&s->cpu[i]), "has_el3", false,
>>> +                                 &error_abort);
>>> +
>>> +        if (!qdev_realize(DEVICE(&s->cpu[i]), NULL, errp)) {
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    /* A9MPCORE peripherals. Can only fail if we pass bad parameters here. 
>>> */
>>> +    object_property_set_int(OBJECT(&s->a9mpcore), "num-cpu", nc->num_cpus,
>>> +                            &error_abort);
>>> +    object_property_set_int(OBJECT(&s->a9mpcore), "num-irq", 
>>> NPCM7XX_NUM_IRQ,
>>> +                            &error_abort);

[1]

>>> +    sysbus_realize(SYS_BUS_DEVICE(&s->a9mpcore), &error_abort);

[2]

>>
>> shouldn't we test the return value and use errp ? I don't know what
>> was agreed upon.

Per https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html:

>> 1. Internal code failing to set simple properties to predefined
>> values is a programming error, so error_abort is appropriate.
>
> That would be my advice.
>
>> 2. qdev_realize() may fail due to user input, so errors should be
>> propagated.
>
> In general, yes.  For a specific device, you may know it can't fail,
> and then &error_abort may be okay.

So it looks correct.

> 
> I'm not sure if I got it 100% right, but what I tried to do was to see
> which submodules could possibly fail due to user input, and propagate
> errors from those modules only.
> 
> For example, the GCR can fail if the user-provided memory size can't
> be encoded into registers, so that one clearly needs to be propagated.
> 
> Other modules don't take any parameters at all, so they can only fail
> due to programming errors, hence error_abort.
> 
> I wasn't able to find any way command line options could cause the
> a9mpcore module to fail, but that's one of the cases I'm very unsure
> about, so I'll be happy to propagate errors from that if you (or
> anyone else) think it's needed.
> 
> I'm also not sure about the CPUs, but ended up going the other way
> since there's a -cpu option, and it's plausible that the user could
> cause it to fail even though I couldn't find any specific options to
> trigger an error.