From owner-freebsd-virtualization@FreeBSD.ORG Wed Oct 29 00:19:37 2014 Return-Path: Delivered-To: freebsd-virtualization@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A85E1BD2 for ; Wed, 29 Oct 2014 00:19:37 +0000 (UTC) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.redhat.com", Issuer "DigiCert SHA2 Extended Validation Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 80B42D98 for ; Wed, 29 Oct 2014 00:19:37 +0000 (UTC) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9T0JVaa015661 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 28 Oct 2014 20:19:31 -0400 Received: from [10.14.16.179] (dhcp-16-179.sjc.redhat.com [10.14.16.179]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9T0JUQw007067; Tue, 28 Oct 2014 20:19:30 -0400 Message-ID: <54503292.9040907@redhat.com> Date: Wed, 29 Oct 2014 01:19:30 +0100 From: Michal Privoznik User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Conrad Meyer , libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 1/2] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests. References: <1414094284-29055-1-git-send-email-cse.cem@gmail.com> <1414094284-29055-2-git-send-email-cse.cem@gmail.com> In-Reply-To: <1414094284-29055-2-git-send-email-cse.cem@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 Cc: Conrad Meyer , freebsd-virtualization@freebsd.org X-BeenThere: freebsd-virtualization@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "Discussion of various virtualization techniques FreeBSD supports." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Oct 2014 00:19:37 -0000 On 23.10.2014 21:58, Conrad Meyer wrote: > Also, flip Bhyve /domain/os/type support from HVM to Xen. Bhyve only > supports paravirtualized guests, and 'xen' is closest to that. We still > default to bhyveloader(1) if no explicit bootloader configuration is > supplied in the domain. > > If the /domain/bootloader looks like grub-bhyve and the user doesn't > supply /domain/bootloader_args, we make an intelligent guess and try > chainloading the first partition on the disk. > > Caveats: > - We can't install from CD without explicit bootloader_args. > - We leave a device.map file lying around in /tmp. I don't see a good > way not to do so without reworking the API somewhat. > > Sponsored by: EMC / Isilon storage division > > Signed-off-by: Conrad Meyer > --- > docs/drvbhyve.html.in | 30 +++++- > docs/formatdomain.html.in | 4 +- > po/libvirt.pot | 4 + > src/bhyve/bhyve_capabilities.c | 2 +- > src/bhyve/bhyve_command.c | 107 +++++++++++++++++++-- > tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.xml | 2 +- > tests/bhyvexml2argvdata/bhyvexml2argv-base.xml | 2 +- > tests/bhyvexml2argvdata/bhyvexml2argv-console.xml | 2 +- > .../bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.xml | 2 +- > .../bhyvexml2argv-disk-virtio.xml | 2 +- > tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.xml | 2 +- > tests/bhyvexml2argvdata/bhyvexml2argv-metadata.xml | 2 +- > tests/bhyvexml2argvdata/bhyvexml2argv-serial.xml | 2 +- > .../bhyvexml2xmlout-metadata.xml | 2 +- > 14 files changed, 139 insertions(+), 26 deletions(-) > > diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in > index 39afdf5..c6c79d7 100644 > --- a/docs/drvbhyve.html.in > +++ b/docs/drvbhyve.html.in > @@ -37,8 +37,7 @@ bhyve+ssh://root@example.com/system (remote access, SSH tunnelled) >

Example config

>

> The bhyve driver in libvirt is in its early stage and under active development. So it supports > -only limited number of features bhyve provides. All the supported features could be found > -in this sample domain XML. > +only limited number of features bhyve provides. >

> >

> @@ -50,13 +49,13 @@ up to 31 PCI devices. > > 

>   <domain type='bhyve'>
> -  <name>bhyve</name>
> -  <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
> +    <name>bhyve</name>
> +    <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid>
>       <memory>219136</memory>
>       <currentMemory>219136</currentMemory>
>       <vcpu>1</vcpu>
>       <os>
> -       <type>hvm</type>
> +       <type>xen</type>
>       </os>
>       <features>
>         <apic/>
> @@ -157,5 +156,26 @@ An example of domain XML device entry for that will look like:

