[PATCH] tools/libxl: Improve videoram setting

[PATCH] tools/libxl: Improve videoram setting

[Fabio Fantoni]

[-- Attachment #1.1: Type: text/plain, Size: 3010 bytes --]

tools/libxl: Improve videoram setting

- The minimal videoram setting must be 8 mb.
- Added videoram setting for qemu upstream with cirrus (added in qemu 1.3).
- Updated xl.cfg man.

Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it>
---
  docs/man/xl.cfg.pod.5    |   14 +++++---------
  tools/libxl/libxl_dm.c   |    7 +++++++
  tools/libxl/xl_cmdimpl.c |    3 ++-
  3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index caba162..9c5cdcd 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -974,19 +974,15 @@ in the B<VFB_SPEC_STRING> for configuring virtual 
frame buffer devices

  Sets the amount of RAM which the emulated video card will contain,
  which in turn limits the resolutions and bit depths which will be
-available. This option is only available when using the B<stdvga>
-option (see below).
+available.
  The default amount of video ram for stdvga is 8MB which is sufficient
-for e.g. 1600x1200 at 32bpp.
+for e.g. 1600x1200 at 32bpp and videoram option is currently working
+only when using the qemu-xen-traditional device-model.

  When using the emulated Cirrus graphics card (B<stdvga=0>)
  the amount of video ram is fixed at 4MB which is sufficient
-for 1024x768 at 32 bpp.
-
-videoram option is currently only available when using the
-qemu-xen-traditional device-model. Upstream qemu-xen device-model
-currently does not support changing the amount of video memory for the
-emulated graphics device.
+for 1024x768 at 32 bpp and videoram option is currently working
+only when using the upstream qemu-xen device-model.

  =item B<stdvga=BOOLEAN>

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c036dc1..e3f5ab9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -430,6 +430,13 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
              break;
          case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
              flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
+            if (b_info->video_memkb) {
+                flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
+                flexarray_vappend(dm_args, "-global",
+                libxl__sprintf(gc, "vga.vram_size_mb=%d",
+                libxl__sizekb_to_mb(b_info->video_memkb)),
+                NULL);
+            }
              break;
          }

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e964bf1..2bfbcf4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -832,7 +832,8 @@ static void parse_config_data(const char *config_source,
      xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);

      if (!xlu_cfg_get_long (config, "videoram", &l, 0))
