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

Re: [libvirt] [PATCH V3 2/3] qemu: Introduce vgamem attribute for video model



On 14.08.2014 14:43, Wang Rui wrote:
From: Zeng Junliang <zengjunliang huawei com>

QEMU suopports to specifie the size of the framebuffer portion of
the "ram" region for vga, vmvga and qxl through the qemu command
line parameter "vgamem_mb".
This patch introduces vgamem attribute for video model, makes the
"vgamem_mb" value configured in libvirt xml. Also, add test cases
and descriptions for it.

Libvirt xml configuration sample(based on VGA):
<video>
   <model type='vga' vgamem='16384' heads='1'/>
</video>

The resulting qemu command line change is the addition of:
-vga std -global VGA.vgamem_mb=16
or
-device VGA,id=video0,vgamem_mb=16,bus=pci.0,addr=0x2

If 'vgamem' is not configured by user, a default value will be assigned
by libvirt. Assume that user wants to start a VM without 'vgamem'
configuration in xml, if QEMU lacks the ability of vgamem, libvirt will
only report a warning rather than make starting failed even though libvirt
has assigned a default value to vgamem. It could avoid a regression.

It's less confusing to introduce the new vgamem attribute.
Prior to the change:
model       libvirt-attribute(xml)      qemu-attribute

qxl         vram                        vram_size
qxl         none                        vgamem_mb
vga         vram                        QEMU has no attribute named vram*
vga         none                        vgamem_mb
vmvga       vram                        QEMU has no attribute named vram*
vmvga       none                        vgamem_mb

After the change:
model       libvirt attribute(xml)      QEMU attribute

qxl         vram                        vram_size
qxl         vgamem                      vgamem_mb
vga         vram                        QEMU has no attribute named vram*
vga         vgamem                      vgamem_mb
vmvga       vram                        QEMU has no attribute named vram*
vmvga       vgamem                      vgamem_mb

Signed-off-by: Zeng Junliang <zengjunliang huawei com>
Signed-off-by: Wang Rui <moon wangrui huawei com>
---
  docs/formatdomain.html.in                          |  35 ++++---
  docs/schemas/domaincommon.rng                      |   5 +
  src/conf/domain_conf.c                             |  52 +++++++++-
  src/conf/domain_conf.h                             |   2 +
  src/libvirt_private.syms                           |   1 +
  src/qemu/qemu_capabilities.c                       |  15 +++
  src/qemu/qemu_capabilities.h                       |   3 +
  src/qemu/qemu_command.c                            | 105 ++++++++++++++++-----
  tests/qemucapabilitiesdata/caps_1.2.2-1.caps       |   3 +
  tests/qemucapabilitiesdata/caps_1.3.1-1.caps       |   3 +
  tests/qemucapabilitiesdata/caps_1.4.2-1.caps       |   3 +
  tests/qemucapabilitiesdata/caps_1.5.3-1.caps       |   3 +
  tests/qemucapabilitiesdata/caps_1.6.0-1.caps       |   3 +
  tests/qemucapabilitiesdata/caps_1.6.50-1.caps      |   3 +
  tests/qemuhelptest.c                               |  10 +-
  .../qemuxml2argvdata/qemuxml2argv-graphics-sdl.xml |   2 +-
  ...emuxml2argv-graphics-spice-agent-file-xfer.args |   5 +-
  ...qemuxml2argv-graphics-spice-agent-file-xfer.xml |   4 +-
  .../qemuxml2argv-graphics-spice-compression.args   |   4 +-
  .../qemuxml2argv-graphics-spice-compression.xml    |   4 +-
  .../qemuxml2argv-graphics-spice-listen-network.xml |   4 +-
  .../qemuxml2argv-graphics-spice-qxl-vga.args       |   4 +-
  .../qemuxml2argv-graphics-spice-qxl-vga.xml        |   4 +-
  .../qemuxml2argv-graphics-spice-sasl.args          |   3 +-
  .../qemuxml2argv-graphics-spice-sasl.xml           |   2 +-
  .../qemuxml2argv-graphics-spice-timeout.xml        |   2 +-
  .../qemuxml2argv-graphics-spice.args               |   5 +-
  .../qemuxml2argv-graphics-spice.xml                |   4 +-
  .../qemuxml2argv-graphics-vnc-std-vga.args         |   4 +
  .../qemuxml2argv-graphics-vnc-std-vga.xml          |  36 +++++++
  .../qemuxml2argv-graphics-vnc-vmware-svga.args     |   4 +
  .../qemuxml2argv-graphics-vnc-vmware-svga.xml      |  36 +++++++
  .../qemuxml2argv-net-bandwidth.xml                 |   2 +-
  .../qemuxml2argv-pcihole64-q35.args                |   3 +-
  .../qemuxml2argv-pcihole64-q35.xml                 |   2 +-
  tests/qemuxml2argvdata/qemuxml2argv-q35.args       |   3 +-
  tests/qemuxml2argvdata/qemuxml2argv-q35.xml        |   2 +-
  .../qemuxml2argv-serial-spiceport.args             |   4 +-
  .../qemuxml2argv-serial-spiceport.xml              |   2 +-
  .../qemuxml2argv-video-device-pciaddr-default.args |   9 +-
  .../qemuxml2argv-video-device-pciaddr-default.xml  |   6 +-
  tests/qemuxml2argvtest.c                           |  22 ++++-
  .../qemuxml2xmlout-graphics-spice-timeout.xml      |   2 +-
  tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml    |   2 +-
  tests/qemuxml2xmltest.c                            |   2 +
  45 files changed, 357 insertions(+), 77 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-std-vga.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-std-vga.xml
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-vmware-svga.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-vnc-vmware-svga.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3012e3c..a0d15c4 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4419,7 +4419,7 @@ qemu-kvm -net nic,model=? /dev/null
    ...
    &lt;devices&gt;
      &lt;video&gt;
