[PATCH] VT-d: don't panic/warn on iommu=no-igfx

[PATCH] VT-d: don't panic/warn on iommu=no-igfx

[Rusty Bird]

When operating on an Intel graphics device, iommu_enable_translation()
panicked (force_iommu==1) or warned (force_iommu==0) about the BIOS if
is_igd_vt_enabled_quirk() returned 0. That's good if the actual BIOS
problem has been detected. But since commit 1463411, returning 0 could
also happen if the user simply passed "iommu=no-igfx", in which case
bailing out _without_ the panic/warning would be more appropriate.

The panic broke the combination "iommu=force,no-igfx", and also the case
where "iommu=no-igfx" is passed but force_iommu=1 is set automatically
by x2apic_bsp_setup().

Move the iommu_igfx check from is_igd_vt_enabled_quirk() into its only
caller iommu_enable_translation(), and tweak the logic.

Signed-off-by: Rusty Bird <rustybird@openmailbox.org>
---

Notes:
    Best viewed with "git show --ignore-space-change --function-context"

 xen/drivers/passthrough/vtd/iommu.c  | 18 ++++++++++++------
 xen/drivers/passthrough/vtd/quirks.c |  3 ---
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 19328f6..2849ea1 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -747,14 +747,20 @@ static void iommu_enable_translation(struct acpi_drhd_unit *drhd)
     unsigned long flags;
     struct iommu *iommu = drhd->iommu;
 
-    if ( is_igd_drhd(drhd) && !is_igd_vt_enabled_quirk() ) 
+    if ( is_igd_drhd(drhd) )
     {
-        if ( force_iommu )
-            panic("BIOS did not enable IGD for VT properly, crash Xen for security purpose");
+        if ( !iommu_igfx )
+            return;
 
-        printk(XENLOG_WARNING VTDPREFIX
-               "BIOS did not enable IGD for VT properly.  Disabling IGD VT-d engine.\n");
-        return;
+        if ( !is_igd_vt_enabled_quirk() )
+        {
+            if ( force_iommu )
+                panic("BIOS did not enable IGD for VT properly, crash Xen for security purpose");
+
+            printk(XENLOG_WARNING VTDPREFIX
+                   "BIOS did not enable IGD for VT properly.  Disabling IGD VT-d engine.\n");
+            return;
+        }
     }
 
     /* apply platform specific errata workarounds */
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 91f96ac..5bbbd96 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -70,9 +70,6 @@ int is_igd_vt_enabled_quirk(void)
 {
     u16 ggc;
 
-    if ( !iommu_igfx )
-        return 0;
-
     if ( !IS_ILK(ioh_id) )
         return 1;
 
-- 
2.9.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: don't panic/warn on iommu=no-igfx

[Tian, Kevin]

> From: Rusty Bird [mailto:rustybird@openmailbox.org]
> Sent: Thursday, July 27, 2017 7:54 PM
> 
> When operating on an Intel graphics device, iommu_enable_translation()
> panicked (force_iommu==1) or warned (force_iommu==0) about the BIOS if
> is_igd_vt_enabled_quirk() returned 0. That's good if the actual BIOS
> problem has been detected. But since commit 1463411, returning 0 could
> also happen if the user simply passed "iommu=no-igfx", in which case
> bailing out _without_ the panic/warning would be more appropriate.
> 
> The panic broke the combination "iommu=force,no-igfx", and also the case
> where "iommu=no-igfx" is passed but force_iommu=1 is set automatically
> by x2apic_bsp_setup().
> 
> Move the iommu_igfx check from is_igd_vt_enabled_quirk() into its only
> caller iommu_enable_translation(), and tweak the logic.
> 
> Signed-off-by: Rusty Bird <rustybird@openmailbox.org>
> ---
> 
> Notes:
>     Best viewed with "git show --ignore-space-change --function-context"
> 
>  xen/drivers/passthrough/vtd/iommu.c  | 18 ++++++++++++------
>  xen/drivers/passthrough/vtd/quirks.c |  3 ---
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 19328f6..2849ea1 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -747,14 +747,20 @@ static void iommu_enable_translation(struct
> acpi_drhd_unit *drhd)
>      unsigned long flags;
>      struct iommu *iommu = drhd->iommu;
> 
> -    if ( is_igd_drhd(drhd) && !is_igd_vt_enabled_quirk() )
> +    if ( is_igd_drhd(drhd) )
>      {
> -        if ( force_iommu )
> -            panic("BIOS did not enable IGD for VT properly, crash Xen for security
> purpose");
> +        if ( !iommu_igfx )
> +            return;

A message might be also helpful here so user can confirm its
boot option takes effect...

> 
> -        printk(XENLOG_WARNING VTDPREFIX
> -               "BIOS did not enable IGD for VT properly.  Disabling IGD VT-d
> engine.\n");
> -        return;
> +        if ( !is_igd_vt_enabled_quirk() )
> +        {
> +            if ( force_iommu )
> +                panic("BIOS did not enable IGD for VT properly, crash Xen for
> security purpose");
> +
> +            printk(XENLOG_WARNING VTDPREFIX
> +                   "BIOS did not enable IGD for VT properly.  Disabling IGD VT-d
> engine.\n");
> +            return;
> +        }
>      }
> 
>      /* apply platform specific errata workarounds */
> diff --git a/xen/drivers/passthrough/vtd/quirks.c
> b/xen/drivers/passthrough/vtd/quirks.c
> index 91f96ac..5bbbd96 100644
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -70,9 +70,6 @@ int is_igd_vt_enabled_quirk(void)
>  {
>      u16 ggc;
> 
> -    if ( !iommu_igfx )
> -        return 0;
> -
>      if ( !IS_ILK(ioh_id) )
>          return 1;
> 
> --
> 2.9.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: don't panic/warn on iommu=no-igfx

[Rusty Bird]

[-- Attachment #1.1: Type: text/plain, Size: 286 bytes --]

Tian, Kevin:
> > From: Rusty Bird [mailto:rustybird@openmailbox.org]
> > +        if ( !iommu_igfx )
> > +            return;
> 
> A message might be also helpful here so user can confirm its
> boot option takes effect...

Done, see v2 patch. Thanks for the feedback!

Rusty

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 931 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel