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

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Fri, Apr 04, 2014 at 03:36:38PM +0200, Stefan Bader wrote:
> 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...

I'd suggest, do a first patch which removes the 'default:' case and sets an
error and honours the VRAM size. Then do a second patch which adds the VRAM
workaround, so we can discuss it separately from the main enablement.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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