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

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

[Tiejun Chen]

When we're working to support IGD GFX passthrough with qemu
upstream, instead of "-gfx_passthru" we'd like to make that
a machine option, "-machine xxx,-igd-passthru=on". This need
to bring a change on tool side.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
v2:
 
* Based on some discussions with Wei we'd like to keep both old
  option, -gfx_passthru, and new machine property option,
  "-machine xxx,-igd-passthru=on" at the same time but deprecate
  the old one. Then finally we remove the old one at that point
  that to give downstream (in this case, Xen) time to cope with the
  change.

 tools/libxl/libxl_dm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..8405f0b 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-net");
             flexarray_append(dm_args, "none");
         }
+        /*
+         * Although we already introduce 'igd-passthru', but we'd like
+         * to remove this until we give downstream time to cope with
+         * the change.
+         */
         if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
             flexarray_append(dm_args, "-gfx_passthru");
         }
@@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                             machinearg, max_ram_below_4g);
             }
         }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            machinearg = libxl__sprintf(gc, "%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]);
-- 
1.9.1

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

[Ian Campbell]

On Mon, 2015-02-02 at 09:17 +0800, Tiejun Chen wrote:
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that
> a machine option, "-machine xxx,-igd-passthru=on". This need
> to bring a change on tool side.

>From which Qemu version is this new option accepted? What will a verison
of qemu prior to then do when presented with the new option?

(nb: you have an extra '-' in the description I think)

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> v2:
>  
> * Based on some discussions with Wei we'd like to keep both old
>   option, -gfx_passthru, and new machine property option,
>   "-machine xxx,-igd-passthru=on" at the same time but deprecate
>   the old one. Then finally we remove the old one at that point
>   that to give downstream (in this case, Xen) time to cope with the
>   change.
> 
>  tools/libxl/libxl_dm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c2b0487..8405f0b 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-net");
>              flexarray_append(dm_args, "none");
>          }
> +        /*
> +         * Although we already introduce 'igd-passthru', but we'd like
> +         * to remove this until we give downstream time to cope with
> +         * the change.
> +         */
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            machinearg = libxl__sprintf(gc, "%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]);

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

[Ian Campbell]

On Mon, 2015-02-02 at 09:17 +0800, Tiejun Chen wrote:
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that
> a machine option, "-machine xxx,-igd-passthru=on". This need
> to bring a change on tool side.

>From which Qemu version is this new option accepted? What will a verison
of qemu prior to then do when presented with the new option?

(nb: you have an extra '-' in the description I think)

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> v2:
>  
> * Based on some discussions with Wei we'd like to keep both old
>   option, -gfx_passthru, and new machine property option,
>   "-machine xxx,-igd-passthru=on" at the same time but deprecate
>   the old one. Then finally we remove the old one at that point
>   that to give downstream (in this case, Xen) time to cope with the
>   change.
> 
>  tools/libxl/libxl_dm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c2b0487..8405f0b 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, "-net");
>              flexarray_append(dm_args, "none");
>          }
> +        /*
> +         * Although we already introduce 'igd-passthru', but we'd like
> +         * to remove this until we give downstream time to cope with
> +         * the change.
> +         */
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            machinearg = libxl__sprintf(gc, "%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]);

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

[Chen, Tiejun]

On 2015/2/2 19:08, Ian Campbell wrote:
> On Mon, 2015-02-02 at 09:17 +0800, Tiejun Chen wrote:
>> When we're working to support IGD GFX passthrough with qemu
>> upstream, instead of "-gfx_passthru" we'd like to make that
>> a machine option, "-machine xxx,-igd-passthru=on". This need
>> to bring a change on tool side.
>
>>From which Qemu version is this new option accepted? What will a verison
> of qemu prior to then do when presented with the new option?

Sorry, maybe I'm not describing this correctly.

Actually qemu upstream never own this option, '-gfx_passthru' at all. 
This just exists alone in qemu-xen-traditional. So here I'm trying to 
introduce a new stuff that doesn't clash anything in qemu upstream.

So I guess I should rephrase this as follows:

     libxl: add one machine property to support IGD GFX passthrough

     When we're working to support IGD GFX passthrough with qemu
     upstream, we'd like to introduce a machine option,
     "-machine xxx,igd-passthru=on", to enable/disable that feature.
     And we also remove that old option, "-gfx_passthru", just from
     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
     no any qemu stream version really need or use that.

>
> (nb: you have an extra '-' in the description I think)

Yeah, I will remove that.

Thanks
Tiejun

>
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> v2:
>>
>> * Based on some discussions with Wei we'd like to keep both old
>>    option, -gfx_passthru, and new machine property option,
>>    "-machine xxx,-igd-passthru=on" at the same time but deprecate
>>    the old one. Then finally we remove the old one at that point
>>    that to give downstream (in this case, Xen) time to cope with the
>>    change.
>>
>>   tools/libxl/libxl_dm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index c2b0487..8405f0b 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>               flexarray_append(dm_args, "-net");
>>               flexarray_append(dm_args, "none");
>>           }
>> +        /*
>> +         * Although we already introduce 'igd-passthru', but we'd like
>> +         * to remove this until we give downstream time to cope with
>> +         * the change.
>> +         */
>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>               flexarray_append(dm_args, "-gfx_passthru");
>>           }
>> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            machinearg = libxl__sprintf(gc, "%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]);
>
>
>

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

[Chen, Tiejun]

On 2015/2/2 19:08, Ian Campbell wrote:
> On Mon, 2015-02-02 at 09:17 +0800, Tiejun Chen wrote:
>> When we're working to support IGD GFX passthrough with qemu
>> upstream, instead of "-gfx_passthru" we'd like to make that
>> a machine option, "-machine xxx,-igd-passthru=on". This need
>> to bring a change on tool side.
>
>>From which Qemu version is this new option accepted? What will a verison
> of qemu prior to then do when presented with the new option?

Sorry, maybe I'm not describing this correctly.

Actually qemu upstream never own this option, '-gfx_passthru' at all. 
This just exists alone in qemu-xen-traditional. So here I'm trying to 
introduce a new stuff that doesn't clash anything in qemu upstream.

So I guess I should rephrase this as follows:

     libxl: add one machine property to support IGD GFX passthrough

     When we're working to support IGD GFX passthrough with qemu
     upstream, we'd like to introduce a machine option,
     "-machine xxx,igd-passthru=on", to enable/disable that feature.
     And we also remove that old option, "-gfx_passthru", just from
     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
     no any qemu stream version really need or use that.

>
> (nb: you have an extra '-' in the description I think)

Yeah, I will remove that.

Thanks
Tiejun

>
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> v2:
>>
>> * Based on some discussions with Wei we'd like to keep both old
>>    option, -gfx_passthru, and new machine property option,
>>    "-machine xxx,-igd-passthru=on" at the same time but deprecate
>>    the old one. Then finally we remove the old one at that point
>>    that to give downstream (in this case, Xen) time to cope with the
>>    change.
>>
>>   tools/libxl/libxl_dm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index c2b0487..8405f0b 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>               flexarray_append(dm_args, "-net");
>>               flexarray_append(dm_args, "none");
>>           }
>> +        /*
>> +         * Although we already introduce 'igd-passthru', but we'd like
>> +         * to remove this until we give downstream time to cope with
>> +         * the change.
>> +         */
>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>               flexarray_append(dm_args, "-gfx_passthru");
>>           }
>> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            machinearg = libxl__sprintf(gc, "%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]);
>
>
>

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

[Wei Liu]

On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that
> a machine option, "-machine xxx,-igd-passthru=on". This need
> to bring a change on tool side.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> v2:
>  
> * Based on some discussions with Wei we'd like to keep both old
>   option, -gfx_passthru, and new machine property option,
>   "-machine xxx,-igd-passthru=on" at the same time but deprecate
>   the old one. Then finally we remove the old one at that point
>   that to give downstream (in this case, Xen) time to cope with the
>   change.
> 

My suggestion has one premise -- if upstream QEMU has already released
that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
at all, then there is nothing to keep and deprecate.

>  tools/libxl/libxl_dm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c2b0487..8405f0b 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,

Note this function is upstream QEMU specfic.

>              flexarray_append(dm_args, "-net");
>              flexarray_append(dm_args, "none");
>          }
> +        /*
> +         * Although we already introduce 'igd-passthru', but we'd like
> +         * to remove this until we give downstream time to cope with
> +         * the change.
> +         */
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }

