Re: [Qemu-devel] [PATCH] Hardware watchdogs (minor update, version 4)

[Anthony Liguori <aliguori@us.ibm.com>]

Richard W.M. Jones wrote:
> This is the hardware virtual watchdogs patch.  The only change since
> the last time I posted it[1] is that I've rebased to latest SVN and
> I've changed the header files to use non-reserved QEMU_* symbols
> instead of _QEMU_* (thanks Marc for pointing this out).
>
> Rich.
>
> [1] http://lists.gnu.org/archive/html/qemu-devel/2009-03/msg00050.html
>     http://lists.gnu.org/archive/html/qemu-devel/2009-02/msg01434.html
>     http://lists.gnu.org/archive/html/qemu-devel/2009-02/msg01404.html
>
>   
>
> Index: Makefile.target
> ===================================================================
> --- Makefile.target	(revision 6718)
> +++ Makefile.target	(working copy)
> @@ -584,6 +584,7 @@
>  OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
>  OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
>  OBJS += device-hotplug.o pci-hotplug.o
> +OBJS+= wdt_ib700.o wdt_i6300esb.o
>  CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
>  endif
>  ifeq ($(TARGET_BASE_ARCH), ppc)
> Index: vl.c
> ===================================================================
> --- vl.c	(revision 6718)
> +++ vl.c	(working copy)
> @@ -43,6 +43,7 @@
>  #include "migration.h"
>  #include "kvm.h"
>  #include "balloon.h"
> +#include "watchdog.h"
>  
>  #include <unistd.h>
>  #include <fcntl.h>
> @@ -4103,6 +4104,8 @@
>             "-old-param      old param mode\n"
>  #endif
>             "-tb-size n      set TB size\n"
> +           "-watchdog ib700|i6300esb[,action=reset|shutdown|poweroff|pause]\n"
> +           "                enable virtual hardware watchdog [default=none]\n"
>   

It would be good to have two separate options.  One that enabled the 
watch dog device and then another that controlled what the behavior of 
it is.  The goal is to separate guest configuration information from how 
the emulated device is configured in the host.  If it's not already 
there, the host configuration should be changable via the monitor too.

>  ifdef CONFIG_BRLAPI
>  OBJS+= baum.o
> Index: watchdog.c
> ===================================================================
> --- watchdog.c	(revision 0)
> +++ watchdog.c	(revision 0)
> @@ -0,0 +1,161 @@
> +/*
> + * Virtual hardware watchdog.
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + * By Richard W.M. Jones (rjones@redhat.com).
> + */
> +
> +#include "qemu-common.h"
> +#include "sysemu.h"
> +#include "hw/wdt_ib700.h"
> +#include "hw/wdt_i6300esb.h"
> +#include "watchdog.h"
> +
> +/* Possible values for action parameter. */
> +#define WDT_RESET        1
> +#define WDT_SHUTDOWN     2
> +#define WDT_POWEROFF     3
> +#define WDT_PAUSE        4
> +
> +static void parse_option (const char *arg, int *action_r);
> +static void parse_rest (const char *rest, int *action_r);
> +
> +/* Linked list of models - virtual watchdog devices add themselves
> + * to this list.
> + */
> +wdt_model *wdt_models = NULL;
>   

This isn't QEMU style.  WatchdogTimerModel would be.  Also, you can use 
the macros in sys-queue.h for lists.

> +/* The raw -watchdog option specified on the command line. */
> +const char *wdt_option = NULL;
>   

I'd rather that command line arguments be parsed in vl.c to cleanly 
separate things.  If you do my above suggestion about splitting up 
options though, this problem goes away.

> +static wdt_model *wdt = NULL;
> +
> +/* Called from the PC code to parse the option and finally configure
> + * the device.
> + */
> +void
> +watchdog_pc_init (PCIBus *pci_bus)
> +{
> +    int action;
> +
> +    if (!wdt_option) return;
> +    parse_option (wdt_option, &action);
> +    if (!wdt) return;           /* No watchdog configured. */
> +    wdt->wdt_methods->wdt_pc_init (pci_bus, action);
> +}
>   

If this is a PCI device, it should be hot pluggable, right?

The normal thing is for pc.c to directly call the device 
initialization.  Definitely don't want to wait until then to parse the 
options.

