[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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<prerna linux vnet ibm com>
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<prerna linux vnet ibm com>
---
  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-acpi        disable 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 till now has been x86-based only, and so it probes host-architecture qemu to gather supported features, and continues to use these while validating a guest's features. For example, libvirt should make a decision about processing 'acpi' XML feature only if the guest architecture fundamentally supports this. At present, libvirt on x86 assumes acpi to be a mandatory feature in any guest XML. In the absence of the <acpi> feature, it adds 'no-acpi' in the qemu command line. On an x86 host, while this might be fine for a guest running via qemu-system-x86_64 , it will not be right for a guest running via qemu-system-sparc. Considering libvirt is potentially capable of launching pure emulation based cross-architecture guests, it would be an incorrect assumption. The decision to even process the 'acpi' tag should be taken based on whether the guest architecture supports this -- this patch attempts to accomplish this. This patch transfers the decision of processing an XML tag to an arch-specific handler. In the absence of this handler, the option is silently ignored.

Finally, looking at the changes, I'd be somewhat amazed if this
refactoring has not broken the test suite since AFAICT, it changes
the ordering of some command line arguments. Did 'make check'
really pass after applying this ?


I'm sorry, I'd tested this by hand and did not run a 'make check' at the time. Ran it now to see that 59 tests pass and 3 tests fail.
I will remember to run 'make check' henceforth.

...
[snip]
...
+typedef struct _virQemuCommandLineFunction virQemuCommandLineFunction;
+typedef struct _virQemuCommandLineFunction* virQemuCommandLineFunctionPtr;

These can both be in the source file too. Also the naming convention for
the QEMU driver source files is a prefix 'qemu' ather than 'virQemu'.


Agree, I'll address this.

@@ -53,6 +63,16 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn,
                                     enum virVMOperationType vmop)
      ATTRIBUTE_NONNULL(1);

+/* Build Additional X86-specific options on command line */
+char * qemuBuildX86CommandLine(virConnectPtr conn,
+                               struct qemud_driver *driver,
+                               virDomainDefPtr def,
+                               virBitmapPtr qemuCaps);

I think this function can just be static - there's no need to be able
to access it outside the source file it lives in.


Will do.
 ..[snip]..

Shared my understanding on the present limitations of qemu / libvirt; and the need for an arch-based cleanup patch. I would love to stand corrected if I have overlooked any aspect. It will also be nice to hear of suggestions on how I can organize this design better.

Thanks,
--
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]