The comment contradicts what I know (or what I think I know). In our
last email exchange you said there was no "-gfx_passthru" in any version
of upstream QEMU.

So, shouldn't you just remove this `if' statement?

Wei.

> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            machinearg = libxl__sprintf(gc, "%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]);
> -- 
> 1.9.1

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

[Wei Liu]

On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> When we're working to support IGD GFX passthrough with qemu
> upstream, instead of "-gfx_passthru" we'd like to make that
> a machine option, "-machine xxx,-igd-passthru=on". This need
> to bring a change on tool side.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
> v2:
>  
> * Based on some discussions with Wei we'd like to keep both old
>   option, -gfx_passthru, and new machine property option,
>   "-machine xxx,-igd-passthru=on" at the same time but deprecate
>   the old one. Then finally we remove the old one at that point
>   that to give downstream (in this case, Xen) time to cope with the
>   change.
> 

My suggestion has one premise -- if upstream QEMU has already released
that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
at all, then there is nothing to keep and deprecate.

>  tools/libxl/libxl_dm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c2b0487..8405f0b 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,

Note this function is upstream QEMU specfic.

>              flexarray_append(dm_args, "-net");
>              flexarray_append(dm_args, "none");
>          }
> +        /*
> +         * Although we already introduce 'igd-passthru', but we'd like
> +         * to remove this until we give downstream time to cope with
> +         * the change.
> +         */
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }

The comment contradicts what I know (or what I think I know). In our
last email exchange you said there was no "-gfx_passthru" in any version
of upstream QEMU.

So, shouldn't you just remove this `if' statement?

Wei.

> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            machinearg = libxl__sprintf(gc, "%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]);
> -- 
> 1.9.1

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

[Ian Jackson]

Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> > When we're working to support IGD GFX passthrough with qemu
> > upstream, instead of "-gfx_passthru" we'd like to make that
> > a machine option, "-machine xxx,-igd-passthru=on". This need
> > to bring a change on tool side.
...
> My suggestion has one premise -- if upstream QEMU has already released
> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> at all, then there is nothing to keep and deprecate.

I think the commit message of the xen.git commit should explain what
options are supported by which versions of qemu (including qemu
upstream's future plans).

That would provide (a) something which summarises the communication
etc. with qemu upstream and can be checked with them if necessary and
(b) something against which the libxl changes can be easily judged.

Thanks,
Ian.

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

[Chen, Tiejun]

On 2015/2/2 20:54, Ian Jackson wrote:
> Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
>> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>>> When we're working to support IGD GFX passthrough with qemu
>>> upstream, instead of "-gfx_passthru" we'd like to make that
>>> a machine option, "-machine xxx,-igd-passthru=on". This need
>>> to bring a change on tool side.
> ...
>> My suggestion has one premise -- if upstream QEMU has already released
>> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
>> at all, then there is nothing to keep and deprecate.
>
> I think the commit message of the xen.git commit should explain what
> options are supported by which versions of qemu (including qemu
> upstream's future plans).
>
> That would provide (a) something which summarises the communication
> etc. with qemu upstream and can be checked with them if necessary and
> (b) something against which the libxl changes can be easily judged.
>

Sorry, looks I'm misleading this to everyone.

Here I picked my reply from another email:

Actually qemu upstream never own this option, '-gfx_passthru' at all. 
This just exists alone in qemu-xen-traditional. So here I'm trying to 
introduce a new stuff that doesn't clash anything in qemu upstream.

So I guess I should rephrase this as follows:

     libxl: add one machine property to support IGD GFX passthrough

     When we're working to support IGD GFX passthrough with qemu
     upstream, we'd like to introduce a machine option,
     "-machine xxx,igd-passthru=on", to enable/disable that feature.
     And we also remove that old option, "-gfx_passthru", just from
     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
     no any qemu stream version really need or use that.

So is it good?

Thanks
Tiejun

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

[Ian Campbell]

On Tue, 2015-02-03 at 09:04 +0800, Chen, Tiejun wrote:
> 
> On 2015/2/2 20:54, Ian Jackson wrote:
> > Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
> >> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> >>> When we're working to support IGD GFX passthrough with qemu
> >>> upstream, instead of "-gfx_passthru" we'd like to make that
> >>> a machine option, "-machine xxx,-igd-passthru=on". This need
> >>> to bring a change on tool side.
> > ...
> >> My suggestion has one premise -- if upstream QEMU has already released
> >> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> >> at all, then there is nothing to keep and deprecate.
> >
> > I think the commit message of the xen.git commit should explain what
> > options are supported by which versions of qemu (including qemu
> > upstream's future plans).
> >
> > That would provide (a) something which summarises the communication
> > etc. with qemu upstream and can be checked with them if necessary and
> > (b) something against which the libxl changes can be easily judged.

> Sorry, looks I'm misleading this to everyone.
> 
> Here I picked my reply from another email:
> 
> Actually qemu upstream never own this option, '-gfx_passthru' at all. 
> This just exists alone in qemu-xen-traditional. So here I'm trying to 
> introduce a new stuff that doesn't clash anything in qemu upstream.
> 
> So I guess I should rephrase this as follows:
> 
>      libxl: add one machine property to support IGD GFX passthrough
> 
>      When we're working to support IGD GFX passthrough with qemu
>      upstream, we'd like to introduce a machine option,

Has this new option been acked/accepted into the upstream qemu code base
yet? I think there should also be a reference to the relevant qemu.git
changeset as well as to any useful conversations about it.

>      "-machine xxx,igd-passthru=on", to enable/disable that feature.
>      And we also remove that old option, "-gfx_passthru", just from
>      the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>      no any qemu stream version really need or use that.
                   ^up ?

What happens if you pass this new option to an older version of qemu
upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
would have done.

I have one more general concern, which is that hardcoding igd-passthru
here may make it harder to support gfx_passthru of other cards in the
future.

Somehow something in the stack needs to either detect or be told which
kind of card to passthrough. Automatic detection would be preferable,
but maybe being told by the user is the only possibility.

Is there any way, given gfx_passthru as a boolean that libxl can
automatically figure out that IGD passthru is what is actually desired
-- e.g. by scanning the set of PCI devices given to the guest perhaps?

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?

Ian.

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

[Chen, Tiejun]

On 2015/2/3 19:07, Ian Campbell wrote:
> On Tue, 2015-02-03 at 09:04 +0800, Chen, Tiejun wrote:
>>
>> On 2015/2/2 20:54, Ian Jackson wrote:
>>> Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
>>>> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>>>>> When we're working to support IGD GFX passthrough with qemu
>>>>> upstream, instead of "-gfx_passthru" we'd like to make that
>>>>> a machine option, "-machine xxx,-igd-passthru=on". This need
>>>>> to bring a change on tool side.
>>> ...
>>>> My suggestion has one premise -- if upstream QEMU has already released
>>>> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
>>>> at all, then there is nothing to keep and deprecate.
>>>
>>> I think the commit message of the xen.git commit should explain what
>>> options are supported by which versions of qemu (including qemu
>>> upstream's future plans).
>>>
>>> That would provide (a) something which summarises the communication
>>> etc. with qemu upstream and can be checked with them if necessary and
>>> (b) something against which the libxl changes can be easily judged.
>
>> Sorry, looks I'm misleading this to everyone.
>>
>> Here I picked my reply from another email:
>>
>> Actually qemu upstream never own this option, '-gfx_passthru' at all.
>> This just exists alone in qemu-xen-traditional. So here I'm trying to
>> introduce a new stuff that doesn't clash anything in qemu upstream.
>>
>> So I guess I should rephrase this as follows:
>>
>>       libxl: add one machine property to support IGD GFX passthrough
>>
>>       When we're working to support IGD GFX passthrough with qemu
>>       upstream, we'd like to introduce a machine option,
>
> Has this new option been acked/accepted into the upstream qemu code base
> yet? I think there should also be a reference to the relevant qemu.git

Yes, Gerd just recommend this option and I guess this should be acked as 
well :)

> changeset as well as to any useful conversations about it.

I just post this patch prior to those qemu patch supporting IGD 
passthrough since I need this option is acked on Xen side. Then I can 
continue submitting all qemu patches.

>
>>       "-machine xxx,igd-passthru=on", to enable/disable that feature.
>>       And we also remove that old option, "-gfx_passthru", just from
>>       the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>>       no any qemu stream version really need or use that.
>                     ^up ?
>
> What happens if you pass this new option to an older version of qemu
> upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
> would have done.

Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of 
qemu upstream. As I mentioned previously, just now we're starting to 
support this in qemu upstream :)

But you known, on Xen side we have two qemu versions, 
qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted 
in both two versions, just qemu-xen-traditional supports IGD passthrough 
completely. For qemu-xen, we just have this term definition but without 
that IGD passthrough feature support actually.

And now we're trying to support IGD passthrough in qemu stream. This 
mean we should set 'device_model_version="qemu-xen", right? Then libxl 
still pass '-gfx_passthru' to qemu upstream. But when I post other 
stuffs to qemu upstream community to support IGD passthrough. Gerd 
thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'. 
So finally you see this change in Xen/tools/libxl.

>
> I have one more general concern, which is that hardcoding igd-passthru
> here may make it harder to support gfx_passthru of other cards in the
> future.

Actually gfx_passthrou is introduced to just work for IGD passthrough 
since something specific to IGD is tricky, so we have to need such a 
flag to handle this precisely, like its fixed at 00:02.0, and expose 
some ISA bridge PCI config info and even host bridge PCI config info.

So I don't thing other cards need this.

>
> Somehow something in the stack needs to either detect or be told which
> kind of card to passthrough. Automatic detection would be preferable,
> but maybe being told by the user is the only possibility.

Based on the above explanation, something should be done before we 
detect to construct that real card , so its difficulty to achieve this 
goal currently.

>
> Is there any way, given gfx_passthru as a boolean that libxl can
> automatically figure out that IGD passthru is what is actually desired
> -- e.g. by scanning the set of PCI devices given to the guest perhaps?

Sorry I don't understand what you mean here. 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.

>
> 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? 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.

Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/2/3 19:07, Ian Campbell wrote:
> On Tue, 2015-02-03 at 09:04 +0800, Chen, Tiejun wrote:
>>
>> On 2015/2/2 20:54, Ian Jackson wrote:
>>> Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
>>>> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>>>>> When we're working to support IGD GFX passthrough with qemu
>>>>> upstream, instead of "-gfx_passthru" we'd like to make that
>>>>> a machine option, "-machine xxx,-igd-passthru=on". This need
>>>>> to bring a change on tool side.
>>> ...
>>>> My suggestion has one premise -- if upstream QEMU has already released
>>>> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
>>>> at all, then there is nothing to keep and deprecate.
>>>
>>> I think the commit message of the xen.git commit should explain what
>>> options are supported by which versions of qemu (including qemu
>>> upstream's future plans).
>>>
>>> That would provide (a) something which summarises the communication
>>> etc. with qemu upstream and can be checked with them if necessary and
>>> (b) something against which the libxl changes can be easily judged.
>
>> Sorry, looks I'm misleading this to everyone.
>>
>> Here I picked my reply from another email:
>>
>> Actually qemu upstream never own this option, '-gfx_passthru' at all.
>> This just exists alone in qemu-xen-traditional. So here I'm trying to
>> introduce a new stuff that doesn't clash anything in qemu upstream.
>>
>> So I guess I should rephrase this as follows:
>>
>>       libxl: add one machine property to support IGD GFX passthrough
>>
>>       When we're working to support IGD GFX passthrough with qemu
>>       upstream, we'd like to introduce a machine option,
>
> Has this new option been acked/accepted into the upstream qemu code base
> yet? I think there should also be a reference to the relevant qemu.git

Yes, Gerd just recommend this option and I guess this should be acked as 
well :)

> changeset as well as to any useful conversations about it.

I just post this patch prior to those qemu patch supporting IGD 
passthrough since I need this option is acked on Xen side. Then I can 
continue submitting all qemu patches.

>
>>       "-machine xxx,igd-passthru=on", to enable/disable that feature.
>>       And we also remove that old option, "-gfx_passthru", just from
>>       the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>>       no any qemu stream version really need or use that.
>                     ^up ?
>
> What happens if you pass this new option to an older version of qemu
> upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
> would have done.

Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of 
qemu upstream. As I mentioned previously, just now we're starting to 
support this in qemu upstream :)

But you known, on Xen side we have two qemu versions, 
qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted 
in both two versions, just qemu-xen-traditional supports IGD passthrough 
completely. For qemu-xen, we just have this term definition but without 
that IGD passthrough feature support actually.

And now we're trying to support IGD passthrough in qemu stream. This 
mean we should set 'device_model_version="qemu-xen", right? Then libxl 
still pass '-gfx_passthru' to qemu upstream. But when I post other 
stuffs to qemu upstream community to support IGD passthrough. Gerd 
thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'. 
So finally you see this change in Xen/tools/libxl.

>
> I have one more general concern, which is that hardcoding igd-passthru
> here may make it harder to support gfx_passthru of other cards in the
> future.

Actually gfx_passthrou is introduced to just work for IGD passthrough 
since something specific to IGD is tricky, so we have to need such a 
flag to handle this precisely, like its fixed at 00:02.0, and expose 
some ISA bridge PCI config info and even host bridge PCI config info.

So I don't thing other cards need this.

>
> Somehow something in the stack needs to either detect or be told which
> kind of card to passthrough. Automatic detection would be preferable,
> but maybe being told by the user is the only possibility.

Based on the above explanation, something should be done before we 
detect to construct that real card , so its difficulty to achieve this 
goal currently.

>
> Is there any way, given gfx_passthru as a boolean that libxl can
> automatically figure out that IGD passthru is what is actually desired
> -- e.g. by scanning the set of PCI devices given to the guest perhaps?

Sorry I don't understand what you mean here. 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.

>
> 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? 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.

Thanks
Tiejun

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

[Ian Campbell]

On Wed, 2015-02-04 at 09:34 +0800, Chen, Tiejun wrote:
> >
> >>       "-machine xxx,igd-passthru=on", to enable/disable that feature.
> >>       And we also remove that old option, "-gfx_passthru", just from
> >>       the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
> >>       no any qemu stream version really need or use that.
> >                     ^up ?
> >
> > What happens if you pass this new option to an older version of qemu
> > upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
> > would have done.
> 
> Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of 
> qemu upstream. As I mentioned previously, just now we're starting to 
> support this in qemu upstream :)
> 
> But you known, on Xen side we have two qemu versions, 
> qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted 
> in both two versions, just qemu-xen-traditional supports IGD passthrough 
> completely. For qemu-xen, we just have this term definition but without 
> that IGD passthrough feature support actually.

I'm afraid I don't follow, you seem to be simultaneously saying that
qemu-xen both does and does not support -gfx_passthru, which cannot be
right. I think from the following paragraph that what you mean is that
upstream qemu-xen has no support for any kind of -gfx_passthru command
line option in any form in any existing version, including the git tree.
Is that correct?

(for the purposes of this conversation qemu-traditional is not of
interest)

> And now we're trying to support IGD passthrough in qemu stream. This 
> mean we should set 'device_model_version="qemu-xen", right? Then libxl 
> still pass '-gfx_passthru' to qemu upstream. But when I post other 
> stuffs to qemu upstream community to support IGD passthrough. Gerd 
> thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'. 
> So finally you see this change in Xen/tools/libxl.
> 
> >
> > I have one more general concern, which is that hardcoding igd-passthru
> > here may make it harder to support gfx_passthru of other cards in the
> > future.
> 
> Actually gfx_passthrou is introduced to just work for IGD passthrough 
> since something specific to IGD is tricky, so we have to need such a 
> flag to handle this precisely, like its fixed at 00:02.0, and expose 
> some ISA bridge PCI config info and even host bridge PCI config info.
> 
> So I don't thing other cards need this.

If one type VGA device needs these sorts of workaround it is not
inconceivable that some other one will also need workarounds in the
future.

Even if you don't consider non-IGD, what about the possibility of a
future IGD device which needs different (or additional, or fewer)
workarounds?

> > Somehow something in the stack needs to either detect or be told which
> > kind of card to passthrough. Automatic detection would be preferable,
> > but maybe being told by the user is the only possibility.
> 
> Based on the above explanation, something should be done before we 
> detect to construct that real card , so its difficulty to achieve this 
> goal currently.
> 
> >
> > Is there any way, given gfx_passthru as a boolean that libxl can
> > automatically figure out that IGD passthru is what is actually desired
> > -- e.g. by scanning the set of PCI devices given to the guest perhaps?
> 
> Sorry I don't understand what you mean here.

"gfx_passthru" is a generically named option, but it is being
implemented in an IGD specific way. We need to consider the possibility
of other graphics devices needing special handling in the future and
plan accordingly such that we can try and maintain our API guarantees
when this happens.

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.
      * Make libxl detect the type of graphics device somehow and
        therefore automatically determine when gfx_passthru=1 =>
        igd-passthru

>  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?

> > 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?).