> +/* Device state. */
> +struct state {
> +    PCIDevice dev;              /* PCI device state, must be first field. */
> +
> +    int action;                 /* Action on expiry. */
> +
> +    int reboot_enabled;         /* "Reboot" on timer expiry.  The real action
> +                                 * performed depends on the action=* param
> +                                 * passed on QEMU command line.
> +                                 */
> +    int clock_scale;            /* Clock scale. */
> +#define CLOCK_SCALE_1KHZ 0
> +#define CLOCK_SCALE_1MHZ 1
> +
> +    int int_type;               /* Interrupt type generated. */
> +#define INT_TYPE_IRQ 0          /* APIC 1, INT 10 */
> +#define INT_TYPE_SMI 2
> +#define INT_TYPE_DISABLED 3
> +
> +    int free_run;               /* If true, reload timer on expiry. */
> +    int locked;                 /* If true, enabled field cannot be changed. */
> +    int enabled;                /* If true, watchdog is enabled. */
> +
> +    QEMUTimer *timer;           /* The actual watchdog timer. */
> +
> +    uint32_t timer1_preload;    /* Values preloaded into timer1, timer2. */
> +    uint32_t timer2_preload;
> +    int stage;                  /* Stage (1 or 2). */
> +
> +    int unlock_state;           /* Guest writes 0x80, 0x86 to unlock the
> +                                 * registers, and we transition through
> +                                 * states 0 -> 1 -> 2 when this happens.
> +                                 */
> +
> +    int previous_reboot_flag;   /* If the watchdog caused the previous
> +                                 * reboot, this flag will be set.
> +                                 */
> +};
> +
> +typedef struct state state;
>   

Way too generic of a type name.

> +static void i6300esb_pc_init (PCIBus *pci_bus, int action);
> +static void i6300esb_map (PCIDevice *dev, int region_num, uint32_t addr, uint32_t size, int type);
> +static void i6300esb_config_write (PCIDevice *dev, uint32_t addr, uint32_t data, int len);
> +static uint32_t i6300esb_config_read (PCIDevice *dev, uint32_t addr, int len);
> +static uint32_t i6300esb_mem_readb (void *vp, target_phys_addr_t addr);
> +static uint32_t i6300esb_mem_readw (void *vp, target_phys_addr_t addr);
> +static uint32_t i6300esb_mem_readl (void *vp, target_phys_addr_t addr);
> +static void i6300esb_mem_writeb (void *vp, target_phys_addr_t addr, uint32_t val);
> +static void i6300esb_mem_writew (void *vp, target_phys_addr_t addr, uint32_t val);
> +static void i6300esb_mem_writel (void *vp, target_phys_addr_t addr, uint32_t val);
> +static void i6300esb_restart_timer (state *, int stage);
> +static void i6300esb_disable_timer (state *);
> +static void i6300esb_timer_expired (void *vp);
> +static void i6300esb_reset (state *d);
> +static void i6300esb_save (QEMUFile *f, void *vp);
> +static int i6300esb_load (QEMUFile *f, void *vp, int version);
>   

If you have to predefine all of these, you're probably doing something 
wrong...

> +static wdt_methods i6300esb_wdt = {
> +    .wdt_pc_init = i6300esb_pc_init,
> +};
> +
> +static wdt_model model = {
> +    .wdt_name = "i6300esb",
> +    .wdt_description = "Intel 6300ESB",
> +    .wdt_methods = &i6300esb_wdt,
> +};
> +
> +void
> +wdt_i6300esb_init (void)
> +{
> +    model.wdt_next = wdt_models;
> +    wdt_models = &model;
> +}
> +
> +/* Create and initialize a virtual Intel 6300ESB during PC creation. */
> +static void
> +i6300esb_pc_init (PCIBus *pci_bus, int action)
> +{
> +    state *d;
> +    uint8_t *pci_conf;
> +
> +    if (!pci_bus) {
> +        fprintf (stderr, "wdt_i6300esb: no PCI bus in this machine\n");
> +        return;
> +    }
> +
> +    d = (state *)
> +        pci_register_device (pci_bus, "i6300esb_wdt", sizeof (state),
> +                             -1,
> +                             i6300esb_config_read, i6300esb_config_write);
> +
> +    d->action = action;
>   

The device emulation shouldn't need to know the action.  It should 
notify something else via a callback.

> +
> +static void
> +i6300esb_config_write (PCIDevice *dev, uint32_t addr, uint32_t data, int len)
> +{
> +    state *d = (state *) dev;
> +    int old;
> +
> +#ifdef I6300ESB_DEBUG
> +    fprintf (stderr, "i6300esb_config_write: addr = %x, data = %x, len = %d\n",
> +             addr, data, len);
> +#endif
>   

A debug macro would be nicer.

> +static void
> +i6300esb_save (QEMUFile *f, void *vp)
> +{
> +    state *d = (state *) vp;
> +
> +    pci_device_save (&d->dev, f);
> +    qemu_put_be32 (f, d->action);
> +    qemu_put_be32 (f, d->reboot_enabled);
> +    qemu_put_be32 (f, d->clock_scale);
> +    qemu_put_be32 (f, d->int_type);
> +    qemu_put_be32 (f, d->free_run);
> +    qemu_put_be32 (f, d->locked);
> +    qemu_put_be32 (f, d->enabled);
> +    qemu_put_timer (f, d->timer);
> +    qemu_put_be32 (f, d->timer1_preload);
> +    qemu_put_be32 (f, d->timer2_preload);
> +    qemu_put_be32 (f, d->stage);
> +    qemu_put_be32 (f, d->unlock_state);
> +    qemu_put_be32 (f, d->previous_reboot_flag);
> +}
>   

host state such as action should not be in the save/restore format.

Regards,

Anthony Liguori