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

2015-03-05 Thread Ian Campbell 
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.

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

2015-03-03 Thread Chen, 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 = GCS

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

2015-03-01 Thread Chen, 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 el

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

2015-02-18 Thread Ian Campbell 
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.

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

2015-02-12 Thread Chen, Tiejun 

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 0x;
+}
+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 0x;
+}
+
+return pci_device_vendor;
+}
+
+static unsigned long sysfs_dev_get_device(libxl__gc *gc,
+  libxl_device_pci *pcidev)
+{
+char *pci_device_device_path =
+

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

2015-02-10 Thread Chen, Tiejun 

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 0x;
+}
+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 0x;
+}
+
+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,

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

2015-02-09 Thread Ian Campbell 
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.

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.

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.

I think it should probably log something at a lowish level when it has
autodetected IGD.

Ian.

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

2015-02-08 Thread Chen, Tiejun 

On 2015/2/6 9:01, Chen, Tiejun wrote:

On 2015/2/5 17:52, Ian Campbell wrote:

On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote:

Indeed this is not something workaround, and I think in any type of VGA
devices, we'd like to diminish this sort of thing gradually, right?

This mightn't come true in real world :)


It's not really something we can control, the h/w guys will do what they
do, including integrating on-board graphics tightly with the N/S-bridges
etc.


Yeah.





I think there are three ways to achieve that:

* Make the libxl/xl option something which is not generic e.g.
  igd_passthru=1
* Add a second option to allow the user to configure the
kind of
  graphics device being passed thru (e.g. gfx_passthru=1,
  passthru_device="igd"), or combine the two by making the
  gfx_passthru option a string instead of a boolean.


It makes more sense but this mean we're going to change that existing
rule in qemu-traditional. But here I guess we shouldn't consider that
case.


qemu-trad is frozen so we wouldn't be adding new features such as
support for new graphics passthru devices, so we can ignore it's
deficiencies in this area and just improve things in the qemu-xen case.
(we may want to add a compat handling for any new option we add to libxl
to translate to -gfx_passthru, but that's about it)


Understood.




* Make libxl detect the type of graphics device somehow and
  therefore automatically determine when gfx_passthru=1 =>
  igd-passthru


This way confounds me all. Can libxl detect the graphics device *before*
we intend to pass a parameter to active qemu?


We know the BDF of all devices which we are going to pass to the guest
and we can observe various properties of that device
via /sys/bus/pci/devices/:B:D:F.


So sounds what you're saying is Xen already have this sort of example in
libxl.








   Currently, we have to set
something as follows,

gfx_passthru=1
pci=["00:02.0"]

This always works for qemu-xen-traditional.

But you should know '00:02.0' doesn't mean we are passing IGD through.


But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID
and other properties) we can unambiguously determine if it is an IGD
device or not, can't we?


Again, like what I said above, I'm not sure if its possible in my case.
If I'm wrong please correct me.


Is my reply above sufficient?


Yes, I can understand what you mean but I need to take close look at
exactly what should be done in libxl :)

In high level, this way may come out as follows,

#1 libxl parse 'pci=[]' to get SBDF
#2 Scan SBDF by accessing sysfs to get vendor/device IDs.
#3 If this pair of IDs is identified to our target device, IGD,
"igd-passthru" would be delivered to qemu.

Right?


What about this?

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..9cccbfe 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -491,6 +491,108 @@ static int sysfs_dev_unbind(libxl__gc *gc, 
libxl_device_pci *pcidev,

 return 0;
 }