>   
Please refer to the Storage documentation for more details on storage
>   management.

>
> +
Using grub2-bhyve or Alternative Bootloaders

> +
> +
It's possible to boot non-FreeBSD guests by specifying an explicit
> +bootloader, e.g. grub-bhyve(1). Arguments to the bootloader may be
> +specified as well. If no arguments are given and bootloader is
> +grub-bhyve, libvirt will try and boot from the first partition of
> +the disk image.

> +
> +
> +  ...
> +    <bootloader>/usr/local/sbin/grub-bhyve</bootloader>
> +    <bootloader_args>...</bootloader_args>
> +  ...
> +

> +
> +
(Of course, to install from a CD a user will have to supply explicit
> +arguments to grub-bhyve.)

> +
> +
Caveat: bootloader_args does not support any quoting.
> +Filenames, etc, must not have spaces or they will be tokenized incorrectly.

> +
>     
>   
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0099ce7..b7b6c46 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -217,7 +217,9 @@
>         a BIOS, and instead the host is responsible to kicking off the
>         operating system boot. This may use a pseudo-bootloader in the
>         host to provide an interface to choose a kernel for the guest.
> -      An example is pygrub with Xen.
> +      An example is pygrub with Xen. The Bhyve hypervisor
> +      also uses a host bootloader, either bhyveload or
> +      grub-bhyve.
>       

>
>   
> diff --git a/po/libvirt.pot b/po/libvirt.pot
> index 0b44ad7..d8c9a4d 100644
> --- a/po/libvirt.pot
> +++ b/po/libvirt.pot
> @@ -851,6 +851,10 @@ msgstr ""
>   msgid "domain should have at least one disk defined"
>   msgstr ""
>
> +#: src/bhyve/bhyve_command.c:407
> +msgid "Custom loader requires explicit %s configuration"
> +msgstr ""
> +
>   #: src/bhyve/bhyve_device.c:50
>   msgid "PCI bus 0 slot 1 is reserved for the implicit LPC PCI-ISA bridge"
>   msgstr ""

There's no need for this. The po/* files are regenerated on the release.

> diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
> index 132ce91..b37a24f 100644
> --- a/src/bhyve/bhyve_capabilities.c
> +++ b/src/bhyve/bhyve_capabilities.c
> @@ -85,7 +85,7 @@ virBhyveCapsBuild(void)
>                                      false, false)) == NULL)
>           return NULL;
>
> -    if ((guest = virCapabilitiesAddGuest(caps, "hvm",
> +    if ((guest = virCapabilitiesAddGuest(caps, "xen",
>                                            VIR_ARCH_X86_64,
>                                            "bhyve",
>                                            NULL, 0, NULL)) == NULL)
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index bea4a59..99956ae 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -300,6 +300,7 @@ virBhyveProcessBuildLoadCmd(virConnectPtr conn,
>   {
>       virCommandPtr cmd;
>       virDomainDiskDefPtr disk;
> +    bool bhyveload, grub_bhyve;
>
>       if (def->ndisks < 1) {
>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -326,19 +327,105 @@ virBhyveProcessBuildLoadCmd(virConnectPtr conn,
>           return NULL;
>       }
>
> -    cmd = virCommandNew(BHYVELOAD);
> +    if (def->os.bootloader == NULL) {
> +        bhyveload = true;
> +        grub_bhyve = false;
> +        cmd = virCommandNew(BHYVELOAD);
> +    } else {
> +        bhyveload = false;
> +        if (strstr(def->os.bootloader, "grub-bhyve") == 0)
> +            grub_bhyve = true;
> +        cmd = virCommandNew(def->os.bootloader);
> +    }
>
> -    /* Memory */
> -    virCommandAddArg(cmd, "-m");
> -    virCommandAddArgFormat(cmd, "%llu",
> -                           VIR_DIV_UP(def->mem.max_balloon, 1024));
> +    if (bhyveload && def->os.bootloaderArgs == NULL) {
> +        VIR_DEBUG("%s: bhyveload with default arguments", __func__);

VIR_DEBUG() itself will report what function has it been called from.

> +
> +        /* Memory (MB) */
> +        virCommandAddArg(cmd, "-m");
> +        virCommandAddArgFormat(cmd, "%llu",
> +                               VIR_DIV_UP(def->mem.max_balloon, 1024));
> +
> +        /* Image path */
> +        virCommandAddArg(cmd, "-d");
> +        virCommandAddArg(cmd, virDomainDiskGetSource(disk));
> +
> +        /* VM name */
> +        virCommandAddArg(cmd, def->name);
> +    } else if (grub_bhyve && def->os.bootloaderArgs == NULL) {
> +        char tmpmapfile[128] = "/tmp/grub-bhyve-device.map_XXXXXX";

This doesn't look nice. I'd prefer char *tmpmapfile combined with 
virAsprintf() and VIR_FREE().

> +        FILE *f;
> +        int fd;
> +
> +        VIR_DEBUG("%s: grub-bhyve with default arguments", __func__);
> +
> +        /*
> +         * XXX Default grub-bhyve has some BIG caveats, but MAY work for some
> +         * typical configurations. In particular:
> +         *
> +         *   - Can't create a new VM this way (no CD, no boot from CD)
> +         *   - Assumes a GRUB install on hd0,msdos1
> +         */
>
> -    /* Image path */
> -    virCommandAddArg(cmd, "-d");
> -    virCommandAddArg(cmd, virDomainDiskGetSource(disk));
> +        /* XXX cleanup this file. */
> +        fd = mkstemp(tmpmapfile);

No, we use mkostemp( .. , O_CLOEXEC) instead.

> +        if (fd < 0) {
> +                virReportError(VIR_ERR_OPEN_FAILED, tmpmapfile);
> +                goto error;
> +        }
> +
> +        f = VIR_FDOPEN(fd, "wb+");
> +        if (f == NULL) {
> +                VIR_FORCE_CLOSE(fd);
> +                virReportError(VIR_ERR_OPEN_FAILED, tmpmapfile);
> +                goto error;
> +        }
> +
> +        /* Grub device.map */
> +        fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk));
> +        /* XXX CDs would look like: "(cd0) /path/to/CD" */
> +
> +        if (VIR_FCLOSE(f) < 0) {
> +                virReportSystemError(errno, "%s", _("failed to close file"));
> +                goto error;
> +        }
>
> -    /* VM name */
> -    virCommandAddArg(cmd, def->name);
> +
> +        virCommandAddArg(cmd, "--device-map");
> +        virCommandAddArg(cmd, tmpmapfile);
> +
> +        /* Memory in MB */
> +        virCommandAddArg(cmd, "--memory");
> +        virCommandAddArgFormat(cmd, "%llu",
> +                               VIR_DIV_UP(def->mem.max_balloon, 1024));
> +
> +        /* To boot from CD, "cd0" here. */
> +        virCommandAddArg(cmd, "--root");
> +        virCommandAddArg(cmd, "hd0,msdos1");
> +
> +        /* VM name */
> +        virCommandAddArg(cmd, def->name);
> +    } else if (def->os.bootloaderArgs == NULL) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Custom loader requires explicit %s configuration"),
> +                       "bootloader_args");
> +        goto error;
> +    } else {
> +        char **blargs, **arg;
> +
> +        VIR_DEBUG("%s: custom loader '%s' with arguments", __func__,
> +                  def->os.bootloader);
> +
> +        /* XXX: Handle quoted? */
> +        blargs = virStringSplit(def->os.bootloaderArgs, " ", 0);
> +        for (arg = blargs; *arg; arg++)
> +                virCommandAddArg(cmd, *arg);
> +        virStringFreeList(blargs);

I don't think it's safe to pass arbitrary arguments from XML. I find 
this too critical to ACK the patch, buy maybe further discussion can 
change my mind.

> +    }
>
>       return cmd;
> +
> +error:
> +    virCommandFree(cmd);
> +    return NULL;

Michal