-      &lt;model type='vga' heads='1'&gt;
+      &lt;model type='vga' vgamem='16384' heads='1'&gt;
          &lt;acceleration accel3d='yes' accel2d='yes'/&gt;
        &lt;/model&gt;
      &lt;/video&gt;
@@ -4434,16 +4434,17 @@ qemu-kvm -net nic,model=? /dev/null
          is set but there is a <code>graphics</code> in domain xml, then libvirt
          will add a default <code>video</code> according to the guest type.
          For a guest of type "kvm", the default <code>video</code> for it is:
-        <code>type</code> with value "cirrus", and <code>heads</code> with
-        value "1". By default, the first video device in domain xml is the
-        primary one, but the optional attribute <code>primary</code>
-        (<span class="since">since 1.0.2</span>) with value 'yes' can be
-        used to mark the primary in cases of multiple video device.
-        The non-primary must be type of "qxl". The optional attribute
+        <code>type</code> with value "cirrus", <code>vgamem</code>
+        (<span class="since">since 1.2.8</span>) with value "16384"
+        and <code>heads</code> with value "1". By default, the first video
+        device in domain xml is the primary one, but the optional attribute
+        <code>primary</code> (<span class="since">since 1.0.2</span>) with
+        value 'yes' can be used to mark the primary in cases of multiple video
+        device. The non-primary must be type of "qxl". The optional attribute
          <code>ram</code> (<span class="since">since 1.0.2</span>) is allowed
          for "qxl" type only and specifies the size of the primary bar,
          while <code>vram</code> specifies the secondary bar size.
-        If "ram" or "vram" are not supplied a default value is used.
+        If "ram", "vram" or "vgamem" are not supplied a default value is used.
        </dd>

        <dt><code>model</code></dt>
@@ -4452,10 +4453,20 @@ qemu-kvm -net nic,model=? /dev/null
          attribute which takes the value "vga", "cirrus", "vmvga", "xen",
          "vbox", or "qxl" (<span class="since">since 0.8.6</span>)
          depending on the hypervisor features available.
