Paolo Bonzini <pbonz...@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 <pbonz...@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 <arm...@redhat.com>
