Stefan Bader wrote:
On 16.07.2014 23:05, Jim Fehlig wrote:
While testing this, I noticed that libvirt will set vram to 9216 if not specified. E.g.
# cat test.xml ... <video> <model type='vga'/> </video> ... # virsh define test.xml # virsh dumpxml test ... <video> <model type='vga' vram='9216' heads='1'/> </video> ...
With type='vga', libxl will then fail to start the domain
libxl: error: libxl_create.c:253:libxl__domain_build_info_setdefault: videoram must be at least 16 MB for STDVGA on QEMU_XEN
Heh, thats "funny". At least this was the same thing I observed when creating new guests with virt-manager. Maybe I blamed the wrong part of the stack. Or they both do it. This lead the the second part of my changes which was not so clear about whether it should or should not go upstream.
So from my side a fixup of some kind would be good but we can work on this in a follow-up.
The problem is, some domains may not start after applying this patch. Consider a domain with the following video config <video> <model type='vga' vram='9216' heads='1'/> </video> This would work pre-patch since libxl_domain_build_info->video_memkb would be initialized to LIBXL_MEMKB_DEFAULT when calling libxl_domain_build_info_init(). But the domain will fail to start post-patch since video_memkb is set to an invalid value. I know the current behavior is not correct either, but would be nice to fix everything up in one series. I started on a second patch that would validate input or set sane defaults in libxlDomainDeviceDefPostParse(), but while testing realized that sane defaults depend on which qemu is used. E.g. the minimum video_memkb values have doubled with QEMU_XEN vs QEMU_XEN_TRADITIONAL - see $xen_root/tools/libxl/libxl_create.c, libxl__domain_build_info_setdefault(). [I'll mention again that it is unfortunate that QEMU_XEN vs QEMU_XEN_TRADITIONAL leaked through the public API.] I'm thinking of writing a utility function to detect the old vs new qemu, hoping there is something in the help output or similar to determine which one <emulator> is. Such a function would be useful for David Scott's old patch to support arbitrary user-provided <emulator> https://www.redhat.com/archives/libvir-list/2013-April/msg02119.html But alas, this will have to wait until I return from vacation. Regards, Jim