-        You can also provide the amount of video memory in kibibytes
-        (blocks of 1024 bytes) using
-        <code>vram</code> and the number of screen with <code>heads</code>.
-        For type of kvm <code>vram</code> attribute is only valid for "qxl".
+        <p>
+        <code>vram</code> attribute specifies the amount of video memory
+        in kibibytes (blocks of 1024 bytes). For type of kvm, it is only
+        valid for type of "qxl".
+        </p>
+        <p>
+        <code>vgamem</code> attribute <span class="since">since 1.2.8,
+        QEMU and KVM only</span> specifies the size of the framebuffer
+        portion of the "ram" region. And it is only valid for type of
+        "vga", "vmvga" and "qxl".
+        </p>
+        <p>
+        <code>heads</code> attribute specifies the number of screen.
+        </p>
        </dd>

        <dt><code>acceleration</code></dt>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 033f2f6..b2cc218 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2786,6 +2786,11 @@
                <ref name="unsignedInt"/>
              </attribute>
            </optional>
+           <optional>
+            <attribute name="vgamem">
+              <ref name="unsignedInt"/>
+            </attribute>
+          </optional>
            <optional>
              <attribute name="heads">
                <ref name="unsignedInt"/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5c762fa..7097570 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9639,6 +9639,21 @@ virDomainVideoDefaultType(const virDomainDef *def)
      }
  }

