Re: [PATCH v3 RESEND] libxl: Add none to vga parameter

[Anthony PERARD <anthony.perard@citrix.com>]

On Thu, Mar 13, 2014 at 11:57:18AM +0100, Fabio Fantoni wrote:
> Il 11/03/2014 16:09, Paul Durrant ha scritto:
> >>-----Original Message-----
> >>From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> >>bounces@lists.xen.org] On Behalf Of Fabio Fantoni
> >>Sent: 22 February 2014 10:37
> >>To: xen-devel@lists.xensource.com
> >>Cc: Anthony Perard; Fabio Fantoni; Ian Jackson; Ian Campbell; Stefano
> >>Stabellini
> >>Subject: [Xen-devel] [PATCH v3 RESEND] libxl: Add none to vga parameter
> >>
> >>Usage:
> >>   vga="none"
> >>
> >>Make possible to not have an emulated vga on hvm domUs.
> >>
> >>Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> >>
> >>---
> >>
> >>Changes in v3:
> >>- set video_memkb to 0 if vga is none.
> >>- remove a check on one condition no more needed.
> >>
> >>Changes in v2:
> >>- libxl_dm.c:
> >>  if vga is none, on qemu traditional:
> >>   - add -vga none parameter.
> >>   - do not add -videoram parameter.
> >>
> >>---
> >>  docs/man/xl.cfg.pod.5       |    2 +-
> >>  tools/libxl/libxl_create.c  |    6 ++++++
> >>  tools/libxl/libxl_dm.c      |    5 +++++
> >>  tools/libxl/libxl_types.idl |    1 +
> >>  tools/libxl/xl_cmdimpl.c    |    2 ++
> >>  5 files changed, 15 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >>index e15a49f..2f36143 100644
> >>--- a/docs/man/xl.cfg.pod.5
> >>+++ b/docs/man/xl.cfg.pod.5
> >>@@ -1082,7 +1082,7 @@ This option is deprecated, use vga="stdvga"
> >>instead.
> >>
> >>  =item B<vga="STRING">
> >>
> >>-Selects the emulated video card (stdvga|cirrus).
> >>+Selects the emulated video card (none|stdvga|cirrus).
> >>  The default is cirrus.
> >>
> >>  =item B<vnc=BOOLEAN>
> >>diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >>index a604cd8..9110394 100644
> >>--- a/tools/libxl/libxl_create.c
> >>+++ b/tools/libxl/libxl_create.c
> >>@@ -226,6 +226,9 @@ int libxl__domain_build_info_setdefault(libxl__gc
> >>*gc,
> >>          switch (b_info->device_model_version) {
> >>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> >>              switch (b_info->u.hvm.vga.kind) {
> >>+            case LIBXL_VGA_INTERFACE_TYPE_NONE:
> >>+                b_info->video_memkb = 0;
> >I've just been testing this patch in conjunction with my secondary emulator series and this zeroing of the video memory is problematic. I've implemented a secondary console emulator and so still need a vram allocation, so can we still allow a specified videoram value even if the default is zero?
> >
> >   Paul
> 
> If I remember correctly video_memkb is set with xl parameter "videoram"
> before setdefault of libxl_create.c, therefore the video_memkb will be 0
> anyway.
> I added zeroing of video_memkb after advice of anthony perard:
> http://lists.xen.org/archives/html/xen-devel/2013-11/msg03692.html
> The patch v2 didn't set videoram to zero and it works. If zeroing of
> videoram is not needed for something else we could just revert the change
> made in v3, otherwise we need to make "videoram" xl parameter work despite
> the zeroing.
> Can Anthony perard or someone else tell me if it is necessary to set to zero
> the videoram if an emulated video card is not present?

Fabio,
I think it should default to 0, yes.
But, like the other cases, set 0 only if
(b_info->video_memkb == LIBXL_MEMKB_DEFAULT), that should be better. And
in this care, "videoram" config option will be taking into account by
xl.

Paul,
"vga=none" will mean no graphic card, so no vram allocation. If
your secondary console emulator does not allocate memory for it self,
then nothing will. I'm not sure that setting "videoram=X" will help, but
it should be taking into account by xl.


Hope that helps.

> 
> >
> >>+                break;
> >>              case LIBXL_VGA_INTERFACE_TYPE_STD:
> >>                  if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> >>                      b_info->video_memkb = 8 * 1024;
> >>@@ -246,6 +249,9 @@ int libxl__domain_build_info_setdefault(libxl__gc
> >>*gc,
> >>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> >>          default:
> >>              switch (b_info->u.hvm.vga.kind) {
> >>+            case LIBXL_VGA_INTERFACE_TYPE_NONE:
> >>+                b_info->video_memkb = 0;
> >>+                break;
> >>              case LIBXL_VGA_INTERFACE_TYPE_STD:
> >>                  if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
> >>                      b_info->video_memkb = 16 * 1024;
> >>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >>index f6f7bbd..761bb61 100644

-- 
Anthony PERARD