>  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
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.

Ian.

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

[Ian Campbell]

On Wed, 2015-02-04 at 09:34 +0800, Chen, Tiejun wrote:
> >
> >>       "-machine xxx,igd-passthru=on", to enable/disable that feature.
> >>       And we also remove that old option, "-gfx_passthru", just from
> >>       the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
> >>       no any qemu stream version really need or use that.
> >                     ^up ?
> >
> > What happens if you pass this new option to an older version of qemu
> > upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
> > would have done.
> 
> Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of 
> qemu upstream. As I mentioned previously, just now we're starting to 
> support this in qemu upstream :)
> 
> But you known, on Xen side we have two qemu versions, 
> qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted 
> in both two versions, just qemu-xen-traditional supports IGD passthrough 
> completely. For qemu-xen, we just have this term definition but without 
> that IGD passthrough feature support actually.

I'm afraid I don't follow, you seem to be simultaneously saying that
qemu-xen both does and does not support -gfx_passthru, which cannot be
right. I think from the following paragraph that what you mean is that
upstream qemu-xen has no support for any kind of -gfx_passthru command
line option in any form in any existing version, including the git tree.
Is that correct?

(for the purposes of this conversation qemu-traditional is not of
interest)

> And now we're trying to support IGD passthrough in qemu stream. This 
> mean we should set 'device_model_version="qemu-xen", right? Then libxl 
> still pass '-gfx_passthru' to qemu upstream. But when I post other 
> stuffs to qemu upstream community to support IGD passthrough. Gerd 
> thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'. 
> So finally you see this change in Xen/tools/libxl.
> 
> >
> > I have one more general concern, which is that hardcoding igd-passthru
> > here may make it harder to support gfx_passthru of other cards in the
> > future.
> 
> Actually gfx_passthrou is introduced to just work for IGD passthrough 
> since something specific to IGD is tricky, so we have to need such a 
> flag to handle this precisely, like its fixed at 00:02.0, and expose 
> some ISA bridge PCI config info and even host bridge PCI config info.
> 
> So I don't thing other cards need this.

If one type VGA device needs these sorts of workaround it is not
inconceivable that some other one will also need workarounds in the
future.

Even if you don't consider non-IGD, what about the possibility of a
future IGD device which needs different (or additional, or fewer)
workarounds?

> > Somehow something in the stack needs to either detect or be told which
> > kind of card to passthrough. Automatic detection would be preferable,
> > but maybe being told by the user is the only possibility.
> 
> Based on the above explanation, something should be done before we 
> detect to construct that real card , so its difficulty to achieve this 
> goal currently.
> 
> >
> > Is there any way, given gfx_passthru as a boolean that libxl can
> > automatically figure out that IGD passthru is what is actually desired
> > -- e.g. by scanning the set of PCI devices given to the guest perhaps?
> 
> Sorry I don't understand what you mean here.

"gfx_passthru" is a generically named option, but it is being
implemented in an IGD specific way. We need to consider the possibility
of other graphics devices needing special handling in the future and
plan accordingly such that we can try and maintain our API guarantees
when this happens.

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.
      * Make libxl detect the type of graphics device somehow and
        therefore automatically determine when gfx_passthru=1 =>
        igd-passthru

>  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?

> > 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?).

>  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
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.

Ian.

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

[Chen, Tiejun]

On 2015/2/4 18:41, Ian Campbell wrote:
> On Wed, 2015-02-04 at 09:34 +0800, Chen, Tiejun wrote:
>>>
>>>>        "-machine xxx,igd-passthru=on", to enable/disable that feature.
>>>>        And we also remove that old option, "-gfx_passthru", just from
>>>>        the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>>>>        no any qemu stream version really need or use that.
>>>                      ^up ?
>>>
>>> What happens if you pass this new option to an older version of qemu
>>> upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
>>> would have done.
>>
>> Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of
>> qemu upstream. As I mentioned previously, just now we're starting to
>> support this in qemu upstream :)
>>
>> But you known, on Xen side we have two qemu versions,
>> qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted
>> in both two versions, just qemu-xen-traditional supports IGD passthrough
>> completely. For qemu-xen, we just have this term definition but without
>> that IGD passthrough feature support actually.
>
> I'm afraid I don't follow, you seem to be simultaneously saying that
> qemu-xen both does and does not support -gfx_passthru, which cannot be
> right. I think from the following paragraph that what you mean is that
> upstream qemu-xen has no support for any kind of -gfx_passthru command
> line option in any form in any existing version, including the git tree.
> Is that correct?

Yes.

>
> (for the purposes of this conversation qemu-traditional is not of
> interest)

Yes.

>
>> And now we're trying to support IGD passthrough in qemu stream. This
>> mean we should set 'device_model_version="qemu-xen", right? Then libxl
>> still pass '-gfx_passthru' to qemu upstream. But when I post other
>> stuffs to qemu upstream community to support IGD passthrough. Gerd
>> thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'.
>> So finally you see this change in Xen/tools/libxl.
>>
>>>
>>> I have one more general concern, which is that hardcoding igd-passthru
>>> here may make it harder to support gfx_passthru of other cards in the
>>> future.
>>
>> Actually gfx_passthrou is introduced to just work for IGD passthrough
>> since something specific to IGD is tricky, so we have to need such a
>> flag to handle this precisely, like its fixed at 00:02.0, and expose
>> some ISA bridge PCI config info and even host bridge PCI config info.
>>
>> So I don't thing other cards need this.
>
> If one type VGA device needs these sorts of workaround it is not
> inconceivable that some other one will also need workarounds in the
> future.
>

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 :)

