[libvirt] [PATCH 0/3] Fix default vram setting for libxl
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices. Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse src/conf/domain_conf.c | 105 ++++++++++++++++++++++-------------------- src/qemu/qemu_parse_command.c | 2 - 2 files changed, 56 insertions(+), 51 deletions(-) -- 2.7.3
Future commit will call DeviceDefPostParse on a device auto-added in DomainDefPostParse. --- src/conf/domain_conf.c | 78 +++++++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 571b7bf..13d0f9f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3924,45 +3924,6 @@ virDomainDefPostParseTimer(virDomainDefPtr def) } -static int -virDomainDefPostParseInternal(virDomainDefPtr def, - virCapsPtr caps ATTRIBUTE_UNUSED, - unsigned int parseFlags) -{ - /* verify init path for container based domains */ - if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("init binary must be specified")); - return -1; - } - - if (virDomainDefPostParseMemory(def, parseFlags) < 0) - return -1; - - if (virDomainDefRejectDuplicateControllers(def) < 0) - return -1; - - if (virDomainDefRejectDuplicatePanics(def) < 0) - return -1; - - if (virDomainDefPostParseTimer(def) < 0) - return -1; - - if (virDomainDefAddImplicitDevices(def) < 0) - return -1; - - /* Mark the first video as primary. If the user specified primary="yes", - * the parser already inserted the device at def->videos[0] */ - if (def->nvideos != 0) - def->videos[0]->primary = true; - - /* clean up possibly duplicated metadata entries */ - virDomainDefMetadataSanitize(def); - - return 0; -} - - /* Check if a drive type address $controller:$bus:$target:$unit is already * taken by a disk or not. */ @@ -4399,6 +4360,45 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, } +static int +virDomainDefPostParseInternal(virDomainDefPtr def, + virCapsPtr caps ATTRIBUTE_UNUSED, + unsigned int parseFlags) +{ + /* verify init path for container based domains */ + if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("init binary must be specified")); + return -1; + } + + if (virDomainDefPostParseMemory(def, parseFlags) < 0) + return -1; + + if (virDomainDefRejectDuplicateControllers(def) < 0) + return -1; + + if (virDomainDefRejectDuplicatePanics(def) < 0) + return -1; + + if (virDomainDefPostParseTimer(def) < 0) + return -1; + + if (virDomainDefAddImplicitDevices(def) < 0) + return -1; + + /* Mark the first video as primary. If the user specified primary="yes", + * the parser already inserted the device at def->videos[0] */ + if (def->nvideos != 0) + def->videos[0]->primary = true; + + /* clean up possibly duplicated metadata entries */ + virDomainDefMetadataSanitize(def); + + return 0; +} + + int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, -- 2.7.3
Commit 6879be48 moved adding of an implicit video device after XML parsing. As a result, libxlDomainDeviceDefPostParse() is no longer called to set the default vram when adding an implicit device. Commit 6879be48 assumes virDomainVideoDefaultRAM() will set the default vram, but it returns 0 if the domain virtType is VIR_DOMAIN_VIRT_XEN. Attempting to start an HVM domain with vram=0 results in error: unsupported configuration: videoram must be at least 4MB for CIRRUS The default vram setting for Xen HVM domains depends on the device model used (qemu-xen vs qemu-traditional), hence setting the default is deferred to libxlDomainDeviceDefPostParse(). Call the device post-parse callback even for implicit video, to fill out the default vram even for VIR_DOMAIN_VIRT_XEN. https://bugzilla.redhat.com/show_bug.cgi?id=1334557 Most-of-commit-message-by: Jim Fehlig <jfehlig@suse.com> --- src/conf/domain_conf.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 13d0f9f..b3165fc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4362,8 +4362,7 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, static int virDomainDefPostParseInternal(virDomainDefPtr def, - virCapsPtr caps ATTRIBUTE_UNUSED, - unsigned int parseFlags) + struct virDomainDefPostParseDeviceIteratorData *data) { /* verify init path for container based domains */ if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) { @@ -4372,7 +4371,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def, return -1; } - if (virDomainDefPostParseMemory(def, parseFlags) < 0) + if (virDomainDefPostParseMemory(def, data->parseFlags) < 0) return -1; if (virDomainDefRejectDuplicateControllers(def) < 0) @@ -4387,11 +4386,22 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefAddImplicitDevices(def) < 0) return -1; - /* Mark the first video as primary. If the user specified primary="yes", - * the parser already inserted the device at def->videos[0] */ - if (def->nvideos != 0) + if (def->nvideos != 0) { + virDomainDeviceDef device = { + .type = VIR_DOMAIN_DEVICE_VIDEO, + .data.video = def->videos[0], + }; + + /* Mark the first video as primary. If the user specified primary="yes", + * the parser already inserted the device at def->videos[0] */ def->videos[0]->primary = true; + /* videos[0] might have been added in AddImplicitDevices, after we've + * done the per-device post-parse */ + if (virDomainDefPostParseDeviceIterator(NULL, &device, NULL, data) < 0) + return -1; + } + /* clean up possibly duplicated metadata entries */ virDomainDefMetadataSanitize(def); @@ -4429,7 +4439,7 @@ virDomainDefPostParse(virDomainDefPtr def, return ret; - if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0) + if ((ret = virDomainDefPostParseInternal(def, &data)) < 0) return ret; if (virDomainDefPostParseCheckFeatures(def, xmlopt) < 0) -- 2.7.3
Move filling out the default video (v)ram to DeviceDefPostParse. This means it can be removed from virDomainVideoDefParseXML and qemuParseCommandLine. Also, we no longer need to special case VIR_DOMAIN_VIRT_XEN, since the per-driver callback gets called before the generic one. --- src/conf/domain_conf.c | 15 ++++++--------- src/qemu/qemu_parse_command.c | 2 -- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3165fc..ed0c471 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4174,6 +4174,12 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { virDomainVideoDefPtr video = dev->data.video; + /* Fill out (V)RAM if the driver-specific callback did not do so */ + if (video->ram == 0 && video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) + video->ram = virDomainVideoDefaultRAM(def, video->type); + if (video->vram == 0) + video->vram = virDomainVideoDefaultRAM(def, video->type); + video->ram = VIR_ROUND_UP_POWER_OF_TWO(video->ram); video->vram = VIR_ROUND_UP_POWER_OF_TWO(video->vram); } @@ -12045,10 +12051,6 @@ unsigned int virDomainVideoDefaultRAM(const virDomainDef *def, const virDomainVideoType type) { - /* Defer setting default vram to the Xen drivers */ - if (def->virtType == VIR_DOMAIN_VIRT_XEN) - return 0; - switch (type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: @@ -12227,8 +12229,6 @@ virDomainVideoDefParseXML(xmlNodePtr node, _("cannot parse video ram '%s'"), ram); goto error; } - } else if (def->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - def->ram = virDomainVideoDefaultRAM(dom, def->type); } if (vram) { @@ -12237,8 +12237,6 @@ virDomainVideoDefParseXML(xmlNodePtr node, _("cannot parse video vram '%s'"), vram); goto error; } - } else { - def->vram = virDomainVideoDefaultRAM(dom, def->type); } if (vram64) { @@ -18697,7 +18695,6 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def) _("cannot determine default video type")); goto cleanup; } - video->vram = virDomainVideoDefaultRAM(def, video->type); video->heads = 1; if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0) goto cleanup; diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 7ce90f9..e30586f 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -2583,9 +2583,7 @@ qemuParseCommandLine(virCapsPtr caps, vid->type = VIR_DOMAIN_VIDEO_TYPE_XEN; else vid->type = video; - vid->vram = virDomainVideoDefaultRAM(def, vid->type); if (vid->type == VIR_DOMAIN_VIDEO_TYPE_QXL) { - vid->ram = virDomainVideoDefaultRAM(def, vid->type); vid->vgamem = QEMU_QXL_VGAMEM_DEFAULT; } else { vid->ram = 0; -- 2.7.3
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com
This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series. Regards, Jim
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com
This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Thanks, pushed now. Jan
On Thu, May 12, 2016 at 10:30:24 +0200, Ján Tomko wrote:
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com
This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Thanks, pushed now.
This broke few parts of the xen testsuite: TEST: xmconfigtest .!.!.!.!.....!.!.!.!.!.!.!.!.!.!.!.!.!.! 40 .!.!.!.!.!.!.!.!!.!.!.! 63 FAIL TEST: xlconfigtest ...!!!.!.!.!.!.....!.!.!!! 26 FAIL TEST: sexpr2xmltest .!.!!!!!.........!....!..!!!!.!!!!!!!!!! 40 !!!!!!!!.! 50 FAIL Some examples: 59) Xen XM-2-XML Format escape-paths ... In '/home/pipo/libvirt/tests/xmconfigdata/test-escape-paths.xml': Offset 1942 Expect [h] Actual [vram='16384' h]
On 05/12/2016 09:30 AM, Ján Tomko wrote:
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com
This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Thanks, pushed now. I wonder if this series should be also on v1.3.3-maint branch alongside commit 96b21fb ("Fix tests to include video ram size") and Jim's series (400e716 b90c4b5 "libxl: A few more fixes related to vram") ?
The bug https://bugzilla.redhat.com/show_bug.cgi?id=1334557 was originally reported on v1.3.3.1 so probably requires theses fixes too. Joao
On 05/16/2016 05:47 AM, Joao Martins wrote:
On 05/12/2016 09:30 AM, Ján Tomko wrote:
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com
This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Thanks, pushed now. I wonder if this series should be also on v1.3.3-maint branch alongside commit 96b21fb ("Fix tests to include video ram size") and Jim's series (400e716 b90c4b5 "libxl: A few more fixes related to vram") ?
The bug https://bugzilla.redhat.com/show_bug.cgi?id=1334557 was originally reported on v1.3.3.1 so probably requires theses fixes too.
I've pushed them to v1.3.3-maint now Thanks, Cole
On 05/16/2016 09:32 AM, Cole Robinson wrote:
On 05/16/2016 05:47 AM, Joao Martins wrote:
On 05/12/2016 09:30 AM, Ján Tomko wrote:
On Wed, May 11, 2016 at 03:47:06PM -0600, Jim Fehlig wrote:
On 05/11/2016 04:57 AM, Ján Tomko wrote:
An alternative to https://www.redhat.com/archives/libvir-list/2016-May/msg00700.html 1462942877-31516-1-git-send-email-jfehlig@suse.com
This series adds a call to DeviceDefPostParse for the implicit video which is being in the DomainDefPostParse phase, after DeviceDefPostParse has been done on all the existing devices.
I much prefer this approach over fixing it in the driver, but I'm not terribly familiar with this code, hence the attempt to fix it in libxl.
Ján Tomko (3): Move virDomainDefPostParseInternal after virDomainDeviceDefPostParse Call per-device post-parse callback even on implicit video Fill out default vram in DeviceDefPostParse
I reviewed and tested the patches and did not notice any problems - ACK series.
Thanks, pushed now. I wonder if this series should be also on v1.3.3-maint branch alongside commit 96b21fb ("Fix tests to include video ram size") and Jim's series (400e716 b90c4b5 "libxl: A few more fixes related to vram") ?
The bug https://bugzilla.redhat.com/show_bug.cgi?id=1334557 was originally reported on v1.3.3.1 so probably requires theses fixes too.
I've pushed them to v1.3.3-maint now
Also note there was another bug that made virt-install fail, something wrong with the <graphics> XML output I suspect, but I didn't dig into it: https://bugzilla.redhat.com/show_bug.cgi?id=1334562 Thanks, Cole
participants (5)
-
Cole Robinson -
Jim Fehlig -
Joao Martins -
Ján Tomko -
Peter Krempa