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

[Fabio Fantoni <fantonifabio@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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: smime.p7s
Description: Firma crittografica S/MIME

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