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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@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
> > 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;

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