Re: [libvirt] [PATCH v2] libxl: Implement basic video device selection

On 04.04.2014 15:17, Daniel P. Berrange wrote: > On Fri, Apr 04, 2014 at 11:36:39AM +0200, Stefan Bader wrote: >> +static int >> +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config) >> +{ >> + libxl_domain_build_info *b_info = &d_config->b_info; >> + >> + /* >> + * Take the first defined video device (graphics card) to display >> + * on the first graphics device (display). >> + * Right now only type and vram info is used and anything beside >> + * type xen and vga is mapped to cirrus. >> + */ >> + if (def->nvideos) { >> + unsigned int min_vram = 8 * 1024; >> + >> + switch (def->videos[0]->type) { >> + case VIR_DOMAIN_VIDEO_TYPE_VGA: >> + case VIR_DOMAIN_VIDEO_TYPE_XEN: >> + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD; >> + /* >> + * Libxl enforces a minimal VRAM size of 8M when using >> + * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or >> + * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN. >> + * Avoid build failures and go with the minimum if less >> + * is specified. >> + */ >> + switch (b_info->device_model_version) { >> + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: >> + min_vram = 8 * 1024; >> + break; >> + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: >> + default: >> + min_vram = 16 * 1024; >> + } >> + break; >> + default: >> + /* >> + * Ignore any other device type and use Cirrus. Again fix >> + * up the minimal VRAM to what libxl expects. >> + */ > > We shouldn't do that 'default'. For any device type that Xen can't support > we should report VIR_ERR_CONFIG_UNSUPPORTED. Ok, I could remove that. At some point it might be possible to enhance that again. But that requires more careful thinking. At least with device_model_version QEMU_XEN (not QEMU_XEN_TRADITIONAL and not to confuse with device_model which just sets the path ;)) at least in theory there should be the same support as for kvm... Anyway, just loud thinking. Would you also rather want the VRAM fixup to be removed as well? I probably wait a bit more for other feedback and then would do a v3...