Date: Wed, 29 Oct 2014 01:19:30 +0100 From: Michal Privoznik <mprivozn@redhat.com> To: Conrad Meyer <cse.cem@gmail.com>, libvir-list@redhat.com Cc: Conrad Meyer <conrad.meyer@isilon.com>, freebsd-virtualization@freebsd.org Subject: Re: [libvirt] [PATCH 1/2] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests. Message-ID: <54503292.9040907@redhat.com> In-Reply-To: <1414094284-29055-2-git-send-email-cse.cem@gmail.com> References: <1414094284-29055-1-git-send-email-cse.cem@gmail.com> <1414094284-29055-2-git-send-email-cse.cem@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <conrad.meyer@isilon.com>
> ---
> 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)
> <h3>Example config</h3>
> <p>
> 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.
> </p>
>
> <p>
> @@ -50,13 +49,13 @@ up to 31 PCI devices.
>
> <pre>
> <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:</p>
> <p>Please refer to the <a href="storage.html">Storage documentation</a> for more details on storage
> management.</p>
>
> +<h3><a name="grubbhyve">Using grub2-bhyve or Alternative Bootloaders</a></h3>
> +
> +<p>It's possible to boot non-FreeBSD guests by specifying an explicit
> +bootloader, e.g. <code>grub-bhyve(1)</code>. Arguments to the bootloader may be
> +specified as well. If no arguments are given and bootloader is
> +<code>grub-bhyve</code>, libvirt will try and boot from the first partition of
> +the disk image.</p>
> +
> +<pre>
> + ...
> + <bootloader>/usr/local/sbin/grub-bhyve</bootloader>
> + <bootloader_args>...</bootloader_args>
> + ...
> +</pre>
> +
> +<p>(Of course, to install from a CD a user will have to supply explicit
> +arguments to <code>grub-bhyve</code>.)</p>
> +
> +<p>Caveat: <code>bootloader_args</code> does not support any quoting.
> +Filenames, etc, must not have spaces or they will be tokenized incorrectly.</p>
> +
> </body>
> </html>
> 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 <code>pygrub</code> with Xen.
> + An example is <code>pygrub</code> with Xen. The Bhyve hypervisor
> + also uses a host bootloader, either <code>bhyveload</code> or
> + <code>grub-bhyve</code>.
> </p>
>
> <pre>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54503292.9040907>