[PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Zhenzhong Duan]

"intel_iommu=off" command line is used to disable iommu and iommu is force
enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
could be used to force disable iommu in tboot for better performance.

By default kernel should panic if iommu init fail in tboot for security
reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".

Fix it by return 0 in tboot_force_iommu() so that force_on is not set.

Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
---
 arch/x86/kernel/tboot.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 992fb1415c0f..9fd4d162b331 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
 
 int tboot_force_iommu(void)
 {
-	if (!tboot_enabled())
+	if (!tboot_enabled() || intel_iommu_tboot_noforce)
 		return 0;
 
-	if (intel_iommu_tboot_noforce)
-		return 1;
-
 	if (no_iommu || swiotlb || dmar_disabled)
 		pr_warn("Forcing Intel-IOMMU to enabled\n");
 
-- 
2.25.1

Re: [PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Zhenzhong Duan]

+intel iommu maintainers,

Can anyone help review this patch? Thanks

Zhenzhong

On Wed, Nov 4, 2020 at 4:15 PM Zhenzhong Duan <zhenzhong.duan@gmail.com> wrote:
>
> "intel_iommu=off" command line is used to disable iommu and iommu is force
> enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
> could be used to force disable iommu in tboot for better performance.
>
> By default kernel should panic if iommu init fail in tboot for security
> reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".
>
> Fix it by return 0 in tboot_force_iommu() so that force_on is not set.
>
> Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> ---
>  arch/x86/kernel/tboot.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> index 992fb1415c0f..9fd4d162b331 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
>
>  int tboot_force_iommu(void)
>  {
> -       if (!tboot_enabled())
> +       if (!tboot_enabled() || intel_iommu_tboot_noforce)
>                 return 0;
>
> -       if (intel_iommu_tboot_noforce)
> -               return 1;
> -
>         if (no_iommu || swiotlb || dmar_disabled)
>                 pr_warn("Forcing Intel-IOMMU to enabled\n");
>
> --
> 2.25.1
>

Re: [PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Zhenzhong Duan]

+intel iommu maintainers,

Can anyone help review this patch? Thanks

Zhenzhong

On Wed, Nov 4, 2020 at 4:15 PM Zhenzhong Duan <zhenzhong.duan@gmail.com> wrote:
>
> "intel_iommu=off" command line is used to disable iommu and iommu is force
> enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
> could be used to force disable iommu in tboot for better performance.
>
> By default kernel should panic if iommu init fail in tboot for security
> reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".
>
> Fix it by return 0 in tboot_force_iommu() so that force_on is not set.
>
> Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> ---
>  arch/x86/kernel/tboot.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> index 992fb1415c0f..9fd4d162b331 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
>
>  int tboot_force_iommu(void)
>  {
> -       if (!tboot_enabled())
> +       if (!tboot_enabled() || intel_iommu_tboot_noforce)
>                 return 0;
>
> -       if (intel_iommu_tboot_noforce)
> -               return 1;
> -
>         if (no_iommu || swiotlb || dmar_disabled)
>                 pr_warn("Forcing Intel-IOMMU to enabled\n");
>
> --
> 2.25.1
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Lu Baolu]

Hi Zhenzhong,

On 11/9/20 10:27 AM, Zhenzhong Duan wrote:
> +intel iommu maintainers,
> 
> Can anyone help review this patch? Thanks
> 
> Zhenzhong
> 
> On Wed, Nov 4, 2020 at 4:15 PM Zhenzhong Duan <zhenzhong.duan@gmail.com> wrote:
>>
>> "intel_iommu=off" command line is used to disable iommu and iommu is force
>> enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
>> could be used to force disable iommu in tboot for better performance.
>>
>> By default kernel should panic if iommu init fail in tboot for security
>> reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".
>>
>> Fix it by return 0 in tboot_force_iommu() so that force_on is not set.
>>
>> Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
>> ---
>>   arch/x86/kernel/tboot.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
>> index 992fb1415c0f..9fd4d162b331 100644
>> --- a/arch/x86/kernel/tboot.c
>> +++ b/arch/x86/kernel/tboot.c
>> @@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
>>
>>   int tboot_force_iommu(void)
>>   {
>> -       if (!tboot_enabled())
>> +       if (!tboot_enabled() || intel_iommu_tboot_noforce)
>>                  return 0;
>>
>> -       if (intel_iommu_tboot_noforce)
>> -               return 1;

This was obviously wrong. It should return false, right?

It looks odd that intel_iommu_tboot_noforce is defined in Intel iommu
implementation, but is used here. How about moving it back to the iommu
driver?

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 992fb1415c0f..420be871d9d4 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -514,9 +514,6 @@ int tboot_force_iommu(void)
         if (!tboot_enabled())
                 return 0;

-       if (intel_iommu_tboot_noforce)
-               return 1;
-
         if (no_iommu || swiotlb || dmar_disabled)
                 pr_warn("Forcing Intel-IOMMU to enabled\n");

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a41f9f13cc86..ba90eb4325d0 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -179,7 +179,7 @@ static int rwbf_quirk;
   * (used when kernel is launched w/ TXT)
   */
  static int force_on = 0;
-int intel_iommu_tboot_noforce;
+static int intel_iommu_tboot_noforce;
  static int no_platform_optin;

  #define ROOT_ENTRY_NR (VTD_PAGE_SIZE/sizeof(struct root_entry))
@@ -4212,7 +4212,8 @@ int __init intel_iommu_init(void)
          * Intel IOMMU is required for a TXT/tboot launch or platform
          * opt in, so enforce that.
          */
-       force_on = tboot_force_iommu() || platform_optin_force_iommu();
+       force_on = (!intel_iommu_tboot_noforce && tboot_force_iommu()) ||
+                       platform_optin_force_iommu();

         if (iommu_init_mempool()) {
                 if (force_on)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index fbf5b3e7707e..d956987ed032 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -798,7 +798,6 @@ extern int iommu_calculate_agaw(struct intel_iommu 
*iommu);
  extern int iommu_calculate_max_sagaw(struct intel_iommu *iommu);
  extern int dmar_disabled;
  extern int intel_iommu_enabled;
-extern int intel_iommu_tboot_noforce;
  extern int intel_iommu_gfx_mapped;
  #else
  static inline int iommu_calculate_agaw(struct intel_iommu *iommu)

>> -
>>          if (no_iommu || swiotlb || dmar_disabled)
>>                  pr_warn("Forcing Intel-IOMMU to enabled\n");
>>
>> --
>> 2.25.1
>>

Best regards,
baolu

Re: [PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Lu Baolu]

Hi Zhenzhong,

On 11/9/20 10:27 AM, Zhenzhong Duan wrote:
> +intel iommu maintainers,
> 
> Can anyone help review this patch? Thanks
> 
> Zhenzhong
> 
> On Wed, Nov 4, 2020 at 4:15 PM Zhenzhong Duan <zhenzhong.duan@gmail.com> wrote:
>>
>> "intel_iommu=off" command line is used to disable iommu and iommu is force
>> enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
>> could be used to force disable iommu in tboot for better performance.
>>
>> By default kernel should panic if iommu init fail in tboot for security
>> reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".
>>
>> Fix it by return 0 in tboot_force_iommu() so that force_on is not set.
>>
>> Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
>> ---
>>   arch/x86/kernel/tboot.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
>> index 992fb1415c0f..9fd4d162b331 100644
>> --- a/arch/x86/kernel/tboot.c
>> +++ b/arch/x86/kernel/tboot.c
>> @@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
>>
>>   int tboot_force_iommu(void)
>>   {
>> -       if (!tboot_enabled())
>> +       if (!tboot_enabled() || intel_iommu_tboot_noforce)
>>                  return 0;
>>
>> -       if (intel_iommu_tboot_noforce)
>> -               return 1;

This was obviously wrong. It should return false, right?

It looks odd that intel_iommu_tboot_noforce is defined in Intel iommu
implementation, but is used here. How about moving it back to the iommu
driver?

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 992fb1415c0f..420be871d9d4 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -514,9 +514,6 @@ int tboot_force_iommu(void)
         if (!tboot_enabled())
                 return 0;

-       if (intel_iommu_tboot_noforce)
-               return 1;
-
         if (no_iommu || swiotlb || dmar_disabled)
                 pr_warn("Forcing Intel-IOMMU to enabled\n");

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a41f9f13cc86..ba90eb4325d0 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -179,7 +179,7 @@ static int rwbf_quirk;
   * (used when kernel is launched w/ TXT)
   */
  static int force_on = 0;
-int intel_iommu_tboot_noforce;
+static int intel_iommu_tboot_noforce;
  static int no_platform_optin;

  #define ROOT_ENTRY_NR (VTD_PAGE_SIZE/sizeof(struct root_entry))
@@ -4212,7 +4212,8 @@ int __init intel_iommu_init(void)
          * Intel IOMMU is required for a TXT/tboot launch or platform
          * opt in, so enforce that.
          */
-       force_on = tboot_force_iommu() || platform_optin_force_iommu();
+       force_on = (!intel_iommu_tboot_noforce && tboot_force_iommu()) ||
+                       platform_optin_force_iommu();

         if (iommu_init_mempool()) {
                 if (force_on)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index fbf5b3e7707e..d956987ed032 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -798,7 +798,6 @@ extern int iommu_calculate_agaw(struct intel_iommu 
*iommu);
  extern int iommu_calculate_max_sagaw(struct intel_iommu *iommu);
  extern int dmar_disabled;
  extern int intel_iommu_enabled;
-extern int intel_iommu_tboot_noforce;
  extern int intel_iommu_gfx_mapped;
  #else
  static inline int iommu_calculate_agaw(struct intel_iommu *iommu)

>> -
>>          if (no_iommu || swiotlb || dmar_disabled)
>>                  pr_warn("Forcing Intel-IOMMU to enabled\n");
>>
>> --
>> 2.25.1
>>

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Zhenzhong Duan]

Hi Baolu,

On Mon, Nov 9, 2020 at 11:15 AM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> Hi Zhenzhong,
>
> On 11/9/20 10:27 AM, Zhenzhong Duan wrote:
> > +intel iommu maintainers,
> >
> > Can anyone help review this patch? Thanks
> >
> > Zhenzhong
> >
> > On Wed, Nov 4, 2020 at 4:15 PM Zhenzhong Duan <zhenzhong.duan@gmail.com> wrote:
> >>
> >> "intel_iommu=off" command line is used to disable iommu and iommu is force
> >> enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
> >> could be used to force disable iommu in tboot for better performance.
> >>
> >> By default kernel should panic if iommu init fail in tboot for security
> >> reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".
> >>
> >> Fix it by return 0 in tboot_force_iommu() so that force_on is not set.
> >>
> >> Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> >> ---
> >>   arch/x86/kernel/tboot.c | 5 +----
> >>   1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> >> index 992fb1415c0f..9fd4d162b331 100644
> >> --- a/arch/x86/kernel/tboot.c
> >> +++ b/arch/x86/kernel/tboot.c
> >> @@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
> >>
> >>   int tboot_force_iommu(void)
> >>   {
> >> -       if (!tboot_enabled())
> >> +       if (!tboot_enabled() || intel_iommu_tboot_noforce)
> >>                  return 0;
> >>
> >> -       if (intel_iommu_tboot_noforce)
> >> -               return 1;
>
> This was obviously wrong. It should return false, right?

I guess you missed above change: "if (!tboot_enabled() ||
intel_iommu_tboot_noforce)".
It does return false.

>
> It looks odd that intel_iommu_tboot_noforce is defined in Intel iommu
> implementation, but is used here. How about moving it back to the iommu
> driver?

That's better, will do. Thanks for your suggestion.

Zhenzhong

Re: [PATCH] iommu/vt-d: avoid unnecessory panic if iommu init fail in tboot

[Zhenzhong Duan]

Hi Baolu,

On Mon, Nov 9, 2020 at 11:15 AM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> Hi Zhenzhong,
>
> On 11/9/20 10:27 AM, Zhenzhong Duan wrote:
> > +intel iommu maintainers,
> >
> > Can anyone help review this patch? Thanks
> >
> > Zhenzhong
> >
> > On Wed, Nov 4, 2020 at 4:15 PM Zhenzhong Duan <zhenzhong.duan@gmail.com> wrote:
> >>
> >> "intel_iommu=off" command line is used to disable iommu and iommu is force
> >> enabled in a tboot system. Meanwhile "intel_iommu=tboot_noforce,off"
> >> could be used to force disable iommu in tboot for better performance.
> >>
> >> By default kernel should panic if iommu init fail in tboot for security
> >> reason, but it's unnecessory if we use "intel_iommu=tboot_noforce,off".
> >>
> >> Fix it by return 0 in tboot_force_iommu() so that force_on is not set.
> >>
> >> Fixes: 7304e8f28bb2 ("iommu/vt-d: Correctly disable Intel IOMMU force on")
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
> >> ---
> >>   arch/x86/kernel/tboot.c | 5 +----
> >>   1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
> >> index 992fb1415c0f..9fd4d162b331 100644
> >> --- a/arch/x86/kernel/tboot.c
> >> +++ b/arch/x86/kernel/tboot.c
> >> @@ -511,12 +511,9 @@ struct acpi_table_header *tboot_get_dmar_table(struct acpi_table_header *dmar_tb
> >>
> >>   int tboot_force_iommu(void)
> >>   {
> >> -       if (!tboot_enabled())
> >> +       if (!tboot_enabled() || intel_iommu_tboot_noforce)
> >>                  return 0;
> >>
> >> -       if (intel_iommu_tboot_noforce)
> >> -               return 1;
>
> This was obviously wrong. It should return false, right?

I guess you missed above change: "if (!tboot_enabled() ||
intel_iommu_tboot_noforce)".
It does return false.

>
> It looks odd that intel_iommu_tboot_noforce is defined in Intel iommu
> implementation, but is used here. How about moving it back to the iommu
> driver?

That's better, will do. Thanks for your suggestion.

Zhenzhong
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu