| Message ID | 54DAC247.6080004@intel.com |
|---|---|
| State | New |
| Headers | show |
Ian, Just ping this, or do you think I should send this as a patch? Thanks Tiejun On 2015/2/11 10:45, Chen, Tiejun wrote: > On 2015/2/9 19:05, Ian Campbell wrote: >> On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote: >> >>> What about this? >> >> I've not read the code in detail,since I'm travelling but from a quick >> glance it looks to be implementing the sort of thing I meant, thanks. > > Thanks for your time. > >> >> A couple of higher level comments: >> >> I'd suggest to put the code for reading the vid/did into a helper >> function so it can be reused. > > Looks good. > >> >> You might like to optionally consider add a forcing option somehow so >> that people with new devices not in the list can control things without >> the need to recompile (e.g. gfx_passthru_kind_override?). Perhaps that >> isn't needed for a first cut though and it would be a libxl API so >> thought required. > > What about 'gfx_passthru_force'? Because what we're doing is, we want to > make sure if we have a such a IGD that needs to workaround by posting a > parameter to qemu. So in case of non-listed devices we just need to > provide a bool to force this regardless of that real device. > >> >> I think it should probably log something at a lowish level when it has >> autodetected IGD. >> > > So I tried to rebase that according to your all comments, > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 98687bd..398d9da 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -361,6 +361,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > libxl_defbool_setdefault(&b_info->u.hvm.nographic, false); > > libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false); > + libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru_force, > false); > > break; > case LIBXL_DOMAIN_TYPE_PV: > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 8599a6a..507034f 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -710,9 +710,6 @@ static char ** > libxl__build_device_model_args_new(libxl__gc *gc, > flexarray_append(dm_args, "-net"); > flexarray_append(dm_args, "none"); > } > - if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { > - flexarray_append(dm_args, "-gfx_passthru"); > - } > } else { > if (!sdl && !vnc) { > flexarray_append(dm_args, "-nographic"); > @@ -757,6 +754,11 @@ static char ** > libxl__build_device_model_args_new(libxl__gc *gc, > machinearg, > max_ram_below_4g); > } > } > + > + if (libxl__is_igd_vga_passthru(gc, guest_config)) { > + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); > + } > + > flexarray_append(dm_args, machinearg); > for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; > i++) > flexarray_append(dm_args, b_info->extra_hvm[i]); > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 934465a..35ec5fc 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc > *gc, uint32_t domid, > libxl_device_pci *pcidev, int num); > _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid); > > +_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc, > + const libxl_domain_config > *d_config); > + > /*----- xswait: wait for a xenstore node to be suitable -----*/ > > typedef struct libxl__xswait_state libxl__xswait_state; > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > index f3ae132..07b9e22 100644 > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc, > libxl_device_pci *pcidev, > return 0; > } > > +static unsigned long sysfs_dev_get_vendor(libxl__gc *gc, > + libxl_device_pci *pcidev) > +{ > + char *pci_device_vendor_path = > + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor", > + pcidev->domain, pcidev->bus, pcidev->dev, > + pcidev->func); > + int read_items; > + unsigned long pci_device_vendor; > + > + FILE *f = fopen(pci_device_vendor_path, "r"); > + if (!f) { > + LOGE(ERROR, > + "pci device "PCI_BDF" does not have vendor attribute", > + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); > + return 0xffff; > + } > + read_items = fscanf(f, "0x%lx\n", &pci_device_vendor); > + fclose(f); > + if (read_items != 1) { > + LOGE(ERROR, > + "cannot read vendor of pci device "PCI_BDF, > + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); > + return 0xffff; > + } > + > + return pci_device_vendor; > +} > + > +static unsigned long sysfs_dev_get_device(libxl__gc *gc, > + libxl_device_pci *pcidev) > +{ > + char *pci_device_device_path = > + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/device", > + pcidev->domain, pcidev->bus, pcidev->dev, > + pcidev->func); > + int read_items; > + unsigned long pci_device_device; > + > + FILE *f = fopen(pci_device_device_path, "r"); > + if (!f) { > + LOGE(ERROR, > + "pci device "PCI_BDF" does not have device attribute", > + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); > + return 0xffff; > + } > + read_items = fscanf(f, "0x%lx\n", &pci_device_device); > + fclose(f); > + if (read_items != 1) { > + LOGE(ERROR, > + "cannot read device of pci device "PCI_BDF, > + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); > + return 0xffff; > + } > + > + return pci_device_device; > +} > + > +static const uint16_t igd_ids[] = { > + /* HSW Classic */ > + 0x0402, /* HSWGT1D, HSWD_w7 */ > + 0x0406, /* HSWGT1M, HSWM_w7 */ > + 0x0412, /* HSWGT2D, HSWD_w7 */ > + 0x0416, /* HSWGT2M, HSWM_w7 */ > + 0x041E, /* HSWGT15D, HSWD_w7 */ > + /* HSW ULT */ > + 0x0A06, /* HSWGT1UT, HSWM_w7 */ > + 0x0A16, /* HSWGT2UT, HSWM_w7 */ > + 0x0A26, /* HSWGT3UT, HSWM_w7 */ > + 0x0A2E, /* HSWGT3UT28W, HSWM_w7 */ > + 0x0A1E, /* HSWGT2UX, HSWM_w7 */ > + 0x0A0E, /* HSWGT1ULX, HSWM_w7 */ > + /* HSW CRW */ > + 0x0D26, /* HSWGT3CW, HSWM_w7 */ > + 0x0D22, /* HSWGT3CWDT, HSWD_w7 */ > + /* HSW Sever */ > + 0x041A, /* HSWSVGT2, HSWD_w7 */ > + /* HSW SRVR */ > + 0x040A, /* HSWSVGT1, HSWD_w7 */ > + /* BSW */ > + 0x1606, /* BDWULTGT1, BDWM_w7 */ > + 0x1616, /* BDWULTGT2, BDWM_w7 */ > + 0x1626, /* BDWULTGT3, BDWM_w7 */ > + 0x160E, /* BDWULXGT1, BDWM_w7 */ > + 0x161E, /* BDWULXGT2, BDWM_w7 */ > + 0x1602, /* BDWHALOGT1, BDWM_w7 */ > + 0x1612, /* BDWHALOGT2, BDWM_w7 */ > + 0x1622, /* BDWHALOGT3, BDWM_w7 */ > + 0x162B, /* BDWHALO28W, BDWM_w7 */ > + 0x162A, /* BDWGT3WRKS, BDWM_w7 */ > + 0x162D, /* BDWGT3SRVR, BDWM_w7 */ > +}; > + > +int libxl__is_igd_vga_passthru(libxl__gc *gc, > + const libxl_domain_config *d_config) > +{ > + unsigned int i, j, num = ARRAY_SIZE(igd_ids); > + libxl_ctx *ctx = libxl__gc_owner(gc); > + > + if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru)) > + return 0; > + > + if (libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru_force)) { > + LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "Force to workaround IGD."); > + return 1; > + } > + > + for (i = 0 ; i < d_config->num_pcidevs ; i++) { > + libxl_device_pci *pcidev = &d_config->pcidevs[i]; > + > + if (sysfs_dev_get_vendor(gc, pcidev) != 0x8086) /* Intel class */ > + continue; > + > + for (j = 0 ; j < num ; j++) { > + if (sysfs_dev_get_device(gc, pcidev) == igd_ids[j]) { /* > IGD */ > + LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Detected supported > IGD."); > + return 1; > + } > + } > + } > + > + return 0; > +} > + > /* > * A brief comment about slots. I don't know what slots are for; > however, > * I have by experimentation determined: > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 02be466..d3015bc 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -430,6 +430,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > ("spice", libxl_spice_info), > > ("gfx_passthru", libxl_defbool), > + ("gfx_passthru_force", > libxl_defbool), > > ("serial", string), > ("boot", string), > > Thanks > Tiejun > > >
On Wed, 2015-02-11 at 10:45 +0800, Chen, Tiejun wrote: > On 2015/2/9 19:05, Ian Campbell wrote: > > On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote: > > > >> What about this? > > > > I've not read the code in detail,since I'm travelling but from a quick > > glance it looks to be implementing the sort of thing I meant, thanks. > > Thanks for your time. > > > > > A couple of higher level comments: > > > > I'd suggest to put the code for reading the vid/did into a helper > > function so it can be reused. > > Looks good. > > > > > You might like to optionally consider add a forcing option somehow so > > that people with new devices not in the list can control things without > > the need to recompile (e.g. gfx_passthru_kind_override?). Perhaps that > > isn't needed for a first cut though and it would be a libxl API so > > thought required. > > What about 'gfx_passthru_force'? Because what we're doing is, we want to > make sure if we have a such a IGD that needs to workaround by posting a > parameter to qemu. So in case of non-listed devices we just need to > provide a bool to force this regardless of that real device. 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. Ian.
On 2015/2/18 21:22, Ian Campbell wrote: > On Wed, 2015-02-11 at 10:45 +0800, Chen, Tiejun wrote: >> On 2015/2/9 19:05, Ian Campbell wrote: >>> On Mon, 2015-02-09 at 14:28 +0800, Chen, Tiejun wrote: >>> >>>> What about this? >>> >>> I've not read the code in detail,since I'm travelling but from a quick >>> glance it looks to be implementing the sort of thing I meant, thanks. >> >> Thanks for your time. >> >>> >>> A couple of higher level comments: >>> >>> I'd suggest to put the code for reading the vid/did into a helper >>> function so it can be reused. >> >> Looks good. >> >>> >>> You might like to optionally consider add a forcing option somehow so >>> that people with new devices not in the list can control things without >>> the need to recompile (e.g. gfx_passthru_kind_override?). Perhaps that >>> isn't needed for a first cut though and it would be a libxl API so >>> thought required. >> >> What about 'gfx_passthru_force'? Because what we're doing is, we want to >> make sure if we have a such a IGD that needs to workaround by posting a >> parameter to qemu. So in case of non-listed devices we just need to >> provide a bool to force this regardless of that real device. > Sorry for this delay response due to our routine Chinese New Year. > 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. 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. 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. Thanks Tiejun
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 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? Ian.
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. 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, - ("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
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
>> 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. Okay. > >> 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?) No, I like that approach currently :) Please see the below, > >> >> - ("gfx_passthru", libxl_defbool), >> + ("gfx_passthru", string), >> >> Then >> >> + >> + if (libxl__is_igd_vga_passthru(gc, guest_config) || This is just working out that way that I already posted previously, and here I paste this code fragment again, +static const pci_info fixup_ids[] = { + /* Intel HSW Classic */ + {0x8086, 0x0402}, /* HSWGT1D, HSWD_w7 */ + {0x8086, 0x0406}, /* HSWGT1M, HSWM_w7 */ + {0x8086, 0x0412}, /* HSWGT2D, HSWD_w7 */ + {0x8086, 0x0416}, /* HSWGT2M, HSWM_w7 */ + {0x8086, 0x041E}, /* HSWGT15D, HSWD_w7 */ + /* Intel HSW ULT */ + {0x8086, 0x0A06}, /* HSWGT1UT, HSWM_w7 */ + {0x8086, 0x0A16}, /* HSWGT2UT, HSWM_w7 */ + {0x8086, 0x0A26}, /* HSWGT3UT, HSWM_w7 */ + {0x8086, 0x0A2E}, /* HSWGT3UT28W, HSWM_w7 */ + {0x8086, 0x0A1E}, /* HSWGT2UX, HSWM_w7 */ + {0x8086, 0x0A0E}, /* HSWGT1ULX, HSWM_w7 */ + /* Intel HSW CRW */ + {0x8086, 0x0D26}, /* HSWGT3CW, HSWM_w7 */ + {0x8086, 0x0D22}, /* HSWGT3CWDT, HSWD_w7 */ + /* Intel HSW Server */ + {0x8086, 0x041A}, /* HSWSVGT2, HSWD_w7 */ + /* Intel HSW SRVR */ + {0x8086, 0x040A}, /* HSWSVGT1, HSWD_w7 */ + /* Intel BSW */ + {0x8086, 0x1606}, /* BDWULTGT1, BDWM_w7 */ + {0x8086, 0x1616}, /* BDWULTGT2, BDWM_w7 */ + {0x8086, 0x1626}, /* BDWULTGT3, BDWM_w7 */ + {0x8086, 0x160E}, /* BDWULXGT1, BDWM_w7 */ + {0x8086, 0x161E}, /* BDWULXGT2, BDWM_w7 */ + {0x8086, 0x1602}, /* BDWHALOGT1, BDWM_w7 */ + {0x8086, 0x1612}, /* BDWHALOGT2, BDWM_w7 */ + {0x8086, 0x1622}, /* BDWHALOGT3, BDWM_w7 */ + {0x8086, 0x162B}, /* BDWHALO28W, BDWM_w7 */ + {0x8086, 0x162A}, /* BDWGT3WRKS, BDWM_w7 */ + {0x8086, 0x162D}, /* BDWGT3SRVR, BDWM_w7 */ +}; + +/* + * Some devices may need some ways to work well. Here like IGD, + * we have to pass a specific option to qemu. + */ +int libxl__is_igd_vga_passthru(libxl__gc *gc, + const libxl_domain_config *d_config) +{ + unsigned int i, j, num = ARRAY_SIZE(fixup_ids); + uint16_t vendor, device; + + for (i = 0 ; i < d_config->num_pcidevs ; i++) { + libxl_device_pci *pcidev = &d_config->pcidevs[i]; + + for (j = 0 ; j < num ; j++) { + vendor = fixup_ids[j].vendor; + device = fixup_ids[j].device; + + if (sysfs_dev_get_vendor(gc, pcidev) == vendor && + sysfs_dev_get_device(gc, pcidev) == device) + return 1; + } + } + + return 0; +} + Is this expected? >> + (b_info->u.hvm.gfx_passthru && >> + strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) { But as you mentioned previously, " You might like to optionally consider add a forcing option somehow so that people with new devices not in the list can control things without the need to recompile (e.g. gfx_passthru_kind_override?). " Here I was trying to convert "gfx_passthru" to address this thing. According to your comment right now, you prefer we should introduce a new field instead of the original 'gfx_passthru' to enumerate such a type. So what about this? libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [ (0, "unknown"), (1, "igd"), ]) Then if we want to override this, just submit the following line into .cfg: gfx_passthru_kind_override = "igd" Thanks Tiejun >> + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); >> + } >> + >> >> Of course we need modify something else to align this change. >> >> Thanks >> Tiejun > > >
Campbell, Are you free to look at my reply? Thanks Tiejun On 2015/3/2 9:20, Chen, Tiejun wrote: >>> 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. > > Okay. > >> >>> 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?) > > No, I like that approach currently :) Please see the below, > >> >>> >>> - ("gfx_passthru", >>> libxl_defbool), >>> + ("gfx_passthru", string), >>> >>> Then >>> >>> + >>> + if (libxl__is_igd_vga_passthru(gc, guest_config) || > > This is just working out that way that I already posted previously, and > here I paste this code fragment again, > > +static const pci_info fixup_ids[] = { > + /* Intel HSW Classic */ > + {0x8086, 0x0402}, /* HSWGT1D, HSWD_w7 */ > + {0x8086, 0x0406}, /* HSWGT1M, HSWM_w7 */ > + {0x8086, 0x0412}, /* HSWGT2D, HSWD_w7 */ > + {0x8086, 0x0416}, /* HSWGT2M, HSWM_w7 */ > + {0x8086, 0x041E}, /* HSWGT15D, HSWD_w7 */ > + /* Intel HSW ULT */ > + {0x8086, 0x0A06}, /* HSWGT1UT, HSWM_w7 */ > + {0x8086, 0x0A16}, /* HSWGT2UT, HSWM_w7 */ > + {0x8086, 0x0A26}, /* HSWGT3UT, HSWM_w7 */ > + {0x8086, 0x0A2E}, /* HSWGT3UT28W, HSWM_w7 */ > + {0x8086, 0x0A1E}, /* HSWGT2UX, HSWM_w7 */ > + {0x8086, 0x0A0E}, /* HSWGT1ULX, HSWM_w7 */ > + /* Intel HSW CRW */ > + {0x8086, 0x0D26}, /* HSWGT3CW, HSWM_w7 */ > + {0x8086, 0x0D22}, /* HSWGT3CWDT, HSWD_w7 */ > + /* Intel HSW Server */ > + {0x8086, 0x041A}, /* HSWSVGT2, HSWD_w7 */ > + /* Intel HSW SRVR */ > + {0x8086, 0x040A}, /* HSWSVGT1, HSWD_w7 */ > + /* Intel BSW */ > + {0x8086, 0x1606}, /* BDWULTGT1, BDWM_w7 */ > + {0x8086, 0x1616}, /* BDWULTGT2, BDWM_w7 */ > + {0x8086, 0x1626}, /* BDWULTGT3, BDWM_w7 */ > + {0x8086, 0x160E}, /* BDWULXGT1, BDWM_w7 */ > + {0x8086, 0x161E}, /* BDWULXGT2, BDWM_w7 */ > + {0x8086, 0x1602}, /* BDWHALOGT1, BDWM_w7 */ > + {0x8086, 0x1612}, /* BDWHALOGT2, BDWM_w7 */ > + {0x8086, 0x1622}, /* BDWHALOGT3, BDWM_w7 */ > + {0x8086, 0x162B}, /* BDWHALO28W, BDWM_w7 */ > + {0x8086, 0x162A}, /* BDWGT3WRKS, BDWM_w7 */ > + {0x8086, 0x162D}, /* BDWGT3SRVR, BDWM_w7 */ > +}; > + > +/* > + * Some devices may need some ways to work well. Here like IGD, > + * we have to pass a specific option to qemu. > + */ > +int libxl__is_igd_vga_passthru(libxl__gc *gc, > + const libxl_domain_config *d_config) > +{ > + unsigned int i, j, num = ARRAY_SIZE(fixup_ids); > + uint16_t vendor, device; > + > + for (i = 0 ; i < d_config->num_pcidevs ; i++) { > + libxl_device_pci *pcidev = &d_config->pcidevs[i]; > + > + for (j = 0 ; j < num ; j++) { > + vendor = fixup_ids[j].vendor; > + device = fixup_ids[j].device; > + > + if (sysfs_dev_get_vendor(gc, pcidev) == vendor && > + sysfs_dev_get_device(gc, pcidev) == device) > + return 1; > + } > + } > + > + return 0; > +} > + > > Is this expected? > >>> + (b_info->u.hvm.gfx_passthru && >>> + strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) { > > But as you mentioned previously, > > " > You might like to optionally consider add a forcing option somehow so > that people with new devices not in the list can control things without > the need to recompile (e.g. gfx_passthru_kind_override?). > " > > Here I was trying to convert "gfx_passthru" to address this thing. > According to your comment right now, you prefer we should introduce a > new field instead of the original 'gfx_passthru' to enumerate such a > type. So what about this? > > libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [ > (0, "unknown"), > (1, "igd"), > ]) > > Then if we want to override this, just submit the following line into .cfg: > > gfx_passthru_kind_override = "igd" > > Thanks > Tiejun > >>> + machinearg = GCSPRINTF("%s,igd-passthru=on", >>> machinearg); >>> + } >>> + >>> >>> Of course we need modify something else to align this change. >>> >>> Thanks >>> Tiejun >> >> >> > > >
On Mon, 2015-03-02 at 09:20 +0800, Chen, Tiejun wrote: > Is this expected? Yes. Can you post it as a proper patch please. I suggest you split the basic stuff and the kind override discussed below in to two patches. > >> + (b_info->u.hvm.gfx_passthru && > >> + strncmp(b_info->u.hvm.gfx_passthru, "igd", 3) == 0) ) { > > But as you mentioned previously, > > " > You might like to optionally consider add a forcing option somehow so > that people with new devices not in the list can control things without > the need to recompile (e.g. gfx_passthru_kind_override?). > " > > Here I was trying to convert "gfx_passthru" to address this thing. > According to your comment right now, you prefer we should introduce a > new field instead of the original 'gfx_passthru' to enumerate such a > type. So what about this? > > libxl_gfx_passthru_kind_type = Enumeration("gfx_passthru_kind_type", [ "kind_type" is redundant. I think just "kind" will do. > (0, "unknown"), "default" I think is better, i.e. if gfx_passthru is enabled then do the probing from the PCI ID list thing. > (1, "igd"), > ]) You would then need to add a field of this type next to the gfx_passthru one in b_config, lets say it's called gfx_passthru_kind. > Then if we want to override this, just submit the following line into .cfg: > > gfx_passthru_kind_override = "igd" So, while we cannot change the defbool in the libxl interface we do actually have a little more freedom in the xl cfg parsing because we can detect bool/integer vs string. So I think it should be possible to arrange to support any of gfx_passthru = 0 => sets build_info.u.gfx_passthru to false gfx_passthru = 1 => sets build_info.u.gfx_passthru to false and build_info.u.gfx_passthru_kind to DEFAULT gfx_passthru = "igd" => sets build_info.u.gfx_passthru to false and build_info.u.gfx_passthru_kind to IGD Take a look at how the "timer_mode" option is handled in xl_cmdimpl.c (except none of these variants are deprecated. You should be able to use the libxl_..._from_string enum helper to do the parsing in the latter case. Ian.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 98687bd..398d9da 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -361,6 +361,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, libxl_defbool_setdefault(&b_info->u.hvm.nographic, false); libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru, false); + libxl_defbool_setdefault(&b_info->u.hvm.gfx_passthru_force, false); break; case LIBXL_DOMAIN_TYPE_PV: diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 8599a6a..507034f 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -710,9 +710,6 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, flexarray_append(dm_args, "-net"); flexarray_append(dm_args, "none"); } - if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { - flexarray_append(dm_args, "-gfx_passthru"); - } } else { if (!sdl && !vnc) { flexarray_append(dm_args, "-nographic"); @@ -757,6 +754,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, machinearg, max_ram_below_4g); } } + + if (libxl__is_igd_vga_passthru(gc, guest_config)) { + machinearg = GCSPRINTF("%s,igd-passthru=on", machinearg); + } + flexarray_append(dm_args, machinearg); for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; i++) flexarray_append(dm_args, b_info->extra_hvm[i]); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 934465a..35ec5fc 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1177,6 +1177,9 @@ _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, int num); _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid); +_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc, + const libxl_domain_config *d_config); + /*----- xswait: wait for a xenstore node to be suitable -----*/ typedef struct libxl__xswait_state libxl__xswait_state; diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index f3ae132..07b9e22 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pcidev, return 0; } +static unsigned long sysfs_dev_get_vendor(libxl__gc *gc, + libxl_device_pci *pcidev) +{ + char *pci_device_vendor_path = + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor", + pcidev->domain, pcidev->bus, pcidev->dev, + pcidev->func); + int read_items; + unsigned long pci_device_vendor; + + FILE *f = fopen(pci_device_vendor_path, "r"); + if (!f) { + LOGE(ERROR, + "pci device "PCI_BDF" does not have vendor attribute", + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + return 0xffff; + } + read_items = fscanf(f, "0x%lx\n", &pci_device_vendor); + fclose(f); + if (read_items != 1) { + LOGE(ERROR, + "cannot read vendor of pci device "PCI_BDF, + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + return 0xffff; + } + + return pci_device_vendor; +} + +static unsigned long sysfs_dev_get_device(libxl__gc *gc, + libxl_device_pci *pcidev) +{ + char *pci_device_device_path = + libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/device", + pcidev->domain, pcidev->bus, pcidev->dev, + pcidev->func); + int read_items; + unsigned long pci_device_device; + + FILE *f = fopen(pci_device_device_path, "r"); + if (!f) { + LOGE(ERROR, + "pci device "PCI_BDF" does not have device attribute", + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + return 0xffff; + } + read_items = fscanf(f, "0x%lx\n", &pci_device_device); + fclose(f); + if (read_items != 1) { + LOGE(ERROR, + "cannot read device of pci device "PCI_BDF, + pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func); + return 0xffff; + } + + return pci_device_device; +} + +static const uint16_t igd_ids[] = { + /* HSW Classic */ + 0x0402, /* HSWGT1D, HSWD_w7 */ + 0x0406, /* HSWGT1M, HSWM_w7 */ + 0x0412, /* HSWGT2D, HSWD_w7 */ + 0x0416, /* HSWGT2M, HSWM_w7 */ + 0x041E, /* HSWGT15D, HSWD_w7 */ + /* HSW ULT */ + 0x0A06, /* HSWGT1UT, HSWM_w7 */ + 0x0A16, /* HSWGT2UT, HSWM_w7 */ + 0x0A26, /* HSWGT3UT, HSWM_w7 */ + 0x0A2E, /* HSWGT3UT28W, HSWM_w7 */ + 0x0A1E, /* HSWGT2UX, HSWM_w7 */ + 0x0A0E, /* HSWGT1ULX, HSWM_w7 */ + /* HSW CRW */ + 0x0D26, /* HSWGT3CW, HSWM_w7 */ + 0x0D22, /* HSWGT3CWDT, HSWD_w7 */ + /* HSW Sever */ + 0x041A, /* HSWSVGT2, HSWD_w7 */ + /* HSW SRVR */ + 0x040A, /* HSWSVGT1, HSWD_w7 */ + /* BSW */ + 0x1606, /* BDWULTGT1, BDWM_w7 */ + 0x1616, /* BDWULTGT2, BDWM_w7 */ + 0x1626, /* BDWULTGT3, BDWM_w7 */ + 0x160E, /* BDWULXGT1, BDWM_w7 */ + 0x161E, /* BDWULXGT2, BDWM_w7 */ + 0x1602, /* BDWHALOGT1, BDWM_w7 */ + 0x1612, /* BDWHALOGT2, BDWM_w7 */ + 0x1622, /* BDWHALOGT3, BDWM_w7 */ + 0x162B, /* BDWHALO28W, BDWM_w7 */ + 0x162A, /* BDWGT3WRKS, BDWM_w7 */ + 0x162D, /* BDWGT3SRVR, BDWM_w7 */ +}; + +int libxl__is_igd_vga_passthru(libxl__gc *gc, + const libxl_domain_config *d_config) +{ + unsigned int i, j, num = ARRAY_SIZE(igd_ids); + libxl_ctx *ctx = libxl__gc_owner(gc); + + if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru)) + return 0; + + if (libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru_force)) { + LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "Force to workaround IGD."); + return 1; + } + + for (i = 0 ; i < d_config->num_pcidevs ; i++) { + libxl_device_pci *pcidev = &d_config->pcidevs[i]; + + if (sysfs_dev_get_vendor(gc, pcidev) != 0x8086) /* Intel class */ + continue; + + for (j = 0 ; j < num ; j++) { + if (sysfs_dev_get_device(gc, pcidev) == igd_ids[j]) { /* IGD */ + LIBXL__LOG(ctx, LIBXL__LOG_INFO, "Detected supported IGD."); + return 1; + } + } + } + + return 0; +} + /* * A brief comment about slots. I don't know what slots are for; however, * I have by experimentation determined: diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 02be466..d3015bc 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -430,6 +430,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("spice", libxl_spice_info), ("gfx_passthru", libxl_defbool), + ("gfx_passthru_force", libxl_defbool),