> Even if you don't consider non-IGD, what about the possibility of a
> future IGD device which needs different (or additional, or fewer)
> workarounds?

As far as I know we're trying to drop off those dependencies on ISA 
bridge and host bridge in our IGD's roadmap. Because in any pass through 
cases, theoretically we should access those resources dedicated to that 
device.

>
>>> Somehow something in the stack needs to either detect or be told which
>>> kind of card to passthrough. Automatic detection would be preferable,
>>> but maybe being told by the user is the only possibility.
>>
>> Based on the above explanation, something should be done before we
>> detect to construct that real card , so its difficulty to achieve this
>> goal currently.
>>
>>>
>>> Is there any way, given gfx_passthru as a boolean that libxl can
>>> automatically figure out that IGD passthru is what is actually desired
>>> -- e.g. by scanning the set of PCI devices given to the guest perhaps?
>>
>> Sorry I don't understand what you mean here.
>
> "gfx_passthru" is a generically named option, but it is being
> implemented in an IGD specific way. We need to consider the possibility
> of other graphics devices needing special handling in the future and
> plan accordingly such that we can try and maintain our API guarantees
> when this happens.

Agreed.

>
> 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.

>        * 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?

>
>>   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.

>
>>> 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.

>
>>   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 :)

Thanks
Tiejun

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

