Re: [libvirt] [RFC PATCH v2 4/4] PowerPC : Bifurcate arch-specific qemu initialization code from generic qemu command line generation.
Hi Daniel,
Thanks for taking a look.
On 11/15/2011 04:33 PM, Daniel P. Berrange wrote:
On Mon, Nov 14, 2011 at 08:26:55PM +0530, Prerna Saxena wrote:
From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001
From: Prerna Saxena
Date: Mon, 14 Nov 2011 19:43:26 +0530
Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from
generic qemu commandline building code.
At present, qemuBuildCommandLine emits many x86-specific options while
generating the qemu command line. This patch proposes a framework to add
arch-specific handler functions for generating different aspects of
command line.
At present, it is used to prevent x86-specific qemu options from
cluttering the qemu command line for other architectures. Going forward,
if newer drivers get defined for any arch, the code for handling these
might be added by extending the handler function for that architecture.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_command.c | 220 ---
src/qemu/qemu_command.h | 20
2 files changed, 169 insertions(+), 71 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b044050..61dabbf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver,
VIR_DOMAIN_FS_DRIVER_TYPE_LAST,
"local",
...
[snip]
...
-if (source != NULL) {
-char *smbioscmd;
+if (!strcmp(def->os.arch, "i686") ||
+ !strcmp(def->os.arch, "x86_64"))
The coding guidelines require use of STREQ or STRNEQ
I'm sorry, overlooked this. I'll correct this to use STREQ.
+qemuCmdFunc =&qemuCmdLineX86;
+else {
+if (!strcmp(def->os.arch, "ppc64"))
+qemuCmdFunc =&qemuCmdLinePPC64;
+ }
If we get an arch that is not PPC or x86, then 'qemuCmdFunc'
can end up NULL
-smbioscmd = qemuBuildSmbiosBiosStr(source);
-if (smbioscmd != NULL) {
-virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
-VIR_FREE(smbioscmd);
-}
-smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid);
-if (smbioscmd != NULL) {
-virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
-VIR_FREE(smbioscmd);
-}
-}
+if (qemuCmdFunc->qemuBuildArchFunction) {
Which will cause a SEGV here.
My bad, I should have had something like:
if (qemuCmdFunc && qemuCmdFunc->qemubuildArchFunction)
...
I'll correct this.
...
[snip]
The convention we try to follow is that if there is something in the
XML which is not supported by the QEMU we're about to run, then we
should raise an error VIR_ERR_CONFIG_UNSUPPORTED.
Thus I'd expect to see a PPC method which raises such an error if
the user requests no-ACPI, serial BIOS output, or SMBIOS data.
Thank you for explaining this.
I get your point -- libvirt should be able to flag an error if the user
tries to specify in XML, a feature unsupported by qemu.
To check if qemu supports a certain feature, libvirt parses the -help
string which is generated by running the qemu binary with the '-h'
argument.
Example: For libvirt to determine if '-no-acpi' is a valid option, it
parses the qemu help string to determine if '-no-acpi' is a supported
feature. If this option is found in the help string, libvirt assumes
that this is an allowed option.
However, qemu itself has an inherent limitation. When generating the
list of allowed options with the '-h' flags, qemu blindly lists all
options instead of doing any arch-specific checking. Example, 'no-acpi'
is defined with an architecture mask for only x86.
$cat $QEMU_GIT/qemu-options.hx :
.
DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
"-no-acpidisable ACPI\n", QEMU_ARCH_I386)
STEXI
@item -no-acpi
@findex -no-acpi
Disable ACPI (Advanced Configuration and Power Interface) support. Use
it if your guest OS complains about ACPI problems (PC target machine
only).
ETEXI
However, the arch-specific masks are not checked while generating the
help string.
$cat $QEMU_GIT/vl.c:
...
const char *options_help =
#define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \
opt_help
#define DEFHEADING(text) stringify(text) "\n"
#include "qemu-options.def"
(where qemu-options.def is generated from qemu-options.hx)
This essentially means that the 'help' string emitted by qemu is
identical, irrespective of the architecture.
So, even if a qemu binary meant for a non-x86 architecture may support a
subset of qemu's x86 features, the qemu binary for that architecture
will always display an identical help string as x86-- never advertising
its true features. This will trick libvirt, because launching
qemu-system-ppc64 or qemu-system-mips with '-no-acpi' errors out with:
"Option no-acpi not supported for this target"
There is also a second part of this problem. Libvirt til
Re: [libvirt] [RFC PATCH v2 4/4] PowerPC : Bifurcate arch-specific qemu initialization code from generic qemu command line generation.
On Mon, Nov 14, 2011 at 08:26:55PM +0530, Prerna Saxena wrote:
> From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001
> From: Prerna Saxena
> Date: Mon, 14 Nov 2011 19:43:26 +0530
> Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from
> generic qemu commandline building code.
> At present, qemuBuildCommandLine emits many x86-specific options while
> generating the qemu command line. This patch proposes a framework to add
> arch-specific handler functions for generating different aspects of
> command line.
> At present, it is used to prevent x86-specific qemu options from
> cluttering the qemu command line for other architectures. Going forward,
> if newer drivers get defined for any arch, the code for handling these
> might be added by extending the handler function for that architecture.
>
> Signed-off-by: Prerna Saxena
> ---
> src/qemu/qemu_command.c | 220
> ---
> src/qemu/qemu_command.h | 20
> 2 files changed, 169 insertions(+), 71 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b044050..61dabbf 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver,
> VIR_DOMAIN_FS_DRIVER_TYPE_LAST,
>"local",
>"handle");
>
> +virQemuCommandLineFunction qemuCmdLineX86 =
> +{ .qemuBuildArchFunction = qemuBuildX86CommandLine,
> +};
> +
> +virQemuCommandLineFunction qemuCmdLinePPC64 =
> +{ .qemuBuildArchFunction = NULL,
> +};
>
> static void
> uname_normalize (struct utsname *ut)
> @@ -3254,6 +3261,8 @@ qemuBuildCommandLine(virConnectPtr conn,
> bool emitBootindex = false;
> int usbcontroller = 0;
> bool usblegacy = false;
> +char *command_str;
> +virQemuCommandLineFunctionPtr qemuCmdFunc;
> uname_normalize(&ut);
>
> virUUIDFormat(def->uuid, uuid);
> @@ -3409,54 +3418,22 @@ qemuBuildCommandLine(virConnectPtr conn,
> }
> }
>
> -if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) &&
> -(def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) {
> -virSysinfoDefPtr source = NULL;
> -bool skip_uuid = false;
> -
> -if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) {
> -qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -_("the QEMU binary %s does not support smbios settings"),
> -emulator);
> -goto error;
> -}
> -
> -/* should we really error out or just warn in those cases ? */
> -if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) {
> -if (driver->hostsysinfo == NULL) {
> -qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -_("Host SMBIOS information is not available"));
> -goto error;
> -}
> -source = driver->hostsysinfo;
> -/* Host and guest uuid must differ, by definition of UUID. */
> -skip_uuid = true;
> -} else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) {
> -if (def->sysinfo == NULL) {
> -qemuReportError(VIR_ERR_XML_ERROR,
> -_("Domain '%s' sysinfo are not available"),
> - def->name);
> -goto error;
> -}
> -source = def->sysinfo;
> -/* domain_conf guaranteed that system_uuid matches guest uuid. */
> -}
> -if (source != NULL) {
> -char *smbioscmd;
> +if (!strcmp(def->os.arch, "i686") ||
> + !strcmp(def->os.arch, "x86_64"))
The coding guidelines require use of STREQ or STRNEQ
> +qemuCmdFunc = &qemuCmdLineX86;
> +else {
> +if (!strcmp(def->os.arch, "ppc64"))
> +qemuCmdFunc = &qemuCmdLinePPC64;
> + }
If we get an arch that is not PPC or x86, then 'qemuCmdFunc'
can end up NULL
>
> -smbioscmd = qemuBuildSmbiosBiosStr(source);
> -if (smbioscmd != NULL) {
> -virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
> -VIR_FREE(smbioscmd);
> -}
> -smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid);
> -if (smbioscmd != NULL) {
> -virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
> -VIR_FREE(smbioscmd);
> -}
> -}
> +if (qemuCmdFunc->qemuBuildArchFunction) {
Which will cause a SEGV here.
> +command_str = (qemuCmdFunc->qemuBuildArchFunction)(conn, driver, def,
> +qemuCaps);
> +if (command_str) {
> +virCommandAddArg(cmd, command_str);
> +VIR_FREE(command_str);
> + }
> }
> -
> /*
> * NB, -nographic *MUST* come before any serial, or monitor
> * or parallel port flags due to
[libvirt] [RFC PATCH v2 4/4] PowerPC : Bifurcate arch-specific qemu initialization code from generic qemu command line generation.
>From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001
From: Prerna Saxena
Date: Mon, 14 Nov 2011 19:43:26 +0530
Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from
generic qemu commandline building code.
At present, qemuBuildCommandLine emits many x86-specific options while
generating the qemu command line. This patch proposes a framework to add
arch-specific handler functions for generating different aspects of
command line.
At present, it is used to prevent x86-specific qemu options from
cluttering the qemu command line for other architectures. Going forward,
if newer drivers get defined for any arch, the code for handling these
might be added by extending the handler function for that architecture.
Signed-off-by: Prerna Saxena
---
src/qemu/qemu_command.c | 220 ---
src/qemu/qemu_command.h | 20
2 files changed, 169 insertions(+), 71 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b044050..61dabbf 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver,
VIR_DOMAIN_FS_DRIVER_TYPE_LAST,
"local",
"handle");
+virQemuCommandLineFunction qemuCmdLineX86 =
+{ .qemuBuildArchFunction = qemuBuildX86CommandLine,
+};
+
+virQemuCommandLineFunction qemuCmdLinePPC64 =
+{ .qemuBuildArchFunction = NULL,
+};
static void
uname_normalize (struct utsname *ut)
@@ -3254,6 +3261,8 @@ qemuBuildCommandLine(virConnectPtr conn,
bool emitBootindex = false;
int usbcontroller = 0;
bool usblegacy = false;
+char *command_str;
+virQemuCommandLineFunctionPtr qemuCmdFunc;
uname_normalize(&ut);
virUUIDFormat(def->uuid, uuid);
@@ -3409,54 +3418,22 @@ qemuBuildCommandLine(virConnectPtr conn,
}
}
-if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) &&
-(def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) {
-virSysinfoDefPtr source = NULL;
-bool skip_uuid = false;
-
-if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) {
-qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-_("the QEMU binary %s does not support smbios settings"),
-emulator);
-goto error;
-}
-
-/* should we really error out or just warn in those cases ? */
-if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) {
-if (driver->hostsysinfo == NULL) {
-qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-_("Host SMBIOS information is not available"));
-goto error;
-}
-source = driver->hostsysinfo;
-/* Host and guest uuid must differ, by definition of UUID. */
-skip_uuid = true;
-} else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) {
-if (def->sysinfo == NULL) {
-qemuReportError(VIR_ERR_XML_ERROR,
-_("Domain '%s' sysinfo are not available"),
- def->name);
-goto error;
-}
-source = def->sysinfo;
-/* domain_conf guaranteed that system_uuid matches guest uuid. */
-}
-if (source != NULL) {
-char *smbioscmd;
+if (!strcmp(def->os.arch, "i686") ||
+ !strcmp(def->os.arch, "x86_64"))
+qemuCmdFunc = &qemuCmdLineX86;
+else {
+if (!strcmp(def->os.arch, "ppc64"))
+qemuCmdFunc = &qemuCmdLinePPC64;
+ }
-smbioscmd = qemuBuildSmbiosBiosStr(source);
-if (smbioscmd != NULL) {
-virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
-VIR_FREE(smbioscmd);
-}
-smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid);
-if (smbioscmd != NULL) {
-virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL);
-VIR_FREE(smbioscmd);
-}
-}
+if (qemuCmdFunc->qemuBuildArchFunction) {
+command_str = (qemuCmdFunc->qemuBuildArchFunction)(conn, driver, def,
+qemuCaps);
+if (command_str) {
+virCommandAddArg(cmd, command_str);
+VIR_FREE(command_str);
+ }
}
-
/*
* NB, -nographic *MUST* come before any serial, or monitor
* or parallel port flags due to QEMU craziness, where it
@@ -3475,25 +3452,6 @@ qemuBuildCommandLine(virConnectPtr conn,
"-nodefaults"); /* Disable default guest devices */
}
-/* Serial graphics adapter */
-if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) {
-if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-_("qemu does not support -device"));
-
