Re: [Xen-devel] [libvirt] [PATCH 2/2] libxl: Fix up VRAM to minimum requirements

[Jim Fehlig <jfehlig@xxxxxxxx>]

Stefan Bader wrote:
> This is a bit debatable. On one side it hides configuration errors
> in a way that makes them hard to spot. On the other side there is
> at least one issue with (maybe some older versions) virt-manager.
> Virt-manager sets VRAM directly, not using the default memory setting
> but uses too small values for libxl.

It sounds like virt-manager should be fixed. I don't think the libxl
driver should contain this type of policy. It could report errors on
user-supplied configuration known not to work with libxl, but I don't
think it should be silently changing the configuration.

Regards,
Jim

>  Worse, those versions do not seem
> to allow to change VRAM from the GUI. So switching the video type to
> VGA makes the guest fail to start until one manually adapts the VRAM
> size in the XML definition.
> With this change this would not happen but VRAM will be bigger than
> the GUI says. This would not be that different from current Cirrus
> behaviour. Only that in that case qemu seems to ignore the provided
> size.
>
> Signed-off-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> ---
>  src/libxl/libxl_conf.c |   27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 2b5c469..9af8abe 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1316,13 +1316,38 @@ libxlSetBuildGraphics(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>       * type xen and vga is mapped to cirrus.
>       */
>      if (def->nvideos) {
> +        unsigned int min_vram = 8 * 1024;
> +
>          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;
> +                /*
> +                 * Libxl enforces a minimal VRAM size of 8M when using
> +                 * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
> +                 * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
> +                 * Avoid build failures and go with the minimum if less
> +                 * is specified.
> +                 */
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 8 * 1024;
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 16 * 1024;
> +                }
>                  break;
>              case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
>                  b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
> +                switch (b_info->device_model_version) {
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +                        min_vram = 4 * 1024; /* Actually the max, too */
> +                        break;
> +                    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +                    default:
> +                        min_vram = 8 * 1024;
> +                }
>                  break;
>              default:
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -1330,7 +1355,7 @@ libxlSetBuildGraphics(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>                                 _("video type not supported by libxl"));
>                  return -1;
>          }
> -        b_info->video_memkb = def->videos[0]->vram ?
> +        b_info->video_memkb = (def->videos[0]->vram >= min_vram) ?
>                                def->videos[0]->vram :
>                                LIBXL_MEMKB_DEFAULT;
>      } else {
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel