Re: [Xen-devel] [Qemu-devel] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-02-27 at 14:28 +0800, Chen, Tiejun wrote:
> On 2015/2/27 0:17, Ian Campbell wrote:
> > On Thu, 2015-02-26 at 14:35 +0800, Chen, Tiejun wrote:
> >
> >>> If we are going to do this then I think we need to arrange for the
> >>> interface to be able to express the need to force the workarounds for a
> >>> particular device. IOW a boolean will not suffice since it doesn't
> >>> indicate that IGD workarounds are needed.
> >>>
> >>> Probably it would be simplest to just leave this functionality out for
> >>> the time being and revisit if/when maintaining the list becomes an
> >>> annoyance or an end user trips over it.
> >>>
> >>
> >> You mean we should maintain one list to save all targeted devices, then
> >> tools uses ids as an index to lookup this list to pass something to qemu.
> >
> > I (think I) meant a list of pci vid:did in libxl, which is matched
> > against the devices passed to the domain (e.g. "pci = [...]" in xl cfg),
> > which then enables the igd workarounds, i.e. by passing the option to
> 
> Yeah, this is exactly what I'm understanding.
> 
> > qemu.
> >
> >> But actually one question that I have always been thinking about is, its
> >> really a responsibility of Xen to determine which device type should be
> >> passed by probing that pair of vendor and device ids? Xen is just one of
> >> so many approaches to qemu so such a rare workaround option can be
> >> passed actively by any user, instead of Xen. Furthermore, its becoming
> >> flexible as well to those cases we want to force overriding this.
> >
> > I'm not sure, but I think you are suggestion that qemu should autodetect
> > this situation, without being explicitly told "igd-passthru=on" on the
> > command line?
> >
> > If the qemu maintainers are amenable to that, and it's not already the
> > case that other components (e.g. hvmloader) need to be told about these
> > workarounds, then I suppose that would work.
> >
> >> So I think qemu should mainly plays this role. If qemu realizes we're
> >> passing through a IGD or other targeted device, it should post a warning
> >> or even error message to indicate what right behavior is needed, or what
> >> is that potential risk by default.
> >
> > Hrm, here it sounds more like you are suggesting that qemu should detect
> > and warn, rather than detect and do the right thing?
> >
> > I'm not sure how Qemu could indicate what the right behaviour is going
> > to be, it'll differ for different hypervisors or even for which Xen
> > toolstack (xl vs libvirt etc) is in use.
> >
> > Or maybe I've misunderstood?
> >
> 
> IGD is a tricky case since Qemu has to construct a ISA bridge and host 
> bridge before we pass IGD device. But we don't like to expose these two 
> bridges unconditionally, and this is also why we need this option.
> 
> Here I just mean when Qemu realizes IGD is passed through but without 
> that appropriate option set, Qemu can post something to explicitly 
> notify user that this option is needed in his case. But it may be a lazy 
> idea.

In any case I think the additions of such warnings in qemu are a
separate to the discussion in this thread, so I propose to leave it
alone for now.

> So now I think I'd better go back handling this on Xen side with your 
> comments. As you said the Boolean doesn't suffice to indicate that IGD 
> workarounds are needed. So I think we can reuse that existing bool 
> 'gfx_passthru'.
> 
> Firstly we can redefine this as string,

Unfortunately not since libxl's API guarantee requires older clients to
keep working, i.e. those who use libxl_defbool_set on this field.

Probably the best which can be done is to deprecate this field in favour
of a new one (the old field would need to be obeyed only if the new one
was set to its default value).

Probably an Enumeration would be better than a raw string here as well.

This approach doesn't allow for the possibility of multiple such
workarounds though. It's unclear to me if this matters or not.

The other option which I've mentioned is to leave gfx_passthru and have
libxl figure out which workarounds to enable based on the set of PCI
devices passed through. I guess you don't like that approach? (due to
the need to maintain the pci vid:did list?)

> 
> -                                       ("gfx_passthru",     libxl_defbool),
> +                                       ("gfx_passthru",     string),
> 
> Then
> 
> +
> +        if (libxl__is_igd_vga_passthru(gc, guest_config) ||
> +            (b_info->u.hvm.gfx_passthru &&
> +             strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) {
> +                machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg);
> +        }
> +
> 
> Of course we need modify something else to align this change.
> 
> Thanks
> Tiejun



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