Re: [Qemu-devel] [PATCH] allow bootdevice change from the monitor

[Aurelien Jarno]

On Sun, Mar 30, 2008 at 09:32:31AM +0200, Gildas wrote:
> 2008/3/29, Aurelien Jarno <address@hidden>:
> > On Wed, Mar 26, 2008 at 02:12:25PM +0100, Gildas wrote:
> >  > This patch allows changing the boot device from within the monitor.
> >
> >
> > I have just updated BOCHS BIOS from upstream CVS. The new version allows
> >  to select the boot device via a menu after the POST. I am therefore not
> >  sure anymore that your patch is still useful. What do you think?
> 
> Hi Aurelien,
> 
> I see the monitor as some kind of iLO/management interface and I think
> the two approaches are somewhat orthogonal and have different use
> cases, so I believe they should be both present:
> - the boot menu after POST is user interactive
> - the command in the monitor may be interactive but can also be
> triggered by scripts (think automated installation such as FAI which
> boot on pxe then on the local drive) or a management layer (think
> libvirt).
> 
> Also I wanted the entry in the monitor menu to be hardware independant
> so even if there is only x86 support for now it might be possible to
> use a single approach to set the boot device for all archs (at least
> the ones making use of the bootdevice parameter, such as sparc and
> ppc).
> 

I see, make sense now. Please find my comments below.


> diff -Nur a/hw/pc.c b/hw/pc.c
> --- a/hw/pc.c   2008-03-18 07:53:05.000000000 +0100
> +++ b/hw/pc.c   2008-03-26 13:18:16.000000000 +0100
> @@ -180,6 +180,33 @@
>      return 0;
>  }
>
> +/* copy/pasted from cmos_init, should be made a general function
> + and used there as well */
> +int pc_set_bootdevice(const char *boot_device)
> +{
> +#define PC_MAX_BOOT_DEVICES 3
> +    RTCState *s = rtc_state;
> +    int nbds, bds[3] = { 0, };
> +    int i;
> +
> +    nbds = strlen(boot_device);
> +    if (nbds > PC_MAX_BOOT_DEVICES) {
> +        fprintf(stderr, "Too many boot devices for PC\n");
> +        return(1);

The message is printed to stderr, while the command has been issued from
the monitor. Not very consistent.

> +    }
> +    for (i = 0; i < nbds; i++) {
> +        bds[i] = boot_device2nibble(boot_device[i]);
> +        if (bds[i] == 0) {
> +            fprintf(stderr, "Invalid boot device for PC: '%c'\n",
> +                    boot_device[i]);

Same here.

> +            return(1);
> +        }
> +    }
> +    rtc_set_memory(s, 0x3d, (bds[1] << 4) | bds[0]);
> +    rtc_set_memory(s, 0x38, (bds[2] << 4));
> +    return(0);
> +}
> +
>  /* hd_table must contain 4 block drivers */
>  static void cmos_init(int ram_size, const char *boot_device,
> BlockDriverState **hd_table)
>  {
> @@ -717,6 +744,8 @@
>      BlockDriverState *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>      BlockDriverState *fd[MAX_FD];
>
> +    set_bootdevice_fct = pc_set_bootdevice;
> +
>      linux_boot = (kernel_filename != NULL);
>
>      /* init CPUs */
> diff -Nur a/monitor.c b/monitor.c
> --- a/monitor.c 2008-02-10 17:33:13.000000000 +0100
> +++ b/monitor.c 2008-03-26 13:17:56.000000000 +0100
> @@ -993,6 +993,21 @@
>                  suffix, addr, size * 2, val);
>  }
>
> +static void do_bootdevice_set(const char *bootdevice)
> +{
> +    int res;
> +
> +    if (set_bootdevice_fct != NULL)  {
> +        res = set_bootdevice_fct(bootdevice);
> +        if (res == 0)
> +            term_printf("boot device list now set to %s\n", bootdevice);
> +        else
> +            term_printf("setting boot device list failed with error %i\n", 
> res);
> +    } else {
> +        term_printf("no function defined to set boot device list for this 
> architecture\n");
> +    }
> +}
> +
>  static void do_system_reset(void)
>  {
>      qemu_system_reset_request();
> @@ -1328,6 +1343,8 @@
>         "capture index", "stop capture" },
>      { "memsave", "lis", do_memory_save,
>        "addr size file", "save to disk virtual memory dump starting at 'addr' 
> of size 'size'", },
> +    { "boot_set", "s", do_bootdevice_set,
> +      "bootdevice", "define new values for the boot device list" },
>      { NULL, NULL, },
>  };
>
> diff -Nur a/sysemu.h b/sysemu.h
> --- a/sysemu.h  2008-03-18 07:53:05.000000000 +0100
> +++ b/sysemu.h  2008-03-26 13:12:20.000000000 +0100
> @@ -9,6 +9,11 @@
>  extern int vm_running;
>  extern const char *qemu_name;
>
> +/* handler to set the boot_device for a specific type of QEMUMachine */
> +/* return 0 if success */
> +typedef int QEMUMachineSetBootFunc(const char *boot_device);
> +QEMUMachineSetBootFunc *set_bootdevice_fct;
> +

Please don't declare variables in a .h. Multiple copies of the varialbe
would exists if the header is included multiple times, causing errors at
link. 

Also, the other part of the code rely on it being defined to NULL by default.

>  typedef struct vm_change_state_entry VMChangeStateEntry;
>  typedef void VMChangeStateHandler(void *opaque, int running);
>  typedef void VMStopHandler(void *opaque, int reason);
>
 

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net