[PATCH v3] tools/libxl: Improve videoram setting

[PATCH v3] tools/libxl: Improve videoram setting

[Fabio Fantoni]

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

tools/libxl: Improve videoram setting

- If videoram setting is less than 8 mb shows error.
- 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   |    6 ++++++
  tools/libxl/xl_cmdimpl.c |   10 ++++++++--
  3 files changed, 19 insertions(+), 11 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..d719130 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -430,6 +430,12 @@ 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, "-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..6e11c8d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -831,8 +831,14 @@ 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 (!xlu_cfg_get_long (config, "videoram", &l, 0)){
+        if ( l < 8 ){
+            fprintf(stderr, "ERROR: videoram must be at least 8 mb\n");
+            exit (1);
+        } else {
+            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 v3] tools/libxl: Improve videoram setting

[Stefano Stabellini]

On Fri, 11 Jan 2013, Fabio Fantoni wrote:
> tools/libxl: Improve videoram setting
> 
> - If videoram setting is less than 8 mb shows error.
> - 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   |    6 ++++++
>  tools/libxl/xl_cmdimpl.c |   10 ++++++++--
>  3 files changed, 19 insertions(+), 11 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..d719130 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -430,6 +430,12 @@ 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, "-global",
> +                libxl__sprintf(gc, "vga.vram_size_mb=%d",
> +                libxl__sizekb_to_mb(b_info->video_memkb)),
> +                NULL);
> +            }
>              break;
>          }

Looking at the QEMU code, it seems to be that there another variable
with the same name to define the videoram size in vga-pci.c:

static Property vga_pci_properties[] = {
    DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),

Is it possible that this code works even for std-vga out of the box?


> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e964bf1..6e11c8d 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -831,8 +831,14 @@ 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 (!xlu_cfg_get_long (config, "videoram", &l, 0)){
> +        if ( l < 8 ){
> +            fprintf(stderr, "ERROR: videoram must be at least 8 mb\n");
> +            exit (1);
> +        } else {
> +            b_info->video_memkb = l * 1024;
> +        }
> +    }

As Ian pointed out, it would be better if you moved the error check in
libxl, probably somewhere towards the beginning of
initiate_domain_create.

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

[Fabio Fantoni]

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

Il 14/01/2013 13:49, Stefano Stabellini ha scritto:
> On Fri, 11 Jan 2013, Fabio Fantoni wrote:
>> tools/libxl: Improve videoram setting
>>
>> - If videoram setting is less than 8 mb shows error.
>> - 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   |    6 ++++++
>>   tools/libxl/xl_cmdimpl.c |   10 ++++++++--
>>   3 files changed, 19 insertions(+), 11 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..d719130 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -430,6 +430,12 @@ 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, "-global",
>> +                libxl__sprintf(gc, "vga.vram_size_mb=%d",
>> +                libxl__sizekb_to_mb(b_info->video_memkb)),
>> +                NULL);
>> +            }
>>               break;
>>           }
> Looking at the QEMU code, it seems to be that there another variable
> with the same name to define the videoram size in vga-pci.c:
>
> static Property vga_pci_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
>
> Is it possible that this code works even for std-vga out of the box?
>
>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index e964bf1..6e11c8d 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -831,8 +831,14 @@ 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 (!xlu_cfg_get_long (config, "videoram", &l, 0)){
>> +        if ( l < 8 ){
>> +            fprintf(stderr, "ERROR: videoram must be at least 8 mb\n");
>> +            exit (1);
>> +        } else {
>> +            b_info->video_memkb = l * 1024;
>> +        }
>> +    }
> As Ian pointed out, it would be better if you moved the error check in
> libxl, probably somewhere towards the beginning of
> initiate_domain_create.
>
>
> -----
> Nessun virus nel messaggio.
> Controllato da AVG - www.avg.com
> Versione: 2013.0.2890 / Database dei virus: 2638/6030 -  Data di rilascio: 13/01/2013
>
>
About std-vga videoram it seems defined at fixed value and I found only 
the cirrus and qxl being custimisable.
For example this is the commit that make cirrus custimisable:
http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=commit;h=19403a68fb8eaefb2e1245b6a8384d3a3ffa7ca0

About error check I added it only in this last version of patch.
Why wouldn't check it near other domU configuration parameters?
This seems more a sanity check to do before initiate starting domU.


