Re: [Xen-devel] [PATCH v8] tools/libxl: Add qxl vga interface support for upstream-qemu-xen.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-02-05 at 13:19 +0000, Fabio Fantoni wrote:
> Il 05/02/2013 11:33, Ian Campbell ha scritto:
> > On Mon, 2013-01-28 at 16:03 +0000, Fabio Fantoni wrote:
> >> tools/libxl: Add qxl vga interface support for
> >>    upstream-qemu-xen.
> >>
> >> Usage:
> >>     qxl=1|0
> >>
> >> Changes from v7:
> >> - Fix videoram settings parameters for qemu.
> >>
> >> Signed-off-by: Fabio Fantoni <fabio.fantoni@xxxxxxxxxx>
> >> Signed-off-by: Zhou Peng <zpengxen@xxxxxxxxx>
> > Unfortunately this patch is whitespace damaged in various places.
> 
> I'll try from linux instead windows, I can also add patches as 
> attachment on next messages?

If sending from Linux doesn't work *then* lets try attachments.

> 
> >> ---
> >>    docs/man/xl.cfg.pod.5       |   11 +++++++++++
> >>    tools/libxl/libxl_create.c  |   12 ++++++++++++
> >>    tools/libxl/libxl_dm.c      |   15 +++++++++++++++
> >>    tools/libxl/libxl_types.idl |    1 +
> >>    tools/libxl/xl_cmdimpl.c    |    7 ++++++-
> >>    5 files changed, 45 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >> index 9c5cdcd..a0f0dc3 100644
> >> --- a/docs/man/xl.cfg.pod.5
> >> +++ b/docs/man/xl.cfg.pod.5
> >> @@ -984,6 +984,9 @@ the amount of video ram is fixed at 4MB which is
> >> sufficient
> >>    for 1024x768 at 32 bpp and videoram option is currently working
> >>    only when using the upstream qemu-xen device-model.
> >>
> >> +For B<qxl> vga, the default is both default and minimal 128MB.
> >> +If B<videoram> is set less than 128MB, an error will be triggered.
> >> +
> >>    =item B<stdvga=BOOLEAN>
> >>
> >>    Select a standard VGA card with VBE (VESA BIOS Extensions) as the
> >> @@ -992,6 +995,14 @@ a Cirrus Logic GD5446 VGA card. If your guest
> >> supports VBE 2.0 or
> >>    later (e.g. Windows XP onwards) then you should enable this.
> >>    stdvga supports more video ram and bigger resolutions than Cirrus.
> >>
> >> +=item B<qxl=BOOLEAN>
> > What happens if I give qxl=1 and stdvga=1?
> >
> > Perhaps we should deprecate stdvga and add a new option:
> >     vga = "stdvga|cirrus|qxl"
> > ?
> 
> Yes that should be nice.
> I'll do a patch that remove stdvga option and add vga option.

Please keep the stdvga as a (deprecated) synonym for vga=stdvga, so that
configuration files are forward compatible.

> >> +
> >> +        if (b_info->device_model_version ==
> >> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> >> +            && b_info->u.hvm.vga.kind == LIBXL_VGA_INTERFACE_TYPE_QXL) {
> >> +            if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT) {
> >> +                b_info->video_memkb = (128 * 1024);
> >> +            }else if (b_info->video_memkb < (128 * 1024)) {
> >> +                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> >> +                    "128 Mib videoram is the minimum for qxl default");
> > You can use the LOG() macros to shorten this line (and in other places
> > including you videoram patch too).
> >
> > Should this error out on qemu == traditional and vga == QXL?
> 
> Must I only replace LIBXL__LOG with LOG?

LOG is just a convenience macro. 
        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, ... )
becomes
        LOG(ERROR, ...)

> I'll add error and exit if vga=qxl and qemu is traditional.

Great. Remember that libxl can't exit(2), so it should return an error
which causes xl to exit.




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