-        b_info->video_memkb = l * 1024;
+        if ( l > 8 )
+            b_info->video_memkb = l * 1024;

      switch(b_info->type) {
      case LIBXL_DOMAIN_TYPE_HVM:
-- 
1.7.9.5



[-- Attachment #1.2: Firma crittografica S/MIME --]
[-- Type: application/pkcs7-signature, Size: 4510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] tools/libxl: Improve videoram setting

[Stefano Stabellini]

On Thu, 10 Jan 2013, Fabio Fantoni wrote:
> tools/libxl: Improve videoram setting
> 
> - The minimal videoram setting must be 8 mb.
> - Added videoram setting for qemu upstream with cirrus (added in qemu 1.3).
> - Updated xl.cfg man.
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@heliman.it>
> ---
>  docs/man/xl.cfg.pod.5    |   14 +++++---------
>  tools/libxl/libxl_dm.c   |    7 +++++++
>  tools/libxl/xl_cmdimpl.c |    3 ++-
>  3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index caba162..9c5cdcd 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -974,19 +974,15 @@ in the B<VFB_SPEC_STRING> for configuring virtual frame
> buffer devices
> 
>  Sets the amount of RAM which the emulated video card will contain,
>  which in turn limits the resolutions and bit depths which will be
> -available. This option is only available when using the B<stdvga>
> -option (see below).
> +available.
>  The default amount of video ram for stdvga is 8MB which is sufficient
> -for e.g. 1600x1200 at 32bpp.
> +for e.g. 1600x1200 at 32bpp and videoram option is currently working
> +only when using the qemu-xen-traditional device-model.
> 
>  When using the emulated Cirrus graphics card (B<stdvga=0>)
>  the amount of video ram is fixed at 4MB which is sufficient
> -for 1024x768 at 32 bpp.
> -
> -videoram option is currently only available when using the
> -qemu-xen-traditional device-model. Upstream qemu-xen device-model
> -currently does not support changing the amount of video memory for the
> -emulated graphics device.
> +for 1024x768 at 32 bpp and videoram option is currently working
> +only when using the upstream qemu-xen device-model.
> 
>  =item B<stdvga=BOOLEAN>

Since you are already doing this for cirrus, you might as well add the
possibility of setting the videoram size for stdvga too.


> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c036dc1..e3f5ab9 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -430,6 +430,13 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
>              break;
>          case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
>              flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
> +            if (b_info->video_memkb) {
> +                flexarray_vappend(dm_args, "-vga", "cirrus", NULL);
> +                flexarray_vappend(dm_args, "-global",
> +                libxl__sprintf(gc, "vga.vram_size_mb=%d",
> +                libxl__sizekb_to_mb(b_info->video_memkb)),
> +                NULL);
> +            }
>              break;
>          }
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e964bf1..2bfbcf4 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -832,7 +832,8 @@ static void parse_config_data(const char *config_source,
>      xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);
> 
>      if (!xlu_cfg_get_long (config, "videoram", &l, 0))
> -        b_info->video_memkb = l * 1024;
> +        if ( l > 8 )
> +            b_info->video_memkb = l * 1024;

Limiting the minimum amount of videoram could be a good idea, but the
minimum should be 4MB, not 8MB

Re: [PATCH] tools/libxl: Improve videoram setting

[Ian Campbell]

On Thu, 2013-01-10 at 14:34 +0000, Stefano Stabellini wrote:
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index e964bf1..2bfbcf4 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -832,7 +832,8 @@ static void parse_config_data(const char *config_source,
> >      xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);
> > 
> >      if (!xlu_cfg_get_long (config, "videoram", &l, 0))
> > -        b_info->video_memkb = l * 1024;
> > +        if ( l > 8 )
> > +            b_info->video_memkb = l * 1024;
> 
> Limiting the minimum amount of videoram could be a good idea, but the
> minimum should be 4MB, not 8MB

I'm not convinced by silently limiting the memory, this deserve at least
a WARNING if not an outright failure.

Also, does this sort of limit checking make more sense in libxl?

Ian.

Re: [PATCH] tools/libxl: Improve videoram setting

[Stefano Stabellini]

On Thu, 10 Jan 2013, Ian Campbell wrote:
> On Thu, 2013-01-10 at 14:34 +0000, Stefano Stabellini wrote:
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index e964bf1..2bfbcf4 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -832,7 +832,8 @@ static void parse_config_data(const char *config_source,
> > >      xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);
> > > 
> > >      if (!xlu_cfg_get_long (config, "videoram", &l, 0))
> > > -        b_info->video_memkb = l * 1024;
> > > +        if ( l > 8 )
> > > +            b_info->video_memkb = l * 1024;
> > 
> > Limiting the minimum amount of videoram could be a good idea, but the
> > minimum should be 4MB, not 8MB
> 
> I'm not convinced by silently limiting the memory, this deserve at least
> a WARNING if not an outright failure.
> 
> Also, does this sort of limit checking make more sense in libxl?

Yes, you are right on both comments

Re: [PATCH] tools/libxl: Improve videoram setting

[Fabio Fantoni]

[-- Attachment #1.1: Type: text/plain, Size: 1690 bytes --]

Il 10/01/2013 15:40, Stefano Stabellini ha scritto:
> On Thu, 10 Jan 2013, Ian Campbell wrote:
>> On Thu, 2013-01-10 at 14:34 +0000, Stefano Stabellini wrote:
>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>>>> index e964bf1..2bfbcf4 100644
>>>> --- a/tools/libxl/xl_cmdimpl.c
>>>> +++ b/tools/libxl/xl_cmdimpl.c
>>>> @@ -832,7 +832,8 @@ static void parse_config_data(const char *config_source,
>>>>       xlu_cfg_get_defbool(config, "localtime", &b_info->localtime, 0);
>>>>
>>>>       if (!xlu_cfg_get_long (config, "videoram", &l, 0))
>>>> -        b_info->video_memkb = l * 1024;
>>>> +        if ( l > 8 )
>>>> +            b_info->video_memkb = l * 1024;
>>> Limiting the minimum amount of videoram could be a good idea, but the
>>> minimum should be 4MB, not 8MB
>> I'm not convinced by silently limiting the memory, this deserve at least
>> a WARNING if not an outright failure.
>>
>> Also, does this sort of limit checking make more sense in libxl?
> Yes, you are right on both comments
>
>
> -----
> Nessun virus nel messaggio.
> Controllato da AVG - www.avg.com
> Versione: 2013.0.2805 / Database dei virus: 2637/6021 -  Data di rilascio: 09/01/2013
>
>
About limiting to 8 mb minimum it seems to be necessary on upstream qemu 
otherwise qemu crash on start, besides libxl already has a default of 8 
mb if videoram is not set.
I'll add libxl error if videoram is < 8, can it be good?
About videoram setting, I didn't find the setting for stdvga in the code.
Here for example the commit for cirrus:
http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=commit;h=19403a68fb8eaefb2e1245b6a8384d3a3ffa7ca0


[-- Attachment #1.2: Firma crittografica S/MIME --]
[-- Type: application/pkcs7-signature, Size: 4510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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