[libvirt] [PATCH 3/3] qemu_process: detect updated video ram size values from QEMU
John Ferlan
jferlan at redhat.com
Tue Dec 16 01:01:54 UTC 2014
On 12/11/2014 10:42 AM, Pavel Hrdina wrote:
> QEMU internally updates the size of video memory if you specify too low
s/you specify/the domain XML had provided a/
?Or something like that...
> memory size or there are some dependencies like for QXL device and its
s/like for/for
s/QXL device and its/a QXL devices'
> 'vgamem' and 'ram' size. We need to know about the changes and store
> them into the status XML to not break migration or managedsave trough
s/trough/through
> different libvirt versions.
>
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
> src/qemu/qemu_monitor.c | 34 +++++++++++++++
> src/qemu/qemu_monitor.h | 10 +++++
> src/qemu/qemu_process.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 152 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index ee4460f..e061e02 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1153,6 +1153,40 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
> return ret;
> }
>
> +
> +/**
> + * For device finds its QOM Object path and after that reads and return
> + * specified property. If the path cannot be found or the QOM Object doesn't
> + * have that property it will fail and report appropriate error message.
> + *
> + * Returns 0 on success, otherwise -1.
> + */
> +int
> +qemuMonitorGetDeviceParam(qemuMonitorPtr mon,
> + const char *device,
> + const char *name,
> + qemuMonitorObjectPropertyPtr prop)
> +{
> + char *path = NULL;
> +
> + if (qemuMonitorFindObjectPath(mon, "/", device, &path) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to find QOM Object path for device '%s'"),
> + device);
> + return -1;
> + }
> +
> + if (qemuMonitorJSONGetObjectProperty(mon, path, name, prop)) {
prop) < 0)
right?
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("QOM Object '%s' has no property '%s'"), device, name);
> + VIR_FREE(path);
> + return -1;
> + }
[1] See below - searches through qemuMonitorFindObjectPath are somewhat
expensive (how long is the chain if we have to start at "/" each time),
but I assume that the 'prop' returned could be an array and then for
each prop<input> you'd be able to fill in the prop<output> rather than
make 'n' sequential calls.
> +
VIR_FREE(path);
(strange my Coverity run didn't pick up on it, but my eyes did). Then I
thought - hey why not create two functions - the first returns path and
the second uses it as many times as there are properties to look up
instead of a prop array...
> + return 0;
> +}
> +
> +
> int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon,
> const char *cmd,
> int scm_fd,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 4918588..dc74964 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -271,6 +271,12 @@ virJSONValuePtr qemuMonitorGetOptions(qemuMonitorPtr mon)
> ATTRIBUTE_NONNULL(1);
> void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
> ATTRIBUTE_NONNULL(1);
> +int qemuMonitorGetDeviceParam(qemuMonitorPtr mon,
> + const char *device,
> + const char *name,
> + qemuMonitorObjectPropertyPtr prop)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> + ATTRIBUTE_NONNULL(4);
> int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon,
> const char *cmd,
> int scm_fd,
> @@ -367,6 +373,10 @@ int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> int **pids);
> int qemuMonitorGetVirtType(qemuMonitorPtr mon,
> int *virtType);
> +int qemuMonitorGetVideoDeviceParam(qemuMonitorPtr mon,
> + const char *device,
> + const char *name,
> + qemuMonitorObjectPropertyPtr prop);
> int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon,
> unsigned long long *currmem);
> int qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ab4df9b..551d48c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3037,6 +3037,110 @@ qemuProcessCleanupChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED,
> }
>
>
> +/**
> + * Loads and update video memory size for video devices according to QEMU
> + * process as the QEMU will silently update the values that we pass to QEMU
> + * through command line. We need to load these updated values and store them
> + * into the status XML.
> + *
So in essence, we are searching the object tree's for link<VGA>,
link<qxl-vga>/link<qxl>, or link<vmware-svga> for the specific
properties listed. Just something to add to this entry so it's easier
for the next person to understand what's going on if they choose to read
the comments...
> + * We will fail if for some reason the values cannot be loaded from QEMU because
> + * its mandatory to get the correct video memory size to status XML to not break
> + * migration.
> + */
> +static int
> +qemuProcessUpdateVideoRamSize(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + int asyncJob)
> +{
> + ssize_t i;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virDomainVideoDefPtr video = NULL;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> + qemuMonitorObjectProperty prop = {
> + QEMU_MONITOR_OBJECT_PROPERTY_ULONG,
> + {0}
> + };
> +
> + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> + return -1;
> +
> + for (i = 0; i < vm->def->nvideos; i++) {
> + video = vm->def->videos[i];
> + switch (video->type) {
> + case VIR_DOMAIN_VIDEO_TYPE_VGA:
> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VGA_VGAMEM)) {
> + if (qemuMonitorGetDeviceParam(priv->mon,
> + "VGA",
> + "vgamem_mb",
> + &prop) < 0) {
> + goto error;
> + }
> +
> + video->vram = prop.val.ul * 1024;
> + }
> + break;
> + case VIR_DOMAIN_VIDEO_TYPE_QXL:
> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QXL_VGA_VGAMEM)) {
> + if (qemuMonitorGetDeviceParam(priv->mon,
> + video->primary ? "qxl-vga" : "qxl",
> + "vram_size",
> + &prop) < 0) {
> + goto error;
> + }
> +
> + video->vram = prop.val.ul / 1024;
> + if (qemuMonitorGetDeviceParam(priv->mon,
> + video->primary ? "qxl-vga" : "qxl",
> + "ram_size",
> + &prop) < 0) {
> + goto error;
> + }
> +
> + video->ram = prop.val.ul / 1024;
> + if (qemuMonitorGetDeviceParam(priv->mon,
> + video->primary ? "qxl-vga" : "qxl",
> + "vgamem_mb",
> + &prop) < 0) {
> + goto error;
> + }
> +
> + video->vgamem = prop.val.ul * 1024;
Three trips through qemuMonitorFindObjectPath is "expensive"... This is
where it might be worthwhile to generate an array of prop's:
prop[0] = "vram_size"
prop[1] = "ram_size"
prop[2] = "vgamem_mb"
Which could then be used in the qemuMonitorGetDeviceParam*s*() to return
each data point desired.
or
return path from the first call and make 'n' calls using that path to
get properties. Although I like the idea of an array better.
> + }
> + break;
> + case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VMWARE_SVGA_VGAMEM)) {
> +
> + if (qemuMonitorGetDeviceParam(priv->mon,
> + "vmware-svga",
> + "vgamem_mb",
> + &prop) < 0) {
> + goto error;
> + }
> +
> + video->vram = prop.val.ul * 1024;
> + }
> + break;
> + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> + case VIR_DOMAIN_VIDEO_TYPE_XEN:
> + case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> + case VIR_DOMAIN_VIDEO_TYPE_LAST:
> + break;
> + }
> + }
We could get here without changing anything.... Either we don't have the
specific CAP available, the value is the same as we already have stored,
or we fall into the last group of case's. In any of those cases the
subsequent save is only ripe for unnecessary errors... Maybe it's best
to have some sort of boolean that indicates a value was changed.
> +
> + qemuDomainObjExitMonitor(driver, vm);
> +
> + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
There's a few places in qemu_driver which do a VIR_WARN("Failed to save
status on vm %s", vm->def->name); if this fails... Not sure if you want
to follow that here too.
> + return -1;
> +
> + return 0;
> +
> + error:
> + qemuDomainObjExitMonitor(driver, vm);
> + return -1;
> +}
> +
> +
> struct qemuProcessHookData {
> virConnectPtr conn;
> virDomainObjPtr vm;
> @@ -4787,6 +4891,10 @@ int qemuProcessStart(virConnectPtr conn,
> }
> qemuDomainObjExitMonitor(driver, vm);
>
> + VIR_DEBUG("Detecting actual memory size for video device");
> + if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0)
> + goto cleanup;
> +
This only works if mon->json, correct? So if we don't have that, then
we're dead in the water?
Currently we're not dead in the water, incorrect values perhaps
displayed/used, but we're not failing to start up.
Is that what we is desired?
I think overall it looks ok, there's a couple of adjustments though and
things to understand better before an ACK though...
John
> if (!(flags & VIR_QEMU_PROCESS_START_PAUSED)) {
> VIR_DEBUG("Starting domain CPUs");
> /* Allow the CPUS to start executing */
>
More information about the libvir-list
mailing list