From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Bader Subject: Re: [PATCH] libxl: Implement basic video device selection Date: Thu, 17 Jul 2014 11:30:57 +0200 Message-ID: <53C797D1.1000007@canonical.com> References: <5387B626.3040407@suse.com> <1404201498-6673-1-git-send-email-stefan.bader@canonical.com> <53C6E91E.9010500@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2283183179689825455==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1X7i2A-0001OA-QS for xen-devel@lists.xenproject.org; Thu, 17 Jul 2014 09:31:27 +0000 In-Reply-To: <53C6E91E.9010500@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jim Fehlig Cc: libvir-list@redhat.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============2283183179689825455== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="SA7KE6AvRJpdCAJtnmGkQAkEAT0TxUkHk" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --SA7KE6AvRJpdCAJtnmGkQAkEAT0TxUkHk Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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). >=20 > Agreed. Without the STRDUP's, seems there is a potential for double > free when libxl_device_vfb and libxl_domain_config objects are disposed= =2E >=20 >> 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 200= 1 >> From: Stefan Bader >> 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 th= e >> 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 th= at >> old couple, they had the same idea on what to do by default. However i= t >> 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 m= odel, >> 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 thi= nks >> 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 configurat= ion >> of the video device. So that it is now possible to at least decide bet= ween >> a Cirrus or standard VGA emulation and to modify the VRAM within certa= in >> 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] >> =20 >=20 > [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. >=20 >> Signed-off-by: Stefan Bader >> --- >> 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 graphicsp= orts, >> libxl_domain_build_info *b_info =3D &d_config->b_info; >> libxl_device_vfb vfb =3D d_config->vfbs[0]; >> =20 >> - 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_inf= o)); >> - 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_inf= o)); >> + 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.xaut= hority) < 0) >> + goto error; >> + } >> + if (VIR_STRDUP(b_info->u.hvm.keymap, vfb.keymap) < 0) >> + goto error; >> } >> =20 >> return 0; >> @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx) >> return NULL; >> } >> =20 >> +static int >> +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config) >> +{ >> + libxl_domain_build_info *b_info =3D &d_config->b_info; >> + >> + if (d_config->c_info.type !=3D 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 =3D LIBXL_VGA_INTERFACE_TYPE_S= TD; >> + break; >> + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS: >> + b_info->u.hvm.vga.kind =3D LIBXL_VGA_INTERFACE_TYPE_C= IRRUS; >> + break; >> + default: >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + "%s", >> + _("video type not supported by libxl")= ); >> + return -1; >> + } >> + b_info->video_memkb =3D def->videos[0]->vram ? >> + def->videos[0]->vram : >> + LIBXL_MEMKB_DEFAULT; >> =20 >=20 > While testing this, I noticed that libvirt will set vram to 9216 if not= > specified. E.g. >=20 > # cat test.xml > ... > > ... > # virsh define test.xml > # virsh dumpxml test > ... > > ... >=20 > With type=3D'vga', libxl will then fail to start the domain >=20 > 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 crea= ting 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 no= t 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 thi= s in a follow-up. >=20 > 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. >=20 > 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 a= t random times. :) -Stefan >=20 > Regards, > Jim >=20 >> + } else { >> + libxl_defbool_set(&b_info->u.hvm.nographic, 1); >> + } >> + >> + return 0; >> +} >> + >> int >> libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, >> virDomainDefPtr def, >> @@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr grap= hicsports, >> if (libxlMakePCIList(def, d_config) < 0) >> return -1; >> =20 >> + /* >> + * Now that any potential VFBs are defined, it is time to update = the >> + * build info with the data of the primary display. Some day libx= l >> + * might implicitely do so but as it does not right now, better b= e >> + * explicit. >> + */ >> + if (libxlMakeVideo(def, d_config) < 0) >> + return -1; >> + >> d_config->on_reboot =3D def->onReboot; >> d_config->on_poweroff =3D def->onPoweroff; >> d_config->on_crash =3D def->onCrash; >> =20 --SA7KE6AvRJpdCAJtnmGkQAkEAT0TxUkHk Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBCgAGBQJTx5fmAAoJEOhnXe7L7s6jk0YP/0Ty0/JIQ8eHFZQhfYEooVPl xamPEcjruDlifCqMDLsw63eqUpfeXNhrX0uWn2dquub+tn96+WoMfDDk3CJM6eIx De2IBxxLRs+ZFc8TwWvcR3ckZyEddV+OXurFmrb07Hfn5LRIAD8Nq4JA+/4Um0yW izSNba4HGcKfBIBf85KOssYm1JpK5kcZJzT53B5pmlB1eHkrynaxmXRTtO3EPZSG MjX1y3045f1U7k/mNRUinu6HqekNpGe9AyLzTGDCrl3eb2lL2P+BWurdQwZuT02g 373IsMRJq6DlbpOftTevagXDuxTrdH8lNFCP1lNYEp/ORtxnvYay93t0SetXCVwq oO1wgVpdrfSLPGk5lnVIfEWJkAJGI5GlNaht2ihv3NQn1wMCOEeo3KXiShYjhmed 7XrQ/jLR2sNa9ePV5XeQc6CRbPgU3XiInYfTuONn052z2euOFJZWdPghQOeUM+KG WUFbHZnDG8EhuW7RHGl9jindroxC+pNQij+l6eDYsi1t1rkyB7Q6uaN/Xrt+CPfj CfsC0cs9T3iziImmqOWz43XMjoaSIez/6NclmJxVl/JXvngzzmnC7I/Jcc7AEUGR G+9Y57Ms9Pmq6zRqLT4mmXrLL/twEYI4GZSpfuiulylSSixIugZcJ+1YMlRjHY9/ XuTwuZhDa3w0S3WgPi4a =Do6q -----END PGP SIGNATURE----- --SA7KE6AvRJpdCAJtnmGkQAkEAT0TxUkHk-- --===============2283183179689825455== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2283183179689825455==--