Re: [libvirt PATCH] xenconfig: Add feature gfx_passthru

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 4/30/20 6:07 AM, Artur Puzio wrote:
> gfx_passthru xl.cfg option enables GPU specific quirks required for working
> Intel GPU passthru. Qemu (used for device model by xen) will refuse to start
> a VM when an IGD is passed, but this option was not set in Xen.

Do we really need to expose this setting to the user? I'm not really sure what 
to think about it after reading the xl.cfg(5) man page. It starts with

  gfx_passthru=BOOLEAN|"STRING"

The setting can be a boolean or a string - nice. And it really seems specific to 
Intel graphics cards. The man page claims that a value of 1 "Enables graphics 
device PCI passthrough and autodetects the type of device which is being used.". 
It also says "Note that some graphics cards (AMD/ATI cards, for example) do not 
necessarily require the gfx_passthru option".

Can't libxl just enable this itself if there's a PCI passthrough device and it's 
detected as type igd?

Regards,
Jim

> Signed-off-by: Artur Puzio <contact@xxxxxxxxxxxx>
> ---
>   docs/formatdomain.html.in     |  7 +++++++
>   docs/schemas/domaincommon.rng |  5 +++++
>   src/conf/domain_conf.c        |  4 ++++
>   src/conf/domain_conf.h        |  1 +
>   src/libxl/libxl_conf.c        | 13 +++++++++++++
>   5 files changed, 30 insertions(+)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 91d6f6c0d3..5307844a23 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2064,6 +2064,7 @@
>     &lt;xen&gt;
>       &lt;e820_host state='on'/&gt;
>       &lt;passthrough state='on' mode='share_pt'/&gt;
> +    &lt;gfx_passthru state='on'/&gt;
>     &lt;/xen&gt;
>     &lt;pvspinlock state='on'/&gt;
>     &lt;gic version='2'/&gt;
> @@ -2270,6 +2271,12 @@
>             <td>on, off; mode - optional string sync_pt or share_pt</td>
>             <td><span class="since">6.3.0</span></td>
>           </tr>
> +        <tr>
> +          <td>gfx_passthru</td>
> +          <td>Enable Intel GPU specific quirks. Required and allowed only when passing an IGD.</td>
> +          <td>on, off</td>
> +          <td><span class="since">6.3.0</span></td>
> +        </tr>
>         </table>
>         </dd>
>         <dt><code>pmu</code></dt>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 9d60b090f3..7d8ea879a1 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -6409,6 +6409,11 @@
>               </optional>
>             </element>
>           </optional>
> +        <optional>
> +          <element name="gfx_passthru">
> +            <ref name="featurestate"/>
> +          </element>
> +        </optional>
>         </interleave>
>       </element>
>     </define>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8a87586936..75f72ff64c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -214,6 +214,7 @@ VIR_ENUM_IMPL(virDomainXen,
>                 VIR_DOMAIN_XEN_LAST,
>                 "e820_host",
>                 "passthrough",
> +              "gfx_passthru"
>   );
>   
>   VIR_ENUM_IMPL(virDomainXenPassthroughMode,
> @@ -19649,6 +19650,7 @@ virDomainFeaturesDefParse(virDomainDefPtr def,
>   
>               switch ((virDomainXen) feature) {
>                   case VIR_DOMAIN_XEN_E820_HOST:
> +                case VIR_DOMAIN_XEN_GFX_PASSTHRU:
>                       break;
>   
>               case VIR_DOMAIN_XEN_PASSTHROUGH:
> @@ -23579,6 +23581,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>               }
>               switch ((virDomainXen) i) {
>               case VIR_DOMAIN_XEN_E820_HOST:
> +            case VIR_DOMAIN_XEN_GFX_PASSTHRU:
>                   break;
>   
>               case VIR_DOMAIN_XEN_PASSTHROUGH:
> @@ -29235,6 +29238,7 @@ virDomainDefFormatFeatures(virBufferPtr buf,
>   
>                   switch ((virDomainXen) j) {
>                   case VIR_DOMAIN_XEN_E820_HOST:
> +                case VIR_DOMAIN_XEN_GFX_PASSTHRU:
>                       virBufferAddLit(&childBuf, "/>\n");
>                       break;
>                   case VIR_DOMAIN_XEN_PASSTHROUGH:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 4afd8f04bc..f28f0741ac 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1867,6 +1867,7 @@ typedef enum {
>   typedef enum {
>       VIR_DOMAIN_XEN_E820_HOST = 0,
>       VIR_DOMAIN_XEN_PASSTHROUGH,
> +    VIR_DOMAIN_XEN_GFX_PASSTHRU,
>   
>       VIR_DOMAIN_XEN_LAST
>   } virDomainXen;
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 458dfc2399..a5605f6200 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -679,6 +679,19 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>               return -1;
>           }
>   #endif
> +        if (def->features[VIR_DOMAIN_FEATURE_XEN] == VIR_TRISTATE_SWITCH_ON) {
> +            switch ((virTristateSwitch) def->xen_features[VIR_DOMAIN_XEN_GFX_PASSTHRU]) {
> +                case VIR_TRISTATE_SWITCH_ON:
> +                    libxl_defbool_set(&b_info->u.hvm.gfx_passthru, true);
> +                    break;
> +                case VIR_TRISTATE_SWITCH_OFF:
> +                    libxl_defbool_set(&b_info->u.hvm.gfx_passthru, false);
> +                    break;
> +                case VIR_TRISTATE_SWITCH_ABSENT:
> +                case VIR_TRISTATE_SWITCH_LAST:
> +                    break;
> +            }
> +        }
>       } else if (pvh) {
>           b_info->cmdline = g_strdup(def->os.cmdline);
>           b_info->kernel = g_strdup(def->os.kernel);