Re: [Xen-devel] [PATCH 2/3] tools:libxl: Add qxl vga interface support v2

[ZhouPeng <zpengxen@xxxxxxxxx>]

On Thu, Jun 7, 2012 at 2:08 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Thu, 2012-06-07 at 04:19 +0100, ZhouPeng wrote:
>> On Wed, Jun 6, 2012 at 9:05 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
>> > On Tue, 2012-06-05 at 12:22 +0100, ZhouPeng wrote:
>> >> changeset:   25454:1804a873a64d
>> >> tag:         tip
>> >> user:        Zhou Peng <ailvpeng25@xxxxxxxxx>
>> >> date:        Tue Jun 05 17:56:46 2012 +0800
>> >> files:       tools/libxl/libxl_create.c tools/libxl/libxl_dm.c
>> >> tools/libxl/libxl_types.idl tools/libxl/xl_cmdimpl.c
>> >> description:
>> >> tools:libxl: Add qxl vga interface support.
>> >>
>> >> Usage:
>> >>  qxl=1
>> >>  qxlvram=64
>> >>  qxlram=64
>> >>
>> >> Signed-off-by: Zhou Peng <ailvpeng25@xxxxxxxxx>
>> >
>> > Thanks.
>> >
>> > As previously mentioned this is  4.3 material. Please can you resubmit
>> > once the 4.3 dev cycle opens.
>> ok, thanks.
>> >
>> >> diff -r 7bd08f83a2ce -r 1804a873a64d tools/libxl/libxl_create.c
>> >> --- a/tools/libxl/libxl_create.c      Tue Jun 05 17:39:37 2012 +0800
>> >> +++ b/tools/libxl/libxl_create.c      Tue Jun 05 17:56:46 2012 +0800
>> >> @@ -23,6 +23,32 @@
>> >>  #include <xc_dom.h>
>> >>  #include <xenguest.h>
>> >>
>> >> +/*
>> >> + * For qxl vga interface, the total video mem is determined by
>> >> + * RAM bar and VRAM bar. But it is not simply linearly determined,
>> >> + * get_qxl_ram_size below gives the details.
>> >
>> > From this statement I expected get_qxl_ram_size to have a nice comment
>> > explaining what is going on, but it doesn't have this.
>> >
>> > Can you please explain somewhere how this value is determined (I can see
>> > it is not simple ;-)). Perhaps a link to some QXL/qemu document
>> > discussing these parameters would be helpful too?
>>
>> Sorry, there is not a piece of docs on ram bar and vram bar, and how
>> the total size
>> of qxl video memory is calculated from ram bar and vram bar parameters.
>>
>> But from my digging into qxl's source code and debugging, it works this way.
>>
>> I asked similar question in spice-list and get response from:
>> http://comments.gmane.org/gmane.comp.emulators.spice.devel/9501
>>
>> Any way, I will rich the document if get more info.
>
> OK, thanks.
>
>> >> +
>> >> +    return (vram + ram + 1023) / 1024;
>> >> +}
>> >> +
>> >>  void libxl_domain_config_init(libxl_domain_config *d_config)
>> >>  {
>> >>      memset(d_config, 0, sizeof(*d_config));
>> >> @@ -195,6 +221,25 @@ int libxl__domain_build_info_setdefault(
>> >>
>> >>          if (b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_DEFAULT)
>> >>              b_info->u.hvm.vga.type = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
>> >> +        if (b_info->device_model_version == 
>> >> LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
>> >> +            && b_info->u.hvm.vga.type == LIBXL_VGA_INTERFACE_TYPE_QXL) {
>> >> +            if (b_info->u.hvm.vga.vramkb == LIBXL_MEMKB_DEFAULT)
>> >> +                b_info->u.hvm.vga.vramkb = 64 * 1024;
>> >> +            if (b_info->u.hvm.vga.ramkb == LIBXL_MEMKB_DEFAULT)
>> >> +                b_info->u.hvm.vga.ramkb = 64 * 1024;
>> >> +            uint32_t qxl_ram = get_qxl_ram_size(b_info->u.hvm.vga.vramkb,
>> >> +                                                b_info->u.hvm.vga.ramkb);
>> >> +            /*
>> >> +             * video_memkb is the real size of video memory to assign.
>> >> +             * If video_memkb can't meet the need of qxl, adjust it
>> >> +             * accordingly.
>> >> +             */
>> >> +            if ((b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
>> >> +                || (b_info->video_memkb < qxl_ram)) {
>> >> +                b_info->video_memkb = qxl_ram;
>> >
>> > If video_memkb is != LIBXL_MEMKB_DEFAULT and it is not sufficiently
>> > large then I think the correct thing to do is to error out and return
>> > ERROR_INVAL.
>>
>> Not agreed.
>> This will let user must to set a proper value to meet qxl, but from
>> discussing above, it's difficult for user to give this decision.
>> qxlram and qxlvram parameters are enough  for user to set qxl's video
>> ram (libvirt also use these two parameters).
>
> If the user has asked for a specific amount of video RAM which is not
> sufficient then the correct answer is to log an error (including the
> required minimum) and exit.
>
> You are correct that it is hard to figure out what "enough" RAM is. I
> expect that for that reason almost all users won't pass any of these
> arguments and will just accept the default, which will work just fine.

I think it's reasonable.
Just use the default will make things much more simpler.
> Ian.
>



-- 
Zhou Peng

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