Re: [Qemu-devel] [v3][PATCH 2/2] libxl: introduce gfx_passthru_kind

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/3/24 22:50, Ian Campbell wrote:
> On Mon, 2015-03-23 at 09:17 +0800, Tiejun Chen wrote:
>> Although we already have 'gfx_passthru' in b_info, this doesn' suffice
>

Fixed.

>                                                                  ^t
>
>> after we want to handle IGD specifically. Now we define a new field of
>> type, gfx_passthru_kind, to indicate we're trying to pass IGD. Actually
>> this means we can benefit this to support other specific devices just
>> by extending gfx_passthru_kind. And then we can cooperate with
>> gfx_passthru to address IGD cases as follows:
>>
>>      gfx_passthru = 0    => sets build_info.u.gfx_passthru to false
>>      gfx_passthru = 1    => sets build_info.u.gfx_passthru to true and
>>                             build_info.u.gfx_passthru_kind to DEFAULT
>>      gfx_passthru = "igd"    => sets build_info.u.gfx_passthru to false
>                                                                     true
>
> You had it right in the code.

Fixed.

>
>>                                 and build_info.u.gfx_passthru_kind to IGD
>>

[snip]

>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
>>   devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
>>   above.
>>
>> -=item B<gfx_passthru=BOOLEAN>
>> +=item B<gfx_passthru="STRING">
>
> I think B<gfx_passthru=BOOLEAN|"STRING"> is more accurate.

Yeah, it make more sense.

>
>>   Enable graphics device PCI passthrough. This option makes an assigned
>>   PCI graphics card become primary graphics card in the VM. The QEMU
>> @@ -699,9 +699,12 @@ working graphics passthrough. See the XenVGAPassthroughTestedAdapters
>>   L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
>>   for currently supported graphics cards for gfx_passthru.
>>
>> -gfx_passthru is currently only supported with the qemu-xen-traditional
>> -device-model. Upstream qemu-xen device-model currently does not have
>> -support for gfx_passthru.
>> +gfx_passthru is currently supported both with the qemu-xen-traditional
>> +device-model and upstream qemu-xen device-model. Note with the
>> +qemu-xen-traditional device-model this option is just treated as BOOLEAN
>> +actually, but with upstream qemu-xen device-model this option is extended
>> +to pass a specific device name to force work. Currently just 'igd' is
>> +defined to support Intel graphics device.
>
> How about:
>
>          When given as a boolean the B<gfx_passthru> option either
>          disables gfx passthru or enables autodetection.
>
>          When given as a string the B<gfx_passthru> option describes the
>          type of passthru to enable.
>
>          Valid options are:
>
>          =over 4
>
>          =item B<gfx_passthru=0>
>
>          Disables gfx_passthru.
>
>          =item B<gfx_passthru=1>, B<gfx_passthru="default">
>
>          Enables gfx_passthru and autodetects the type of device which is
>          being used.
>
>          =item "igd"
>
>          Enables gfx_passthru of the Intel Graphics Device.
>
>          =back
>
>          Note: When used with the qemu-xen-traditional device model only
>          IGD passthru is supported.
>
> (do check my pod syntax, I'm mostly making it up!)

Please take a look at this,

@@ -671,7 +671,7 @@ through to this VM. See L<seize|/"seize_boolean"> above.
  devices passed through to this VM. See L<power_mgt|/"power_mgmt_boolean">
  above.

-=item B<gfx_passthru=BOOLEAN>
+=item B<gfx_passthru=BOOLEAN|"STRING">

  Enable graphics device PCI passthrough. This option makes an assigned
  PCI graphics card become primary graphics card in the VM. The QEMU
@@ -699,9 +699,35 @@ working graphics passthrough. See the 
XenVGAPassthroughTestedAdapters
  L<http://wiki.xen.org/wiki/XenVGAPassthroughTestedAdapters> wiki page
  for currently supported graphics cards for gfx_passthru.

-gfx_passthru is currently only supported with the qemu-xen-traditional
-device-model. Upstream qemu-xen device-model currently does not have
-support for gfx_passthru.
+gfx_passthru is currently supported both with the qemu-xen-traditional
+device-model and upstream qemu-xen device-model.
+
+When given as a boolean the B<gfx_passthru> option either disables gfx
+passthru or enables autodetection.
+
+But when given as a string the B<gfx_passthru> option describes the type
+of device to enable. Not this behavior is only supported with upstream
+qemu-xen device-model.
+
+Currently, valid options are:
+
+=over 4
+
+=item B<gfx_passthru=0>
+
+Disables graphics device PCI passthrough.
+
+=item B<gfx_passthru=1>, B<gfx_passthru="default">
+
+Enables graphics device PCI passthrough and autodetects the type of device
+which is being used.
+
+=item "igd"
+
+Enables graphics device PCI passthrough but force set the type of device
+with the Intel Graphics Device.
+
+=back

  Note that some graphics adapters (AMD/ATI cards, for example) do not
  necessarily require gfx_passthru option, so you can use the normal Xen

>
> The note at the end makes me thing that perhaps something ought to check
> this constraint in the qemu-xen-traditional case. It might be easiest to

I understand what you mean but that table just includes IGDs existed on 
BDW and HSW. Because in the case of qemu upstream we're just covering 
these platforms, and with our discussion we don't have any plan to add 
those legacy platforms in the future. But qemu-xen-traditional still 
covers those platforms. So I'm afraid its not good to check this with 
that table as well.

> do it in libxl__build_device_model_args_old using the
> libxl__detect_gfx_passthru_kind helper you have added
>
> e.g. where libxl__build_device_model_args_old has:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
> would become something like:
>          if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
>              if (libxl__detect_gfx_passthru_kind(gc, guest_config) !=
>                  LIBXL_GFX_PASSTHRU_KIND_IGD) {
>                  LOG("only IGD gfx_passthru is supported with qemu-xen-traditional");
>                  return NULL;
>              }
>              flexarray_append(dm_args, "-gfx_passthru");
>          }
>
> Or something like that. What do you think?
>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index c97c62d..26a01cb 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -3632,6 +3632,10 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
>>    */
>>   void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>>                                       const libxl_bitmap *sptr);
>> +
>> +bool libxl__is_igd_vga_passthru(libxl__gc *gc,
>> +                                const libxl_domain_config *d_config);
>
> This should be in the previous patch. In fact I think it is and this is
> a redundant second instance.

Yes, we can remove this completely.

Thanks
Tiejun