Re: [PATCH 10/12] hpet: return errors from realize if properties are incorrect

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Do not silently adjust num_timers, and fail if intcap is 0.

A bad habit of ours.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/timer/hpet.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index d1b7bc52b7b..d78aba04bcd 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -689,8 +689,14 @@ static void hpet_realize(DeviceState *dev, Error **errp)
>      int i;
>      HPETTimer *timer;
>  
> +    if (s->num_timers < HPET_MIN_TIMERS || s->num_timers > HPET_MAX_TIMERS) {
> +        error_setg(errp, "hpet.num_timers must be between %d and %d",
> +                   HPET_MIN_TIMERS, HPET_MAX_TIMERS);
> +        return;
> +    }
>      if (!s->intcap) {
> -        warn_report("Hpet's intcap not initialized");
> +        error_setg(errp, "hpet.hpet-intcap not initialized");
> +        return;
>      }
>      if (hpet_fw_cfg.count == UINT8_MAX) {
>          /* first instance */
> @@ -698,7 +704,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (hpet_fw_cfg.count == 8) {
> -        error_setg(errp, "Only 8 instances of HPET is allowed");
> +        error_setg(errp, "Only 8 instances of HPET are allowed");
>          return;
>      }
>  
> @@ -708,11 +714,6 @@ static void hpet_realize(DeviceState *dev, Error **errp)
>          sysbus_init_irq(sbd, &s->irqs[i]);
>      }
>  
> -    if (s->num_timers < HPET_MIN_TIMERS) {
> -        s->num_timers = HPET_MIN_TIMERS;
> -    } else if (s->num_timers > HPET_MAX_TIMERS) {
> -        s->num_timers = HPET_MAX_TIMERS;
> -    }
>      for (i = 0; i < HPET_MAX_TIMERS; i++) {
>          timer = &s->timer[i];
>          timer->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, hpet_timer, timer);

This device is not user-creatable.  It is only ever created (and
realized) by board code.  Errors should not happen.  If they happen
anyway, it's a board code bug.

The code creating it is pc_basic_device_init():

    if (pcms->hpet_enabled) {
        qemu_irq rtc_irq;

        hpet = qdev_try_new(TYPE_HPET);
        if (!hpet) {
            error_report("couldn't create HPET device");
            exit(1);
        }

Could just as well use qdev_new().  Differently confusing error message,
though.

        /*
         * For pc-piix-*, hpet's intcap is always IRQ2. For pc-q35-*,
         * use IRQ16~23, IRQ8 and IRQ2.  If the user has already set
         * the property, use whatever mask they specified.
         */
        uint8_t compat = object_property_get_uint(OBJECT(hpet),
                HPET_INTCAP, NULL);
        if (!compat) {
            qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
        }
        sysbus_realize_and_unref(SYS_BUS_DEVICE(hpet), &error_fatal);

If this fails, it's a programming error, i.e. &error_abort is more
appropriate.  Hmm, can the user mess with property values via -global?
If yes, it could be a user error.

        [...]
    }

I'm rambling.  The patch is fine.

Reviewed-by: Markus Armbruster <armbru@redhat.com>