[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/0000: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.

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

[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/0000: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

[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/0000: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

[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/0000: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, /* 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)
+{
+    int i, j, ret = 0;
+
+    if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru))
+        return ret;
+
+    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+        libxl_device_pci *pcidev = &d_config->pcidevs[i];
+        char *pci_device_vendor_path =
+            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+                           pcidev->domain, pcidev->bus, pcidev->dev,
+                           pcidev->func);
+        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_vendor, pci_device_device;
+        unsigned int num = ARRAY_SIZE(igd_ids);
+
+        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);
+            continue;
+        }
+        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);
+            continue;
+        }
+        if (pci_device_vendor != 0x8086) /* Intel class */
+            continue;
+
+        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);
+            continue;
+        }
+        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);
+            continue;
+        }
+        for (j = 0 ; j < num ; j++) {
+            if (pci_device_device == igd_ids[j]) { /* IGD class */
+                ret = 1;
+                break;
+            }
+        }
+    }
+
+    return ret;
+}
+
  /*
   * A brief comment about slots.  I don't know what slots are for; however,
   * I have by experimentation determined:


Thanks
Tiejun

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

[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] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[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

[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 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

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

[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 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
>
>
>

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

[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 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
>
>
>

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

[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

[Chen, Tiejun]

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

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

[Ian Campbell]

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.

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

[Chen, Tiejun]

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

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

[Chen, Tiejun]

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

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

[Ian Campbell]

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

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

[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 else to align this change.
>>
>> Thanks
>> Tiejun
>
>
>

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

[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 else to align this change.
>>
>> Thanks
>> Tiejun
>
>
>

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

[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 = GCSPRINTF("%s,igd-passthru=on",
>>> machinearg);
>>> +        }
>>> +
>>>
>>> Of course we need modify something else to align this change.
>>>
>>> Thanks
>>> Tiejun
>>
>>
>>
>
>
>

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

[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 = GCSPRINTF("%s,igd-passthru=on",
>>> machinearg);
>>> +        }
>>> +
>>>
>>> Of course we need modify something else to align this change.
>>>
>>> Thanks
>>> Tiejun
>>
>>
>>
>
>
>

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

[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

[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] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[Ian Campbell]

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

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

[Ian Campbell]

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.

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

[Chen, Tiejun]

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

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

[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] [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough

[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 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

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

[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/0000: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, /* 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)
+{
+    int i, j, ret = 0;
+
+    if (!libxl_defbool_val(d_config->b_info.u.hvm.gfx_passthru))
+        return ret;
+
+    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
+        libxl_device_pci *pcidev = &d_config->pcidevs[i];
+        char *pci_device_vendor_path =
+            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
+                           pcidev->domain, pcidev->bus, pcidev->dev,
+                           pcidev->func);
+        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_vendor, pci_device_device;
+        unsigned int num = ARRAY_SIZE(igd_ids);
+
+        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);
+            continue;
+        }
+        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);
+            continue;
+        }
+        if (pci_device_vendor != 0x8086) /* Intel class */
+            continue;
+
+        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);
+            continue;
+        }
+        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);
+            continue;
+        }
+        for (j = 0 ; j < num ; j++) {
+            if (pci_device_device == igd_ids[j]) { /* IGD class */
+                ret = 1;
+                break;
+            }
+        }
+    }
+
+    return ret;
+}
+
  /*
   * A brief comment about slots.  I don't know what slots are for; however,
   * I have by experimentation determined:


Thanks
Tiejun

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

[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/0000: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.

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

[Chen, Tiejun]

On 2015/2/4 18:41, Ian Campbell wrote:
> On Wed, 2015-02-04 at 09:34 +0800, Chen, Tiejun wrote:
>>>
>>>>        "-machine xxx,igd-passthru=on", to enable/disable that feature.
>>>>        And we also remove that old option, "-gfx_passthru", just from
>>>>        the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>>>>        no any qemu stream version really need or use that.
>>>                      ^up ?
>>>
>>> What happens if you pass this new option to an older version of qemu
>>> upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
>>> would have done.
>>
>> Neither '-gfx_passthru' nor 'igd-passthrou' exists in any version of
>> qemu upstream. As I mentioned previously, just now we're starting to
>> support this in qemu upstream :)
>>
>> But you known, on Xen side we have two qemu versions,
>> qemu-xen-traditional and qemu-xen. Although '-gfx_passthru' is adopted
>> in both two versions, just qemu-xen-traditional supports IGD passthrough
>> completely. For qemu-xen, we just have this term definition but without
>> that IGD passthrough feature support actually.
>
> I'm afraid I don't follow, you seem to be simultaneously saying that
> qemu-xen both does and does not support -gfx_passthru, which cannot be
> right. I think from the following paragraph that what you mean is that
> upstream qemu-xen has no support for any kind of -gfx_passthru command
> line option in any form in any existing version, including the git tree.
> Is that correct?

Yes.

>
> (for the purposes of this conversation qemu-traditional is not of
> interest)

Yes.

>
>> And now we're trying to support IGD passthrough in qemu stream. This
>> mean we should set 'device_model_version="qemu-xen", right? Then libxl
>> still pass '-gfx_passthru' to qemu upstream. But when I post other
>> stuffs to qemu upstream community to support IGD passthrough. Gerd
>> thought "-machine xxx,igd-passthru=on" is better than '-gfx_passthru'.
>> So finally you see this change in Xen/tools/libxl.
>>
>>>
>>> I have one more general concern, which is that hardcoding igd-passthru
>>> here may make it harder to support gfx_passthru of other cards in the
>>> future.
>>
>> Actually gfx_passthrou is introduced to just work for IGD passthrough
>> since something specific to IGD is tricky, so we have to need such a
>> flag to handle this precisely, like its fixed at 00:02.0, and expose
>> some ISA bridge PCI config info and even host bridge PCI config info.
>>
>> So I don't thing other cards need this.
>
> If one type VGA device needs these sorts of workaround it is not
> inconceivable that some other one will also need workarounds in the
> future.
>

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 :)

> Even if you don't consider non-IGD, what about the possibility of a
> future IGD device which needs different (or additional, or fewer)
> workarounds?

As far as I know we're trying to drop off those dependencies on ISA 
bridge and host bridge in our IGD's roadmap. Because in any pass through 
cases, theoretically we should access those resources dedicated to that 
device.

>
>>> Somehow something in the stack needs to either detect or be told which
>>> kind of card to passthrough. Automatic detection would be preferable,
>>> but maybe being told by the user is the only possibility.
>>
>> Based on the above explanation, something should be done before we
>> detect to construct that real card , so its difficulty to achieve this
>> goal currently.
>>
>>>
>>> Is there any way, given gfx_passthru as a boolean that libxl can
>>> automatically figure out that IGD passthru is what is actually desired
>>> -- e.g. by scanning the set of PCI devices given to the guest perhaps?
>>
>> Sorry I don't understand what you mean here.
>
> "gfx_passthru" is a generically named option, but it is being
> implemented in an IGD specific way. We need to consider the possibility
> of other graphics devices needing special handling in the future and
> plan accordingly such that we can try and maintain our API guarantees
> when this happens.

Agreed.

>
> 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.

>        * 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?

>
>>   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.

>
>>> 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.

>
>>   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 :)

Thanks
Tiejun

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

[Ian Campbell]

On Tue, 2015-02-03 at 09:04 +0800, Chen, Tiejun wrote:
> 
> On 2015/2/2 20:54, Ian Jackson wrote:
> > Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
> >> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> >>> When we're working to support IGD GFX passthrough with qemu
> >>> upstream, instead of "-gfx_passthru" we'd like to make that
> >>> a machine option, "-machine xxx,-igd-passthru=on". This need
> >>> to bring a change on tool side.
> > ...
> >> My suggestion has one premise -- if upstream QEMU has already released
> >> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> >> at all, then there is nothing to keep and deprecate.
> >
> > I think the commit message of the xen.git commit should explain what
> > options are supported by which versions of qemu (including qemu
> > upstream's future plans).
> >
> > That would provide (a) something which summarises the communication
> > etc. with qemu upstream and can be checked with them if necessary and
> > (b) something against which the libxl changes can be easily judged.

> Sorry, looks I'm misleading this to everyone.
> 
> Here I picked my reply from another email:
> 
> Actually qemu upstream never own this option, '-gfx_passthru' at all. 
> This just exists alone in qemu-xen-traditional. So here I'm trying to 
> introduce a new stuff that doesn't clash anything in qemu upstream.
> 
> So I guess I should rephrase this as follows:
> 
>      libxl: add one machine property to support IGD GFX passthrough
> 
>      When we're working to support IGD GFX passthrough with qemu
>      upstream, we'd like to introduce a machine option,

Has this new option been acked/accepted into the upstream qemu code base
yet? I think there should also be a reference to the relevant qemu.git
changeset as well as to any useful conversations about it.

>      "-machine xxx,igd-passthru=on", to enable/disable that feature.
>      And we also remove that old option, "-gfx_passthru", just from
>      the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>      no any qemu stream version really need or use that.
                   ^up ?

What happens if you pass this new option to an older version of qemu
upstream? I suppose it doesn't fail any worse than passing -gfx_passthru
would have done.

I have one more general concern, which is that hardcoding igd-passthru
here may make it harder to support gfx_passthru of other cards in the
future.

Somehow something in the stack needs to either detect or be told which
kind of card to passthrough. Automatic detection would be preferable,
but maybe being told by the user is the only possibility.

Is there any way, given gfx_passthru as a boolean that libxl can
automatically figure out that IGD passthru is what is actually desired
-- e.g. by scanning the set of PCI devices given to the guest perhaps?

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?

Ian.

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

[Chen, Tiejun]

On 2015/2/2 20:54, Ian Jackson wrote:
> Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
>> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>>> When we're working to support IGD GFX passthrough with qemu
>>> upstream, instead of "-gfx_passthru" we'd like to make that
>>> a machine option, "-machine xxx,-igd-passthru=on". This need
>>> to bring a change on tool side.
> ...
>> My suggestion has one premise -- if upstream QEMU has already released
>> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
>> at all, then there is nothing to keep and deprecate.
>
> I think the commit message of the xen.git commit should explain what
> options are supported by which versions of qemu (including qemu
> upstream's future plans).
>
> That would provide (a) something which summarises the communication
> etc. with qemu upstream and can be checked with them if necessary and
> (b) something against which the libxl changes can be easily judged.
>

Sorry, looks I'm misleading this to everyone.

Here I picked my reply from another email:

Actually qemu upstream never own this option, '-gfx_passthru' at all. 
This just exists alone in qemu-xen-traditional. So here I'm trying to 
introduce a new stuff that doesn't clash anything in qemu upstream.

So I guess I should rephrase this as follows:

     libxl: add one machine property to support IGD GFX passthrough

     When we're working to support IGD GFX passthrough with qemu
     upstream, we'd like to introduce a machine option,
     "-machine xxx,igd-passthru=on", to enable/disable that feature.
     And we also remove that old option, "-gfx_passthru", just from
     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
     no any qemu stream version really need or use that.

So is it good?

Thanks
Tiejun

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

[Ian Jackson]

Wei Liu writes ("Re: [v2][PATCH] libxl: add one machine property to support IGD GFX passthrough"):
> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> > When we're working to support IGD GFX passthrough with qemu
> > upstream, instead of "-gfx_passthru" we'd like to make that
> > a machine option, "-machine xxx,-igd-passthru=on". This need
> > to bring a change on tool side.
...
> My suggestion has one premise -- if upstream QEMU has already released
> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> at all, then there is nothing to keep and deprecate.

I think the commit message of the xen.git commit should explain what
options are supported by which versions of qemu (including qemu
upstream's future plans).

That would provide (a) something which summarises the communication
etc. with qemu upstream and can be checked with them if necessary and
(b) something against which the libxl changes can be easily judged.

Thanks,
Ian.

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

[Chen, Tiejun]

On 2015/2/2 20:19, Wei Liu wrote:
> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>> When we're working to support IGD GFX passthrough with qemu
>> upstream, instead of "-gfx_passthru" we'd like to make that
>> a machine option, "-machine xxx,-igd-passthru=on". This need
>> to bring a change on tool side.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> v2:
>>
>> * Based on some discussions with Wei we'd like to keep both old
>>    option, -gfx_passthru, and new machine property option,
>>    "-machine xxx,-igd-passthru=on" at the same time but deprecate
>>    the old one. Then finally we remove the old one at that point
>>    that to give downstream (in this case, Xen) time to cope with the
>>    change.
>>
>
> My suggestion has one premise -- if upstream QEMU has already released
> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> at all, then there is nothing to keep and deprecate.

Understood.

>
>>   tools/libxl/libxl_dm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index c2b0487..8405f0b 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>
> Note this function is upstream QEMU specfic.

Yeah.

>
>>               flexarray_append(dm_args, "-net");
>>               flexarray_append(dm_args, "none");
>>           }
>> +        /*
>> +         * Although we already introduce 'igd-passthru', but we'd like
>> +         * to remove this until we give downstream time to cope with
>> +         * the change.
>> +         */
>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>               flexarray_append(dm_args, "-gfx_passthru");
>>           }
>
> The comment contradicts what I know (or what I think I know). In our
> last email exchange you said there was no "-gfx_passthru" in any version
> of upstream QEMU.
>
> So, shouldn't you just remove this `if' statement?

Right. So what about this?

     libxl: add one machine property to support IGD GFX passthrough

     When we're working to support IGD GFX passthrough with qemu
     upstream, we'd like to introduce a machine option,
     "-machine xxx,igd-passthru=on", to enable/disable that feature.
     And we also remove that old option, "-gfx_passthru", just from
     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
     no any qemu stream version really need or use that.

     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..b888f19 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,9 +701,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");
