Re: [Qemu-devel] [PATCH] Hardware watchdog patch, version 5

[Anthony Liguori]

Richard W.M. Jones wrote:
> I think that it's unlikely anyone would want to hotplug a virtual
> watchdog device, so that would just be adding complicated code that no
> one would ever use.
>
> Rich.
>   

I think we're getting pretty close.  I did a thorough review and I don't 
see any major issues except for the lack of a Signed-off-by :-)  So if 
you make the stylistic suggestions below, I think it'll be ready for 
inclusion.

BTW, if anyone wants to write a QEMU CodingStyle, it would be 
tremendously useful.  I'll try to get to it at some point but it may 
take me a while.

> Index: Makefile.target
> ===================================================================
> --- Makefile.target     (revision 6855)
> +++ Makefile.target     (working copy)
> @@ -582,6 +582,9 @@
>  # Serial mouse
>  OBJS += msmouse.o
>  
> +# Generic watchdog support
> +OBJS += watchdog.o
> +
>  ifeq ($(TARGET_BASE_ARCH), i386)
>  # Hardware support
>  OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
> @@ -589,7 +592,8 @@
>  OBJS+= cirrus_vga.o apic.o ioapic.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
> -CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
> +OBJS+= wdt_ib700.o wdt_i6300esb.o
> +CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE -DHAS_WATCHDOGS
>   

I don't think this is the right way to address this problem as it isn't 
likely to scale as we add additional devices.  I'd suggest either 
registering the watch dogs in the pc init function or better yet, as 
part of PCI/ISA initialization.  Hopefully this will all get a lot 
cleaner as Markus' device config patches get included but for now, let's 
try to at least avoid using CFLAGS to solve this.

>  endif
>  ifeq ($(TARGET_BASE_ARCH), ppc)
>  CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
> Index: vl.c
> ===================================================================
> --- vl.c        (revision 6855)
> +++ vl.c        (working copy)
> +static LIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
> +
> +void
> +watchdog_add_model (WatchdogTimerModel *model)
> +{
> +    LIST_INSERT_HEAD (&watchdog_list, model, entry);
> +}
>   

void watchdog_add_model(WatchdogTimerModel *mode)
{
   LIST_INSERT_HEAD(&watchdog_list, model, entry);
}

Return type on the same line as function name, no space after identifier 
and first '('.  This is how the vast majority of QEMU code does it.

> +
> +static void
> +select_watchdog (const char *p)
> +{
> +    WatchdogTimerModel *model;
> +
> +    if (watchdog) {
> +        fprintf (stderr,
> +                 "qemu: only one watchdog option may be given\n");
> +        exit (1);
> +    }
> +
> +    /* -watchdog ? lists available devices and exits cleanly. */
> +    if (strcmp (p, "?") == 0) {
> +        LIST_FOREACH (model, &watchdog_list, entry) {
> +            fprintf (stderr, "\t%s\t%s\n",
> +                     model->wdt_name, model->wdt_description);
> +        }
> +        exit (0);
> +    }
>   

How about returning a value in this function and then having the caller 
exit?  At some point RSN we're going to have to go through QEMU and get 
rid of all the open-coded calls to exit().

> Index: qemu-doc.texi
> ===================================================================
> --- qemu-doc.texi       (revision 6855)
> +++ qemu-doc.texi       (working copy)
> @@ -1189,6 +1189,43 @@
>  @item -echr 20
>  @end table
>  
> address@hidden -watchdog @var{model}
> +Create a virtual hardware watchdog device.  Once enabled (by a guest
> +action), the watchdog must be periodically polled by an agent inside
> +the guest or else the guest will be restarted.
> +
> +The @var{model} is the model of hardware watchdog to emulate.  Choices
> +for model are: @code{ib700} (iBASE 700) which is a very simple ISA
> +watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O
> +controller hub) which is a much more featureful PCI-based dual-timer
> +watchdog.  Choose a model for which your guest has drivers.
> +
> address@hidden -watchdog-action @var{action}
> +
> +The @var{action} controls what QEMU will do when the watchdog timer
> +expires.
> +The default is
> address@hidden (forcefully reset the guest).
> +Other possible actions are:
> address@hidden (attempt to gracefully shutdown the guest),
> address@hidden (forcefully poweroff the guest),
> address@hidden (pause the guest),
> address@hidden (print a debug message and continue), or
> address@hidden (do nothing).
> +
> +Note that the @code{shutdown} action requires that the guest responds
> +to ACPI signals, which it may not be able to do in the sort of
> +situations where the watchdog would have expired, and thus
> address@hidden shutdown} is not recommended for production use.
> +
> +Use @code{-watchdog ?} to list available hardware models.  Only one
> +watchdog can be enabled for a guest.
> +
> address@hidden @code
> address@hidden -watchdog i6300esb -watchdog-action pause
> address@hidden -watchdog ib700
> address@hidden table
> +
>   

