Re: [libvirt] [PATCH 3/3] qemu_process: detect updated video ram size values from QEMU

On 12/11/2014 10:42 AM, Pavel Hrdina wrote: s/you specify/the domain XML had provided a/ ?Or something like that... s/like for/for s/QXL device and its/a QXL devices' s/trough/through prop) < 0) right? [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