@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            machinearg = libxl__sprintf(gc, "%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]);

Thanks
Tiejun

>
> Wei.
>
>> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            machinearg = libxl__sprintf(gc, "%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]);
>> --
>> 1.9.1
>

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

[Chen, Tiejun]

On 2015/2/2 20:19, Wei Liu wrote:
> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>> When we're working to support IGD GFX passthrough with qemu
>> upstream, instead of "-gfx_passthru" we'd like to make that
>> a machine option, "-machine xxx,-igd-passthru=on". This need
>> to bring a change on tool side.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>> v2:
>>
>> * Based on some discussions with Wei we'd like to keep both old
>>    option, -gfx_passthru, and new machine property option,
>>    "-machine xxx,-igd-passthru=on" at the same time but deprecate
>>    the old one. Then finally we remove the old one at that point
>>    that to give downstream (in this case, Xen) time to cope with the
>>    change.
>>
>
> My suggestion has one premise -- if upstream QEMU has already released
> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> at all, then there is nothing to keep and deprecate.

Understood.

>
>>   tools/libxl/libxl_dm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index c2b0487..8405f0b 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>
> Note this function is upstream QEMU specfic.

Yeah.

>
>>               flexarray_append(dm_args, "-net");
>>               flexarray_append(dm_args, "none");
>>           }
>> +        /*
>> +         * Although we already introduce 'igd-passthru', but we'd like
>> +         * to remove this until we give downstream time to cope with
>> +         * the change.
>> +         */
>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>               flexarray_append(dm_args, "-gfx_passthru");
>>           }
>
> The comment contradicts what I know (or what I think I know). In our
> last email exchange you said there was no "-gfx_passthru" in any version
> of upstream QEMU.
>
> So, shouldn't you just remove this `if' statement?

Right. So what about this?

     libxl: add one machine property to support IGD GFX passthrough

     When we're working to support IGD GFX passthrough with qemu
     upstream, we'd like to introduce a machine option,
     "-machine xxx,igd-passthru=on", to enable/disable that feature.
     And we also remove that old option, "-gfx_passthru", just from
     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
     no any qemu stream version really need or use that.

     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c2b0487..b888f19 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -701,9 +701,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");
@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            machinearg = libxl__sprintf(gc, "%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]);

Thanks
Tiejun

>
> Wei.
>
>> @@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            machinearg = libxl__sprintf(gc, "%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]);
>> --
>> 1.9.1
>

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

[Wei Liu]

On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote:
> 
> On 2015/2/2 20:19, Wei Liu wrote:
> >On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> >>When we're working to support IGD GFX passthrough with qemu
> >>upstream, instead of "-gfx_passthru" we'd like to make that
> >>a machine option, "-machine xxx,-igd-passthru=on". This need
> >>to bring a change on tool side.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>---
> >>v2:
> >>
> >>* Based on some discussions with Wei we'd like to keep both old
> >>   option, -gfx_passthru, and new machine property option,
> >>   "-machine xxx,-igd-passthru=on" at the same time but deprecate
> >>   the old one. Then finally we remove the old one at that point
> >>   that to give downstream (in this case, Xen) time to cope with the
> >>   change.
> >>
> >
> >My suggestion has one premise -- if upstream QEMU has already released
> >that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> >at all, then there is nothing to keep and deprecate.
> 
> Understood.
> 
> >
> >>  tools/libxl/libxl_dm.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >>index c2b0487..8405f0b 100644
> >>--- a/tools/libxl/libxl_dm.c
> >>+++ b/tools/libxl/libxl_dm.c
> >>@@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >
> >Note this function is upstream QEMU specfic.
> 
> Yeah.
> 
> >
> >>              flexarray_append(dm_args, "-net");
> >>              flexarray_append(dm_args, "none");
> >>          }
> >>+        /*
> >>+         * Although we already introduce 'igd-passthru', but we'd like
> >>+         * to remove this until we give downstream time to cope with
> >>+         * the change.
> >>+         */
> >>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >>              flexarray_append(dm_args, "-gfx_passthru");
> >>          }
> >
> >The comment contradicts what I know (or what I think I know). In our
> >last email exchange you said there was no "-gfx_passthru" in any version
> >of upstream QEMU.
> >
> >So, shouldn't you just remove this `if' statement?
> 
> Right. So what about this?
> 
>     libxl: add one machine property to support IGD GFX passthrough
> 
>     When we're working to support IGD GFX passthrough with qemu
>     upstream, we'd like to introduce a machine option,
>     "-machine xxx,igd-passthru=on", to enable/disable that feature.
>     And we also remove that old option, "-gfx_passthru", just from
>     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>     no any qemu stream version really need or use that.
> 
>     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> 

Yes. I think a patch like this reflects the reality.

It would be nice, as Ian J suggested, to state which version of QEMU
upstream introduces that new option in commit message.

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c2b0487..b888f19 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -701,9 +701,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");
> @@ -748,6 +745,11 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            machinearg = libxl__sprintf(gc, "%s,igd-passthru=on",
> machinearg);
> +        }
> +

Please use GCSPRINTF macro.

Wei.

>          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]);
> 

> Thanks
> Tiejun
> 
> >
> >Wei.
> >
> >>@@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >>                                              machinearg, max_ram_below_4g);
> >>              }
> >>          }
> >>+
> >>+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >>+            machinearg = libxl__sprintf(gc, "%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]);
> >>--
> >>1.9.1
> >

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

[Chen, Tiejun]

On 2015/2/3 18:19, Wei Liu wrote:
> On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote:
>>
>> On 2015/2/2 20:19, Wei Liu wrote:
>>> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>>>> When we're working to support IGD GFX passthrough with qemu
>>>> upstream, instead of "-gfx_passthru" we'd like to make that
>>>> a machine option, "-machine xxx,-igd-passthru=on". This need
>>>> to bring a change on tool side.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>> v2:
>>>>
>>>> * Based on some discussions with Wei we'd like to keep both old
>>>>    option, -gfx_passthru, and new machine property option,
>>>>    "-machine xxx,-igd-passthru=on" at the same time but deprecate
>>>>    the old one. Then finally we remove the old one at that point
>>>>    that to give downstream (in this case, Xen) time to cope with the
>>>>    change.
>>>>
>>>
>>> My suggestion has one premise -- if upstream QEMU has already released
>>> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
>>> at all, then there is nothing to keep and deprecate.
>>
>> Understood.
>>
>>>
>>>>   tools/libxl/libxl_dm.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>>>> index c2b0487..8405f0b 100644
>>>> --- a/tools/libxl/libxl_dm.c
>>>> +++ b/tools/libxl/libxl_dm.c
>>>> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>>
>>> Note this function is upstream QEMU specfic.
>>
>> Yeah.
>>
>>>
>>>>               flexarray_append(dm_args, "-net");
>>>>               flexarray_append(dm_args, "none");
>>>>           }
>>>> +        /*
>>>> +         * Although we already introduce 'igd-passthru', but we'd like
>>>> +         * to remove this until we give downstream time to cope with
>>>> +         * the change.
>>>> +         */
>>>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>>>               flexarray_append(dm_args, "-gfx_passthru");
>>>>           }
>>>
>>> The comment contradicts what I know (or what I think I know). In our
>>> last email exchange you said there was no "-gfx_passthru" in any version
>>> of upstream QEMU.
>>>
>>> So, shouldn't you just remove this `if' statement?
>>
>> Right. So what about this?
>>
>>      libxl: add one machine property to support IGD GFX passthrough
>>
>>      When we're working to support IGD GFX passthrough with qemu
>>      upstream, we'd like to introduce a machine option,
>>      "-machine xxx,igd-passthru=on", to enable/disable that feature.
>>      And we also remove that old option, "-gfx_passthru", just from
>>      the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>>      no any qemu stream version really need or use that.
>>
>>      Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>
>
> Yes. I think a patch like this reflects the reality.
>
> It would be nice, as Ian J suggested, to state which version of QEMU
> upstream introduces that new option in commit message.