+int
+virDomainVideoDefaultVgamem(int type)
+{
+    switch (type) {
+    case VIR_DOMAIN_VIDEO_TYPE_VGA:
+    case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
+    case VIR_DOMAIN_VIDEO_TYPE_QXL:
+        /* QEMU use 16M as default value for vga/vmvga/qxl device*/
+        return 16 * 1024;
+
+    default:
+        return 0;
+    }
+}
+
  static virDomainVideoAccelDefPtr
  virDomainVideoAccelDefParseXML(xmlNodePtr node)
  {
@@ -9694,6 +9709,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
      char *type = NULL;
      char *heads = NULL;
      char *vram = NULL;
+    char *vgamem = NULL;
      char *ram = NULL;
      char *primary = NULL;

@@ -9703,11 +9719,12 @@ virDomainVideoDefParseXML(xmlNodePtr node,
      cur = node->children;
      while (cur != NULL) {
          if (cur->type == XML_ELEMENT_NODE) {
-            if (!type && !vram && !ram && !heads &&
+            if (!type && !vram && !ram && !heads && !vgamem &&
                  xmlStrEqual(cur->name, BAD_CAST "model")) {
                  type = virXMLPropString(cur, "type");
                  ram = virXMLPropString(cur, "ram");
                  vram = virXMLPropString(cur, "vram");
+                vgamem = virXMLPropString(cur, "vgamem");
                  heads = virXMLPropString(cur, "heads");

                  if ((primary = virXMLPropString(cur, "primary")) != NULL) {
@@ -9754,13 +9771,31 @@ virDomainVideoDefParseXML(xmlNodePtr node,
      if (vram) {
          if (virStrToLong_ui(vram, NULL, 10, &def->vram) < 0) {
              virReportError(VIR_ERR_XML_ERROR,
-                           _("cannot parse video ram '%s'"), vram);
+                           _("cannot parse video vram '%s'"), vram);
              goto error;
          }
      } else {
          def->vram = virDomainVideoDefaultRAM(dom, def->type);
      }

+    if (vgamem) {
+        if (def->type != VIR_DOMAIN_VIDEO_TYPE_VGA &&
+            def->type != VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
+            def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Unsupported vgamem attribute for %s device"),
+                             virDomainVideoTypeToString(def->type));
+            goto error;
+        }
+        if (virStrToLong_ui(vgamem, NULL, 10, &def->vgamem) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("cannot parse video vgamem '%s'"), vgamem);
+            goto error;
+        }
+    } else {
+        def->vgamem = virDomainVideoDefaultVgamem(def->type);
+    }
+
      if (heads) {
          if (virStrToLong_ui(heads, NULL, 10, &def->heads) < 0) {
              virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -9777,6 +9812,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
      VIR_FREE(type);
      VIR_FREE(ram);
      VIR_FREE(vram);
+    VIR_FREE(vgamem);
      VIR_FREE(heads);

      return def;
@@ -9786,6 +9822,7 @@ virDomainVideoDefParseXML(xmlNodePtr node,
      VIR_FREE(type);
      VIR_FREE(ram);
      VIR_FREE(vram);
+    VIR_FREE(vgamem);
      VIR_FREE(heads);
      return NULL;
  }
@@ -12996,6 +13033,7 @@ virDomainDefParseXML(xmlDocPtr xml,
              VIR_FREE(video);
              goto error;
          }
+        video->vgamem = virDomainVideoDefaultVgamem(video->type);
          video->vram = virDomainVideoDefaultRAM(def, video->type);
          video->heads = 1;
          if (VIR_ALLOC_N(def->videos, 1) < 0) {
@@ -13891,6 +13929,14 @@ virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src,
          return false;
      }

+    if (src->vgamem != dst->vgamem) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Target video card vgamem %u does not match source %u"),
+                       dst->vgamem, src->vgamem);
+        return false;
+    }
+
+
      if (src->heads != dst->heads) {
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                         _("Target video card heads %u does not match source %u"),
@@ -16937,6 +16983,8 @@ virDomainVideoDefFormat(virBufferPtr buf,
          virBufferAsprintf(buf, " ram='%u'", def->ram);
      if (def->vram)
          virBufferAsprintf(buf, " vram='%u'", def->vram);
+    if (def->vgamem)
+        virBufferAsprintf(buf, " vgamem='%u'", def->vgamem);
      if (def->heads)
          virBufferAsprintf(buf, " heads='%u'", def->heads);
      if (def->primary)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ff7d640..d1ef6ec 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1244,6 +1244,7 @@ struct _virDomainVideoDef {
      int type;
      unsigned int ram;  /* kibibytes (multiples of 1024) */
      unsigned int vram; /* kibibytes (multiples of 1024) */
+    unsigned int vgamem; /* kibibytes (multiples of 1024) */
      unsigned int heads;
      bool primary;
      virDomainVideoAccelDefPtr accel;
@@ -2492,6 +2493,7 @@ virDomainFSDefPtr virDomainFSRemove(virDomainDefPtr def, size_t i);

  int virDomainVideoDefaultType(const virDomainDef *def);
  int virDomainVideoDefaultRAM(const virDomainDef *def, int type);
+int virDomainVideoDefaultVgamem(int type);

  int virDomainObjListNumOfDomains(virDomainObjListPtr doms,
                                   bool active,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 08111d4..e481d92 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -416,6 +416,7 @@ virDomainVcpuPinFindByVcpu;
  virDomainVcpuPinIsDuplicate;
  virDomainVideoDefaultRAM;
  virDomainVideoDefaultType;
+virDomainVideoDefaultVgamem;
  virDomainVideoDefFree;
  virDomainVideoTypeFromString;
  virDomainVideoTypeToString;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 360cc67..146d67c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -265,6 +265,10 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
                "numa",
                "memory-backend-file",
                "usb-audio",
+              "vga-vgamem",
+
+              "qxl-vgamem", /* 175 */
+              "vmware-vgamem",
      );


@@ -1073,6 +1077,14 @@ virQEMUCapsComputeCmdFlags(const char *help,
              virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_QXL);
          if ((p = strstr(p, "|none")) && p < nl)
              virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_NONE);
+        /* It seems that QEMU supports to be communicated with
+         * qmp command since 1.2.0. When qemuCaps->usedQMP is
+         * true, these logical code will be invalid. Does it need here? */
+        if (version >= 1002000) {
+            virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_VGAMEM_MB);
+            virQEMUCapsSet(qemuCaps, QEMU_CAPS_QXL_VGAMEM_MB);
+            virQEMUCapsSet(qemuCaps, QEMU_CAPS_VMWARE_VGAMEM_MB);

At least for cirrus vga this is not true. The vgamem_mb property of cirrus vga was introduced in 19403a68 (v1.3.0). However, I'd expect these to be set by something else than version based test. For instance, aren't the properties listen in device prop query command?

+        }
      }
      if (strstr(help, "-spice"))
          virQEMUCapsSet(qemuCaps, QEMU_CAPS_SPICE);
@@ -3034,6 +3046,9 @@ virQEMUCapsInitQMPBasic(virQEMUCapsPtr qemuCaps)
      virQEMUCapsSet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE);
      virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY);
      virQEMUCapsSet(qemuCaps, QEMU_CAPS_HOST_PCI_MULTIDOMAIN);
+    virQEMUCapsSet(qemuCaps, QEMU_CAPS_VGA_VGAMEM_MB);
+    virQEMUCapsSet(qemuCaps, QEMU_CAPS_QXL_VGAMEM_MB);
+    virQEMUCapsSet(qemuCaps, QEMU_CAPS_VMWARE_VGAMEM_MB);

No.

  }

  /* Capabilities that are architecture depending
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 2911759..cdf6920 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -213,6 +213,9 @@ typedef enum {
      QEMU_CAPS_NUMA               = 171, /* newer -numa handling with disjoint cpu ranges */
      QEMU_CAPS_OBJECT_MEMORY_FILE = 172, /* -object memory-backend-file */
      QEMU_CAPS_OBJECT_USB_AUDIO   = 173, /* usb-audio device support */
+    QEMU_CAPS_VGA_VGAMEM_MB      = 174, /* -global VGA.vgamem_mb */
+    QEMU_CAPS_QXL_VGAMEM_MB      = 175, /* -global qxl-vga.vgamem_mb */
+    QEMU_CAPS_VMWARE_VGAMEM_MB   = 176, /* -global vmware-svga.vgamem_mb */

      QEMU_CAPS_LAST,                   /* this must always be the last item */
  } virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c3f860e..c15099a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4827,6 +4827,14 @@ qemuBuildDeviceVideoStr(virDomainDefPtr def,
          virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024);
      }

