On 16.07.2014 23:05, Jim Fehlig wrote:
> Stefan Bader wrote:
>> being as bad with timely responses. Ok, so how about the following?
>>
>> One note: it could be the STRDUP's are not strictly needed. But
>> to me it felt wrong to have two places refer to the same strings
>> (as MakeVFB copies the struct containing the pointers).
>
> Agreed. Without the STRDUP's, seems there is a potential for double
> free when libxl_device_vfb and libxl_domain_config objects are disposed.
>
>> If this
>> is not needed, then all changes now in MakeVFB probably can be
>> dropped (except setting the keyboard layout, maybe; which I
>> might miss ;)).
>>
>> -Stefan
>>
>>
>> >From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
>> Date: Thu, 27 Mar 2014 16:01:18 +0100
>> Subject: [PATCH] libxl: Implement basic video device selection
>>
>> This started as an investigation into an issue where libvirt (using the
>> libxl driver) and the Xen host, like an old couple, could not agree on
>> who is responsible for selecting the VNC port to use.
>>
>> Things usually (and a bit surprisingly) did work because, just like that
>> old couple, they had the same idea on what to do by default. However it
>> was possible that this ended up in a big argument.
>>
>> The problem is that display information exists in two different places:
>> in the vfbs list and in the build info. And for launching the device model,
>> only the latter is used. But that never gets initialized from libvirt. So
>> Xen allows the device model to select a default port while libvirt thinks
>> it has told Xen that this is done by libvirt (though the vfbs config).
>>
>> While fixing that, I made a stab at actually evaluating the configuration
>> of the video device. So that it is now possible to at least decide between
>> a Cirrus or standard VGA emulation and to modify the VRAM within certain
>> limits using libvirt.
>>
>> [v2: Check return code of VIR_STRDUP and fix indentation]
>> [v3: Split out VRAM fixup and return error for unsupported video type]
>> [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
>>
>
> [meta-comment]
> libvirt prefers patch version history like this to be below the '---'
> following your Signed-off-by, so as to not pollute the commit message.
Ah yeah, makes sense. I try to keep it in mind.
>
>> Signed-off-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
>> ---
>> src/libxl/libxl_conf.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 8eeaf82..43cabcf 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
>> libxl_domain_build_info *b_info = &d_config->b_info;
>> libxl_device_vfb vfb = d_config->vfbs[0];
>>
>> - if (libxl_defbool_val(vfb.vnc.enable))
>> + if (libxl_defbool_val(vfb.vnc.enable)) {
>> memcpy(&b_info->u.hvm.vnc, &vfb.vnc, sizeof(libxl_vnc_info));
>> - else if (libxl_defbool_val(vfb.sdl.enable))
>> + if (VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb.vnc.listen) < 0)
>> + goto error;
>> + if (VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb.vnc.passwd) < 0)
>> + goto error;
>> + } else if (libxl_defbool_val(vfb.sdl.enable)) {
>> memcpy(&b_info->u.hvm.sdl, &vfb.sdl, sizeof(libxl_sdl_info));
>> + if (VIR_STRDUP(b_info->u.hvm.sdl.display, vfb.sdl.display) < 0)
>> + goto error;
>> + if (VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb.sdl.xauthority) < 0)
>> + goto error;
>> + }
>> + if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0)
>> + goto error;
>> }
>>
>> return 0;
>> @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>> return NULL;
>> }
>>
>> +static int
>> +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
>> +{
>> + libxl_domain_build_info *b_info = &d_config->b_info;
>> +
>> + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_HVM)
>> + return 0;
>> +
>> + /*
>> + * Take the first defined video device (graphics card) to display
>> + * on the first graphics device (display).
>> + * Right now only type and vram info is used and anything beside
>> + * type xen and vga is mapped to cirrus.
>> + */
>> + if (def->nvideos) {
>> + switch (def->videos[0]->type) {
>> + case VIR_DOMAIN_VIDEO_TYPE_VGA:
>> + case VIR_DOMAIN_VIDEO_TYPE_XEN:
>> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
>> + break;
>> + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
>> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>> + break;
>> + default:
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + "%s",
>> + _("video type not supported by libxl"));
>> + return -1;
>> + }
>> + b_info->video_memkb = def->videos[0]->vram ?
>> + def->videos[0]->vram :
>> + LIBXL_MEMKB_DEFAULT;
>>
>
> While testing this, I noticed that libvirt will set vram to 9216 if not
> specified. E.g.
>
> # cat test.xml
> ...
> <video>
> <model type='vga'/>
> </video>
> ...
> # virsh define test.xml
> # virsh dumpxml test
> ...
> <video>
> <model type='vga' vram='9216' heads='1'/>
> </video>
> ...
>
> With type='vga', libxl will then fail to start the domain
>
> libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault:
> videoram must be at least 16 MB for STDVGA on QEMU_XEN
Heh, thats "funny". At least this was the same thing I observed when creating
new guests with virt-manager. Maybe I blamed the wrong part of the stack. Or
they both do it. This lead the the second part of my changes which was not so
clear about whether it should or should not go upstream.
So from my side a fixup of some kind would be good but we can work on this in a
follow-up.
>
> This could be handled in libxlDomainDeviceDefPostParse(), where we can
> check for sane vram values for the various VIR_DOMAIN_VIDEO_TYPE_*, or
> set sane defaults if vram is not specified.
>
> BTW, sorry again for the delayed response. I somehow missed your
> message and only stumbled across it today :-/. And be warned that any
> followup may be delayed as I'll be on vacation July 18-27.
Oh I am equally bad at having it sit in my reader and then getting back at
random times. :)
-Stefan
>
> Regards,
> Jim
>
>> + } else {
>> + libxl_defbool_set(&b_info->u.hvm.nographic, 1);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int
>> libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>> virDomainDefPtr def,
>> @@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>> if (libxlMakePCIList(def, d_config) < 0)
>> return -1;
>>
>> + /*
>> + * Now that any potential VFBs are defined, it is time to update the
>> + * build info with the data of the primary display. Some day libxl
>> + * might implicitely do so but as it does not right now, better be
>> + * explicit.
>> + */
>> + if (libxlMakeVideo(def, d_config) < 0)
>> + return -1;
>> +
>> d_config->on_reboot = def->onReboot;
>> d_config->on_poweroff = def->onPoweroff;
>> d_config->on_crash = def->onCrash;
>>
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list