[-- 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 v3] tools/libxl: Improve videoram setting

[Stefano Stabellini]

On Mon, 14 Jan 2013, Fabio Fantoni wrote:
> Il 14/01/2013 13:49, Stefano Stabellini ha scritto:
> > On Fri, 11 Jan 2013, Fabio Fantoni wrote:
> > > tools/libxl: Improve videoram setting
> > > 
> > > - If videoram setting is less than 8 mb shows error.
> > > - 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   |    6 ++++++
> > >   tools/libxl/xl_cmdimpl.c |   10 ++++++++--
> > >   3 files changed, 19 insertions(+), 11 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..d719130 100644
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -430,6 +430,12 @@ 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, "-global",
> > > +                libxl__sprintf(gc, "vga.vram_size_mb=%d",
> > > +                libxl__sizekb_to_mb(b_info->video_memkb)),
> > > +                NULL);
> > > +            }
> > >               break;
> > >           }
> > Looking at the QEMU code, it seems to be that there another variable
> > with the same name to define the videoram size in vga-pci.c:
> > 
> > static Property vga_pci_properties[] = {
> >      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> > 
> > Is it possible that this code works even for std-vga out of the box?
> > 
> > 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index e964bf1..6e11c8d 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -831,8 +831,14 @@ 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 (!xlu_cfg_get_long (config, "videoram", &l, 0)){
> > > +        if ( l < 8 ){
> > > +            fprintf(stderr, "ERROR: videoram must be at least 8 mb\n");
> > > +            exit (1);
> > > +        } else {
> > > +            b_info->video_memkb = l * 1024;
> > > +        }
> > > +    }
> > As Ian pointed out, it would be better if you moved the error check in
> > libxl, probably somewhere towards the beginning of
> > initiate_domain_create.
> > 
> > 
> > -----
> > Nessun virus nel messaggio.
> > Controllato da AVG - www.avg.com
> > Versione: 2013.0.2890 / Database dei virus: 2638/6030 -  Data di rilascio:
> > 13/01/2013
> > 
> > 
> About std-vga videoram it seems defined at fixed value and I found only the
> cirrus and qxl being custimisable.
> For example this is the commit that make cirrus custimisable:
> http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=commit;h=19403a68fb8eaefb2e1245b6a8384d3a3ffa7ca0

I did a quick test and it seems that it should be possible to change the
amount of videoram for stdvga too using the same command line option,
however at the moment it just errors out.
Therefore I am OK with this patch only taking care of Cirrus for the moment.


> About error check I added it only in this last version of patch.
> Why wouldn't check it near other domU configuration parameters?
> This seems more a sanity check to do before initiate starting domU.
 
I know that it might seem counterintuitive but we try to keep all the
significant informations about the platform within libxl. So if 8MB is
an important limit of the platform that libxl should know about it,
while xl should stay as dumb as possible.

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

[Fabio Fantoni]

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

Il 14/01/2013 19:21, Stefano Stabellini ha scritto:
> On Mon, 14 Jan 2013, Fabio Fantoni wrote:
>> Il 14/01/2013 13:49, Stefano Stabellini ha scritto:
>>> On Fri, 11 Jan 2013, Fabio Fantoni wrote:
>>>> tools/libxl: Improve videoram setting
>>>>
>>>> - If videoram setting is less than 8 mb shows error.
>>>> - 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   |    6 ++++++
>>>>    tools/libxl/xl_cmdimpl.c |   10 ++++++++--
>>>>    3 files changed, 19 insertions(+), 11 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..d719130 100644
>>>> --- a/tools/libxl/libxl_dm.c
>>>> +++ b/tools/libxl/libxl_dm.c
>>>> @@ -430,6 +430,12 @@ 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, "-global",
>>>> +                libxl__sprintf(gc, "vga.vram_size_mb=%d",
>>>> +                libxl__sizekb_to_mb(b_info->video_memkb)),
>>>> +                NULL);
>>>> +            }
>>>>                break;
>>>>            }
>>> Looking at the QEMU code, it seems to be that there another variable
>>> with the same name to define the videoram size in vga-pci.c:
>>>
>>> static Property vga_pci_properties[] = {
>>>       DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
>>>
>>> Is it possible that this code works even for std-vga out of the box?
>>>
>>>
>>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>>>> index e964bf1..6e11c8d 100644
>>>> --- a/tools/libxl/xl_cmdimpl.c
>>>> +++ b/tools/libxl/xl_cmdimpl.c
>>>> @@ -831,8 +831,14 @@ 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 (!xlu_cfg_get_long (config, "videoram", &l, 0)){
>>>> +        if ( l < 8 ){
>>>> +            fprintf(stderr, "ERROR: videoram must be at least 8 mb\n");
>>>> +            exit (1);
>>>> +        } else {
>>>> +            b_info->video_memkb = l * 1024;
>>>> +        }
>>>> +    }
>>> As Ian pointed out, it would be better if you moved the error check in
>>> libxl, probably somewhere towards the beginning of
>>> initiate_domain_create.
>>>
>>>
>>> -----
>>> Nessun virus nel messaggio.
>>> Controllato da AVG - www.avg.com
>>> Versione: 2013.0.2890 / Database dei virus: 2638/6030 -  Data di rilascio:
>>> 13/01/2013
>>>
>>>
>> About std-vga videoram it seems defined at fixed value and I found only the
>> cirrus and qxl being custimisable.
>> For example this is the commit that make cirrus custimisable:
>> http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=commit;h=19403a68fb8eaefb2e1245b6a8384d3a3ffa7ca0
> I did a quick test and it seems that it should be possible to change the
> amount of videoram for stdvga too using the same command line option,
> however at the moment it just errors out.
> Therefore I am OK with this patch only taking care of Cirrus for the moment.
>
>
>> About error check I added it only in this last version of patch.
>> Why wouldn't check it near other domU configuration parameters?
>> This seems more a sanity check to do before initiate starting domU.
>   
> I know that it might seem counterintuitive but we try to keep all the
> significant informations about the platform within libxl. So if 8MB is
> an important limit of the platform that libxl should know about it,
> while xl should stay as dumb as possible.
>

I tried to move the check within libxl_create.c, at the point where it 
sets video_memkb default value, but I couldn't make it exit in case of 
failure. Also I'm not sure if the check should go there. Can you help me 
to do it please?


[-- 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 v3] tools/libxl: Improve videoram setting

[Stefano Stabellini]

On Tue, 15 Jan 2013, Fabio Fantoni wrote:
> Il 14/01/2013 19:21, Stefano Stabellini ha scritto:
> > On Mon, 14 Jan 2013, Fabio Fantoni wrote:
> > > Il 14/01/2013 13:49, Stefano Stabellini ha scritto:
> > > > On Fri, 11 Jan 2013, Fabio Fantoni wrote:
> > > > > tools/libxl: Improve videoram setting
> > > > > 
> > > > > - If videoram setting is less than 8 mb shows error.
> > > > > - 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   |    6 ++++++
> > > > >    tools/libxl/xl_cmdimpl.c |   10 ++++++++--
> > > > >    3 files changed, 19 insertions(+), 11 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..d719130 100644
> > > > > --- a/tools/libxl/libxl_dm.c
> > > > > +++ b/tools/libxl/libxl_dm.c
> > > > > @@ -430,6 +430,12 @@ 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, "-global",
> > > > > +                libxl__sprintf(gc, "vga.vram_size_mb=%d",
> > > > > +                libxl__sizekb_to_mb(b_info->video_memkb)),
> > > > > +                NULL);
> > > > > +            }
> > > > >                break;
> > > > >            }
> > > > Looking at the QEMU code, it seems to be that there another variable
> > > > with the same name to define the videoram size in vga-pci.c:
> > > > 
> > > > static Property vga_pci_properties[] = {
> > > >       DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb,
> > > > 16),
> > > > 
> > > > Is it possible that this code works even for std-vga out of the box?
> > > > 
> > > > 
> > > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > > > index e964bf1..6e11c8d 100644
> > > > > --- a/tools/libxl/xl_cmdimpl.c
> > > > > +++ b/tools/libxl/xl_cmdimpl.c
> > > > > @@ -831,8 +831,14 @@ 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 (!xlu_cfg_get_long (config, "videoram", &l, 0)){
> > > > > +        if ( l < 8 ){
> > > > > +            fprintf(stderr, "ERROR: videoram must be at least 8
> > > > > mb\n");
> > > > > +            exit (1);
> > > > > +        } else {
> > > > > +            b_info->video_memkb = l * 1024;
> > > > > +        }
> > > > > +    }
> > > > As Ian pointed out, it would be better if you moved the error check in
> > > > libxl, probably somewhere towards the beginning of
> > > > initiate_domain_create.
> > > > 
> > > > 
> > > > -----
> > > > Nessun virus nel messaggio.
> > > > Controllato da AVG - www.avg.com
> > > > Versione: 2013.0.2890 / Database dei virus: 2638/6030 -  Data di
> > > > rilascio:
> > > > 13/01/2013
> > > > 
> > > > 
> > > About std-vga videoram it seems defined at fixed value and I found only
> > > the
> > > cirrus and qxl being custimisable.
> > > For example this is the commit that make cirrus custimisable:
> > > http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=commit;h=19403a68fb8eaefb2e1245b6a8384d3a3ffa7ca0
> > I did a quick test and it seems that it should be possible to change the
> > amount of videoram for stdvga too using the same command line option,
> > however at the moment it just errors out.
> > Therefore I am OK with this patch only taking care of Cirrus for the moment.
> > 
> > 
> > > About error check I added it only in this last version of patch.
> > > Why wouldn't check it near other domU configuration parameters?
> > > This seems more a sanity check to do before initiate starting domU.
> >   I know that it might seem counterintuitive but we try to keep all the
> > significant informations about the platform within libxl. So if 8MB is
> > an important limit of the platform that libxl should know about it,
> > while xl should stay as dumb as possible.
> > 
> 
> I tried to move the check within libxl_create.c, at the point where it sets
> video_memkb default value, but I couldn't make it exit in case of failure.
> Also I'm not sure if the check should go there. Can you help me to do it
> please?

Sure.
I was thinking something like this:

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c6daec9..3bf8026e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -628,6 +628,13 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM &&
+        d_config->b_info.video_memkb < 8192) {
+        LOG(ERROR, "videoram must be at least 8MB");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
     for (i = 0; i < d_config->num_disks; i++) {
         ret = libxl__device_disk_setdefault(gc, &d_config->disks[i]);
         if (ret) goto error_out;

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

[Ian Campbell]

> > >   I know that it might seem counterintuitive but we try to keep all the
> > > significant informations about the platform within libxl. So if 8MB is
> > > an important limit of the platform that libxl should know about it,
> > > while xl should stay as dumb as possible.
> > > 
> > 
> > I tried to move the check within libxl_create.c, at the point where it sets
> > video_memkb default value, but I couldn't make it exit in case of failure.
> > Also I'm not sure if the check should go there. Can you help me to do it
> > please?
> 
> Sure.
> I was thinking something like this:
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index c6daec9..3bf8026e 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -628,6 +628,13 @@ static void initiate_domain_create(libxl__egc *egc,

A better place for this check is libxl__domain_build_info_setdefault,
which is more like "set defaults and validate what is already there"
than the name would suggest.

>          goto error_out;
>      }
>  
> +    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM &&
> +        d_config->b_info.video_memkb < 8192) {
> +        LOG(ERROR, "videoram must be at least 8MB");
> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +

Ian.

Re: [Qemu-devel] [PATCH v3] tools/libxl: Improve videoram setting

[Fabio Fantoni]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Il 14/01/2013 19:21, Stefano Stabellini ha scritto:
> I did a quick test and it seems that it should be possible to change 
> the amount of videoram for stdvga too using the same command line 
> option, however at the moment it just errors out. Therefore I am OK 
> with this patch only taking care of Cirrus for the moment. 
I found details about stdvga on qemu upstream:
http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=blob_plain;f=docs/specs/standard-vga.txt;hb=HEAD

It seems that stdvga memory by default is 16 mb, while xen reserves only 
8 mb by default and doen't logs any error.
For cirrus, increasing memory seems to be correct and without error with 
my patch.
WIth both cirrus and stdvga under qemu upstream with xen the performance 
are really poor even if I increase video memory, respect to qemu-only 
and qemu-kvm (without xen).
Qxl is definitely not working under xen and conversely is ok on qemu-kvm 
and qemu-only.

It seem that xen need change and/or fix to have full working emulated 
vga on qemu upstream.
At the moment all emulated vgas have problems with xen that aren't 
present without xen.

The performance differences are noticeable (in some case very big) with 
xen and without xen using resolution > 1024x768.

Probably the first link explain the change/fix necessary in xen about 
vga (probably in hvmloader).
I tried to do that more times failing but unfortunately I do not have 
sufficient knowledge about this.
Can someone help me please?

I think this is important, years ago the minimal resolution used on 
desktop was 1024x768, and no problem with actual vga setting but now 
minimal resolution seems increased to up 1366x768 and many people are 
using even higher resolutions.
http://www.screenresolution.org/year-2013/


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

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

[Fabio Fantoni]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Il 14/01/2013 19:21, Stefano Stabellini ha scritto:
> I did a quick test and it seems that it should be possible to change 
> the amount of videoram for stdvga too using the same command line 
> option, however at the moment it just errors out. Therefore I am OK 
> with this patch only taking care of Cirrus for the moment. 
I found details about stdvga on qemu upstream:
http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=blob_plain;f=docs/specs/standard-vga.txt;hb=HEAD

It seems that stdvga memory by default is 16 mb, while xen reserves only 
8 mb by default and doen't logs any error.
For cirrus, increasing memory seems to be correct and without error with 
my patch.
WIth both cirrus and stdvga under qemu upstream with xen the performance 
are really poor even if I increase video memory, respect to qemu-only 
and qemu-kvm (without xen).
Qxl is definitely not working under xen and conversely is ok on qemu-kvm 
and qemu-only.

It seem that xen need change and/or fix to have full working emulated 
vga on qemu upstream.
At the moment all emulated vgas have problems with xen that aren't 
present without xen.

The performance differences are noticeable (in some case very big) with 
xen and without xen using resolution > 1024x768.

Probably the first link explain the change/fix necessary in xen about 
vga (probably in hvmloader).
I tried to do that more times failing but unfortunately I do not have 
sufficient knowledge about this.
Can someone help me please?

I think this is important, years ago the minimal resolution used on 
desktop was 1024x768, and no problem with actual vga setting but now 
minimal resolution seems increased to up 1366x768 and many people are 
using even higher resolutions.
http://www.screenresolution.org/year-2013/


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

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

[Trolle Selander]

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

Just as a quick note, when I originally added the variable vram option in
"traditional" qemu-xen, it required patches to the vga BIOS as well as to
qemu. I'm not sure that those patches ever made it into upstream vgabios,
though I did post the patches to qemu-devel at one point.

/Trolle


On Fri, Feb 1, 2013 at 6:52 AM, Fabio Fantoni <fantonifabio@tiscali.it>wrote:

> Il 14/01/2013 19:21, Stefano Stabellini ha scritto:
>
>> I did a quick test and it seems that it should be possible to change the
>> amount of videoram for stdvga too using the same command line option,
>> however at the moment it just errors out. Therefore I am OK with this patch
>> only taking care of Cirrus for the moment.
>>
> I found details about stdvga on qemu upstream:
> http://xenbits.xen.org/gitweb/**?p=staging/qemu-upstream-**
> unstable.git;a=blob_plain;f=**docs/specs/standard-vga.txt;**hb=HEAD<http://xenbits.xen.org/gitweb/?p=staging/qemu-upstream-unstable.git;a=blob_plain;f=docs/specs/standard-vga.txt;hb=HEAD>
>
> It seems that stdvga memory by default is 16 mb, while xen reserves only 8
> mb by default and doen't logs any error.
> For cirrus, increasing memory seems to be correct and without error with
> my patch.
> WIth both cirrus and stdvga under qemu upstream with xen the performance
> are really poor even if I increase video memory, respect to qemu-only and
> qemu-kvm (without xen).
> Qxl is definitely not working under xen and conversely is ok on qemu-kvm
> and qemu-only.
>
> It seem that xen need change and/or fix to have full working emulated vga
> on qemu upstream.
> At the moment all emulated vgas have problems with xen that aren't present
> without xen.
>
> The performance differences are noticeable (in some case very big) with
> xen and without xen using resolution > 1024x768.
>
> Probably the first link explain the change/fix necessary in xen about vga
> (probably in hvmloader).
> I tried to do that more times failing but unfortunately I do not have
> sufficient knowledge about this.
> Can someone help me please?
>
> I think this is important, years ago the minimal resolution used on
> desktop was 1024x768, and no problem with actual vga setting but now
> minimal resolution seems increased to up 1366x768 and many people are using
> even higher resolutions.
> http://www.screenresolution.**org/year-2013/<http://www.screenresolution.org/year-2013/>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

[-- Attachment #1.2: Type: text/html, Size: 3262 bytes --]

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

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