+    /* 1. Ignore cirrus-vga as guests would not use it anyway.
+     * 2. QEMU accepts MByte for vgamem_mb and ensure its value
+     * a power of two and range: 1 MB -> 256 MB */
+    if (video->type != VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
+        video->vgamem > 1024) {
+        virBufferAsprintf(&buf, ",vgamem_mb=%u", video->vgamem / 1024);
+    }
+
      if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0)
          goto error;

@@ -8766,36 +8774,86 @@ qemuBuildCommandLine(virConnectPtr conn,

                  virCommandAddArgList(cmd, "-vga", vgastr, NULL);

-                if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
-                    (def->videos[0]->vram || def->videos[0]->ram) &&
-                    virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
-                    const char *dev = (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL_VGA)
-                                       ? "qxl-vga" : "qxl");
+                if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+                    const char *dev = NULL;
+                    bool vgamemSupport = true;
                      int ram = def->videos[0]->ram;
                      int vram = def->videos[0]->vram;
+                    int vgamem = def->videos[0]->vgamem;
+                    switch (primaryVideoType) {
+                    case VIR_DOMAIN_VIDEO_TYPE_VGA:
+                        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
+                            dev = "VGA";
+                        if (dev && vgamem &&
+                            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VGA_VGAMEM_MB)) {
+                            vgamemSupport = false;
+                            VIR_WARN("This QEMU does not support vgamem "
+                                     "attribute, ignore it");

Why? If I deliberately set vgamem the domain startup process must fail if qemu doesn't support it.

Michal


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