Nice documentation.  Particularly the ACPI gotcha bits.  We need more of 
that.

> Index: hw/watchdog.h
> ===================================================================
> --- hw/watchdog.h       (revision 0)
> +++ hw/watchdog.h       (revision 0)
> @@ -0,0 +1,65 @@
> +/*
> + * 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., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
> + * USA.
> + *
> + * By Richard W.M. Jones (address@hidden).
> + */
> +
> +#ifndef QEMU_WATCHDOG_H
> +#define QEMU_WATCHDOG_H
> +
> +extern void wdt_i6300esb_init (void);
> +extern void wdt_ib700_init (void);
> +
> +/* Possible values for action parameter. */
> +#define WDT_RESET        1     /* Hard reset. */
> +#define WDT_SHUTDOWN     2     /* Shutdown. */
> +#define WDT_POWEROFF     3     /* Quit. */
> +#define WDT_PAUSE        4     /* Pause. */
> +#define WDT_DEBUG        5     /* Prints a message and continues running. */
> +#define WDT_NONE         6     /* Do nothing. */
> +
> +struct WatchdogTimerModel {
> +    LIST_ENTRY(WatchdogTimerModel) entry;
> +
> +    /* Short name of the device - used to select it on the command line. */
> +    const char *wdt_name;
> +    /* Longer description (eg. manufacturer and full model number). */
> +    const char *wdt_description;
> +
> +    /* This callback should create/register the device.  It is called
> +     * indirectly from hw/pc.c when the virtual PC is being set up.
> +     */
> +    void (*wdt_pc_init) (PCIBus *pci_bus);
> +};
> +typedef struct WatchdogTimerModel WatchdogTimerModel;
> +
> +/* in vl.c */
> +extern int select_watchdog_action (const char *action);
> +extern void watchdog_add_model (WatchdogTimerModel *model);
> +extern void watchdog_perform_action (void);
>   
What not move these things to watchdog.c?  We should try to avoid adding 
stuff to vl.c whenever possible...

> +#ifdef I6300ESB_DEBUG
> +#define i6300esb_debug(fs,...) \
> +    fprintf(stderr,"i6300esb: %s: "fs,__func__,##__VA_ARGS__)
> +#else
> +#define i6300esb_debug(fs,...)
>   
#define i6300esb_debug(fs,...) do {} while (0)
> +
> +static void i6300esb_pc_init (PCIBus *pci_bus);
> +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 (I6300State *, int stage);
> +static void i6300esb_disable_timer (I6300State *);
> +static void i6300esb_timer_expired (void *vp);
> +static void i6300esb_reset (I6300State *d);
> +static void i6300esb_save (QEMUFile *f, void *vp);
> +static int i6300esb_load (QEMUFile *f, void *vp, int version);
>   

If you have to predefine a bunch of statics, it suggests you should 
probably rearrange your code.

Regards,

Anthony Liguori