+/* Currently we only support HSW and BDW in qemu upstream.*/
+static const uint16_t igd_ids[] = {
+/* HSW Classic */
+0x0402, /* HSWG

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

2015-02-05 Thread Chen, Tiejun 

On 2015/2/5 17:52, Ian Campbell wrote:

On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote:

Indeed this is not something workaround, and I think in any type of VGA
devices, we'd like to diminish this sort of thing gradually, right?

This mightn't come true in real world :)


It's not really something we can control, the h/w guys will do what they
do, including integrating on-board graphics tightly with the N/S-bridges
etc.


Yeah.





I think there are three ways to achieve that:

* Make the libxl/xl option something which is not generic e.g.
  igd_passthru=1
* Add a second option to allow the user to configure the kind of
  graphics device being passed thru (e.g. gfx_passthru=1,
  passthru_device="igd"), or combine the two by making the
  gfx_passthru option a string instead of a boolean.


It makes more sense but this mean we're going to change that existing
rule in qemu-traditional. But here I guess we shouldn't consider that case.


qemu-trad is frozen so we wouldn't be adding new features such as
support for new graphics passthru devices, so we can ignore it's
deficiencies in this area and just improve things in the qemu-xen case.
(we may want to add a compat handling for any new option we add to libxl
to translate to -gfx_passthru, but that's about it)


Understood.




* Make libxl detect the type of graphics device somehow and
  therefore automatically determine when gfx_passthru=1 =>
  igd-passthru


This way confounds me all. Can libxl detect the graphics device *before*
we intend to pass a parameter to active qemu?


We know the BDF of all devices which we are going to pass to the guest
and we can observe various properties of that device
via /sys/bus/pci/devices/:B:D:F.


So sounds what you're saying is Xen already have this sort of example in 
libxl.









   Currently, we have to set
something as follows,

gfx_passthru=1
pci=["00:02.0"]

This always works for qemu-xen-traditional.

But you should know '00:02.0' doesn't mean we are passing IGD through.


But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID
and other properties) we can unambiguously determine if it is an IGD
device or not, can't we?


Again, like what I said above, I'm not sure if its possible in my case.
If I'm wrong please correct me.


Is my reply above sufficient?


Yes, I can understand what you mean but I need to take close look at 
exactly what should be done in libxl :)


In high level, this way may come out as follows,

#1 libxl parse 'pci=[]' to get SBDF
#2 Scan SBDF by accessing sysfs to get vendor/device IDs.
#3 If this pair of IDs is identified to our target device, IGD, 
"igd-passthru" would be delivered to qemu.


Right?




If not then that _might_ suggest we should deprecate the gdx_passthru
option at the libxl interface and switch to something more expressive,
such as an Enumeration of card types (with a singleton of IGD right
now), but I'm not really very familiar with passthru nor the qemu side
of this.

What happens if you try to pass two different GFX cards to a guest?



Are you saying two IGDs?


Yes, or any combination of two cards, perhaps from different vendors
(AIUI some laptops have this with IGD and Nvidia or ATI?).


One IGD and multiple other type of Graphic display cards can coexist.


... but if they both need special handling then we need a way to
communicate that.


   Its not possible since as I said above, IGD is
tricky because it depends on something from ISA bridge and host bridge.
So we can't provide two or more different setting configurations to own
more IGDs just in one platform.


This is because IGD must be a "primary" VGA device? I understand that


No. I mean ISA bridge and host bridge just provide one set of IGD
resource so its difficult to configure two or more IGDs.


there can only be one of those in a system, but I think it is possible
to have multiple secondary VGA devices or different types in one system.



What I'm saying is, its impossible to own two same IGDs in our current
platform :)


Understood, but systems needn't be homogeneous wrt graphics devices.



Ian,

Thanks for your kind discussion.

Tiejun

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

2015-02-05 Thread Ian Campbell 
On Thu, 2015-02-05 at 09:22 +0800, Chen, Tiejun wrote:
> Indeed this is not something workaround, and I think in any type of VGA 
> devices, we'd like to diminish this sort of thing gradually, right?
> 
> This mightn't come true in real world :)

It's not really something we can control, the h/w guys will do what they
do, including integrating on-board graphics tightly with the N/S-bridges
etc.

> >
> > I think there are three ways to achieve that:
> >
> >* Make the libxl/xl option something which is not generic e.g.
> >  igd_passthru=1
> >* Add a second option to allow the user to configure the kind of
> >  graphics device being passed thru (e.g. gfx_passthru=1,
> >  passthru_device="igd"), or combine the two by making the
> >  gfx_passthru option a string instead of a boolean.
> 
> It makes more sense but this mean we're going to change that existing 
> rule in qemu-traditional. But here I guess we shouldn't consider that case.

qemu-trad is frozen so we wouldn't be adding new features such as
support for new graphics passthru devices, so we can ignore it's
deficiencies in this area and just improve things in the qemu-xen case.
(we may want to add a compat handling for any new option we add to libxl
to translate to -gfx_passthru, but that's about it)

> >* Make libxl detect the type of graphics device somehow and
> >  therefore automatically determine when gfx_passthru=1 =>
> >  igd-passthru
> 
> This way confounds me all. Can libxl detect the graphics device *before* 
> we intend to pass a parameter to active qemu?

We know the BDF of all devices which we are going to pass to the guest
and we can observe various properties of that device
via /sys/bus/pci/devices/:B:D:F.

> 
> >
> >>   Currently, we have to set
> >> something as follows,
> >>
> >> gfx_passthru=1
> >> pci=["00:02.0"]
> >>
> >> This always works for qemu-xen-traditional.
> >>
> >> But you should know '00:02.0' doesn't mean we are passing IGD through.
> >
> > But by looking at the device 00:02.0 (e.g. its PCI vendor and device ID
> > and other properties) we can unambiguously determine if it is an IGD
> > device or not, can't we?
> 
> Again, like what I said above, I'm not sure if its possible in my case. 
> If I'm wrong please correct me.

Is my reply above sufficient?

> >>> If not then that _might_ suggest we should deprecate the gdx_passthru
> >>> option at the libxl interface and switch to something more expressive,
> >>> such as an Enumeration of card types (with a singleton of IGD right
> >>> now), but I'm not really very familiar with passthru nor the qemu side
> >>> of this.
> >>>
> >>> What happens if you try to pass two different GFX cards to a guest?
> >>>
> >>
> >> Are you saying two IGDs?
> >
> > Yes, or any combination of two cards, perhaps from different vendors
> > (AIUI some laptops have this with IGD and Nvidia or ATI?).
> 
> One IGD and multiple other type of Graphic display cards can coexist.

... but if they both need special handling then we need a way to
communicate that.

> >>   Its not possible since as I said above, IGD is
> >> tricky because it depends on something from ISA bridge and host bridge.
> >> So we can't provide two or more different setting configurations to own
> >> more IGDs just in one platform.
> >
> > This is because IGD must be a "primary" VGA device? I understand that
> 
> No. I mean ISA bridge and host bridge just provide one set of IGD 
> resource so its difficult to configure two or more IGDs.
> 
> > there can only be one of those in a system, but I think it is possible
> > to have multiple secondary VGA devices or different types in one system.
> >
> 
> What I'm saying is, its impossible to own two same IGDs in our current 
> platform :)

Understood, but systems needn't be homogeneous wrt graphics devices.

Ian.