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

> 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.