I have to provide this exact info after those qemu stuffs are merged, 
but now at lease you guys agree this option named as 'igd-passthrough', 
its enough to first continue working all qemu patches out. Once its 
finished I will go back.

>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index c2b0487..b888f19 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -701,9 +701,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");
>> @@ -748,6 +745,11 @@ static char **
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            machinearg = libxl__sprintf(gc, "%s,igd-passthru=on",
>> machinearg);
>> +        }
>> +
>
> Please use GCSPRINTF macro.
>

Sure.

@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            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]);


Thanks
Tiejun

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

[Chen, Tiejun]

On 2015/2/3 18:19, Wei Liu wrote:
> On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote:
>>
>> On 2015/2/2 20:19, Wei Liu wrote:
>>> On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
>>>> When we're working to support IGD GFX passthrough with qemu
>>>> upstream, instead of "-gfx_passthru" we'd like to make that
>>>> a machine option, "-machine xxx,-igd-passthru=on". This need
>>>> to bring a change on tool side.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> ---
>>>> v2:
>>>>
>>>> * Based on some discussions with Wei we'd like to keep both old
>>>>    option, -gfx_passthru, and new machine property option,
>>>>    "-machine xxx,-igd-passthru=on" at the same time but deprecate
>>>>    the old one. Then finally we remove the old one at that point
>>>>    that to give downstream (in this case, Xen) time to cope with the
>>>>    change.
>>>>
>>>
>>> My suggestion has one premise -- if upstream QEMU has already released
>>> that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
>>> at all, then there is nothing to keep and deprecate.
>>
>> Understood.
>>
>>>
>>>>   tools/libxl/libxl_dm.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>>>> index c2b0487..8405f0b 100644
>>>> --- a/tools/libxl/libxl_dm.c
>>>> +++ b/tools/libxl/libxl_dm.c
>>>> @@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>>>
>>> Note this function is upstream QEMU specfic.
>>
>> Yeah.
>>
>>>
>>>>               flexarray_append(dm_args, "-net");
>>>>               flexarray_append(dm_args, "none");
>>>>           }
>>>> +        /*
>>>> +         * Although we already introduce 'igd-passthru', but we'd like
>>>> +         * to remove this until we give downstream time to cope with
>>>> +         * the change.
>>>> +         */
>>>>           if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>>>>               flexarray_append(dm_args, "-gfx_passthru");
>>>>           }
>>>
>>> The comment contradicts what I know (or what I think I know). In our
>>> last email exchange you said there was no "-gfx_passthru" in any version
>>> of upstream QEMU.
>>>
>>> So, shouldn't you just remove this `if' statement?
>>
>> Right. So what about this?
>>
>>      libxl: add one machine property to support IGD GFX passthrough
>>
>>      When we're working to support IGD GFX passthrough with qemu
>>      upstream, we'd like to introduce a machine option,
>>      "-machine xxx,igd-passthru=on", to enable/disable that feature.
>>      And we also remove that old option, "-gfx_passthru", just from
>>      the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>>      no any qemu stream version really need or use that.
>>
>>      Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>
>
> Yes. I think a patch like this reflects the reality.
>
> It would be nice, as Ian J suggested, to state which version of QEMU
> upstream introduces that new option in commit message.

I have to provide this exact info after those qemu stuffs are merged, 
but now at lease you guys agree this option named as 'igd-passthrough', 
its enough to first continue working all qemu patches out. Once its 
finished I will go back.

>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index c2b0487..b888f19 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -701,9 +701,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");
>> @@ -748,6 +745,11 @@ static char **
>> libxl__build_device_model_args_new(libxl__gc *gc,
>>                                               machinearg, max_ram_below_4g);
>>               }
>>           }
>> +
>> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>> +            machinearg = libxl__sprintf(gc, "%s,igd-passthru=on",
>> machinearg);
>> +        }
>> +
>
> Please use GCSPRINTF macro.
>

Sure.

@@ -748,6 +745,11 @@ static char ** 
libxl__build_device_model_args_new(libxl__gc *gc,
                                              machinearg, max_ram_below_4g);
              }
          }
+
+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
+            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]);


Thanks
Tiejun

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

[Wei Liu]

On Tue, Feb 03, 2015 at 09:01:53AM +0800, Chen, Tiejun wrote:
> 
> On 2015/2/2 20:19, Wei Liu wrote:
> >On Mon, Feb 02, 2015 at 09:17:23AM +0800, Tiejun Chen wrote:
> >>When we're working to support IGD GFX passthrough with qemu
> >>upstream, instead of "-gfx_passthru" we'd like to make that
> >>a machine option, "-machine xxx,-igd-passthru=on". This need
> >>to bring a change on tool side.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>---
> >>v2:
> >>
> >>* Based on some discussions with Wei we'd like to keep both old
> >>   option, -gfx_passthru, and new machine property option,
> >>   "-machine xxx,-igd-passthru=on" at the same time but deprecate
> >>   the old one. Then finally we remove the old one at that point
> >>   that to give downstream (in this case, Xen) time to cope with the
> >>   change.
> >>
> >
> >My suggestion has one premise -- if upstream QEMU has already released
> >that -gfx_passthru option. If there is no "old one" (in upstream QEMU)
> >at all, then there is nothing to keep and deprecate.
> 
> Understood.
> 
> >
> >>  tools/libxl/libxl_dm.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >>diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> >>index c2b0487..8405f0b 100644
> >>--- a/tools/libxl/libxl_dm.c
> >>+++ b/tools/libxl/libxl_dm.c
> >>@@ -701,6 +701,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >
> >Note this function is upstream QEMU specfic.
> 
> Yeah.
> 
> >
> >>              flexarray_append(dm_args, "-net");
> >>              flexarray_append(dm_args, "none");
> >>          }
> >>+        /*
> >>+         * Although we already introduce 'igd-passthru', but we'd like
> >>+         * to remove this until we give downstream time to cope with
> >>+         * the change.
> >>+         */
> >>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >>              flexarray_append(dm_args, "-gfx_passthru");
> >>          }
> >
> >The comment contradicts what I know (or what I think I know). In our
> >last email exchange you said there was no "-gfx_passthru" in any version
> >of upstream QEMU.
> >
> >So, shouldn't you just remove this `if' statement?
> 
> Right. So what about this?
> 
>     libxl: add one machine property to support IGD GFX passthrough
> 
>     When we're working to support IGD GFX passthrough with qemu
>     upstream, we'd like to introduce a machine option,
>     "-machine xxx,igd-passthru=on", to enable/disable that feature.
>     And we also remove that old option, "-gfx_passthru", just from
>     the case of LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN since actually
>     no any qemu stream version really need or use that.
> 
>     Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> 

Yes. I think a patch like this reflects the reality.

It would be nice, as Ian J suggested, to state which version of QEMU
upstream introduces that new option in commit message.

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index c2b0487..b888f19 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -701,9 +701,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");
> @@ -748,6 +745,11 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                              machinearg, max_ram_below_4g);
>              }
>          }
> +
> +        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> +            machinearg = libxl__sprintf(gc, "%s,igd-passthru=on",
> machinearg);
> +        }
> +

Please use GCSPRINTF macro.

Wei.

>          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]);
> 

> Thanks
> Tiejun
> 
> >
> >Wei.
> >
> >>@@ -748,6 +753,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >>                                              machinearg, max_ram_below_4g);
> >>              }
> >>          }
> >>+
> >>+        if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
> >>+            machinearg = libxl__sprintf(gc, "%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]);
> >>--
> >>1.9.1
> >