[PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Suravee Suthikulpanit]

This bug is triggered when rebooting VM on a system which
SVM AVIC is enabled but IOMMU AVIC is disabled in the BIOS.

The series reworks interrupt remapping intialiation to
check for IOMMU AVIC support (GAMSup) at earlier stage using
EFR provided by IVRS table instead of the PCI MMIO register,
which is available after PCI support for IOMMU is initialized.
This helps avoid having to disable and clean up the already
initialized interrupt-remapping-related parameter. 

Thanks,
Suravee

Suravee Suthikulpanit (2):
  iommu/amd: Introduce helper function to check feature bit on all
    IOMMUs
  iommu/amd: Remove iommu_init_ga()

Wei Huang (1):
  iommu/amd: Relocate GAMSup check to early_enable_iommus

 drivers/iommu/amd/init.c | 45 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Suravee Suthikulpanit via iommu]

This bug is triggered when rebooting VM on a system which
SVM AVIC is enabled but IOMMU AVIC is disabled in the BIOS.

The series reworks interrupt remapping intialiation to
check for IOMMU AVIC support (GAMSup) at earlier stage using
EFR provided by IVRS table instead of the PCI MMIO register,
which is available after PCI support for IOMMU is initialized.
This helps avoid having to disable and clean up the already
initialized interrupt-remapping-related parameter. 

Thanks,
Suravee

Suravee Suthikulpanit (2):
  iommu/amd: Introduce helper function to check feature bit on all
    IOMMUs
  iommu/amd: Remove iommu_init_ga()

Wei Huang (1):
  iommu/amd: Relocate GAMSup check to early_enable_iommus

 drivers/iommu/amd/init.c | 45 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

-- 
2.17.1

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

[PATCH 1/3] iommu/amd: Introduce helper function to check feature bit on all IOMMUs

[Suravee Suthikulpanit]

IOMMU advertises feature via Extended Features Register (EFR).
The helper function checks if the specified feature bit is set
across all IOMMUs.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..c97961451ac5 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -298,6 +298,19 @@ int amd_iommu_get_num_iommus(void)
 	return amd_iommus_present;
 }
 
+static bool check_feature_on_all_iommus(u64 mask)
+{
+	bool ret = false;
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		ret = iommu_feature(iommu, mask);
+		if (!ret)
+			return false;
+	}
+
+	return true;
+}
 /*
  * For IVHD type 0x11/0x40, EFR is also available via IVHD.
  * Default to IVHD EFR since it is available sooner
-- 
2.17.1

[PATCH 1/3] iommu/amd: Introduce helper function to check feature bit on all IOMMUs

[Suravee Suthikulpanit via iommu]

IOMMU advertises feature via Extended Features Register (EFR).
The helper function checks if the specified feature bit is set
across all IOMMUs.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..c97961451ac5 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -298,6 +298,19 @@ int amd_iommu_get_num_iommus(void)
 	return amd_iommus_present;
 }
 
+static bool check_feature_on_all_iommus(u64 mask)
+{
+	bool ret = false;
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		ret = iommu_feature(iommu, mask);
+		if (!ret)
+			return false;
+	}
+
+	return true;
+}
 /*
  * For IVHD type 0x11/0x40, EFR is also available via IVHD.
  * Default to IVHD EFR since it is available sooner
-- 
2.17.1

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

[PATCH 2/3] iommu/amd: Relocate GAMSup check to early_enable_iommus

[Suravee Suthikulpanit]

From: Wei Huang <wei.huang2@amd.com>

Currently, iommu_init_ga() checks and disables IOMMU VAPIC support
(i.e. AMD AVIC support in IOMMU) when GAMSup feature bit is not set.
However it forgets to clear IRQ_POSTING_CAP from the previously set
amd_iommu_irq_ops.capability.

This triggers an invalid page fault bug during guest VM warm reboot
if AVIC is enabled since the irq_remapping_cap(IRQ_POSTING_CAP) is
incorrectly set, and crash the system with the following kernel trace.

    BUG: unable to handle page fault for address: 0000000000400dd8
    RIP: 0010:amd_iommu_deactivate_guest_mode+0x19/0xbc
    Call Trace:
     svm_set_pi_irte_mode+0x8a/0xc0 [kvm_amd]
     ? kvm_make_all_cpus_request_except+0x50/0x70 [kvm]
     kvm_request_apicv_update+0x10c/0x150 [kvm]
     svm_toggle_avic_for_irq_window+0x52/0x90 [kvm_amd]
     svm_enable_irq_window+0x26/0xa0 [kvm_amd]
     vcpu_enter_guest+0xbbe/0x1560 [kvm]
     ? avic_vcpu_load+0xd5/0x120 [kvm_amd]
     ? kvm_arch_vcpu_load+0x76/0x240 [kvm]
     ? svm_get_segment_base+0xa/0x10 [kvm_amd]
     kvm_arch_vcpu_ioctl_run+0x103/0x590 [kvm]
     kvm_vcpu_ioctl+0x22a/0x5d0 [kvm]
     __x64_sys_ioctl+0x84/0xc0
     do_syscall_64+0x33/0x40
     entry_SYSCALL_64_after_hwframe+0x44/0xae

Fixes by moving the initializing of AMD IOMMU interrupt remapping mode
(amd_iommu_guest_ir) earlier before setting up the
amd_iommu_irq_ops.capability with appropriate IRQ_POSTING_CAP flag.

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index c97961451ac5..ea3330ed545d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -867,13 +867,6 @@ static int iommu_init_ga(struct amd_iommu *iommu)
 	int ret = 0;
 
 #ifdef CONFIG_IRQ_REMAP
-	/* Note: We have already checked GASup from IVRS table.
-	 *       Now, we need to make sure that GAMSup is set.
-	 */
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
-	    !iommu_feature(iommu, FEATURE_GAM_VAPIC))
-		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
-
 	ret = iommu_init_ga_log(iommu);
 #endif /* CONFIG_IRQ_REMAP */
 
@@ -2490,6 +2483,14 @@ static void early_enable_iommus(void)
 	}
 
 #ifdef CONFIG_IRQ_REMAP
+	/*
+	 * Note: We have already checked GASup from IVRS table.
+	 *       Now, we need to make sure that GAMSup is set.
+	 */
+	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
+	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
+		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
+
 	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
 		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
 #endif
-- 
2.17.1

[PATCH 2/3] iommu/amd: Relocate GAMSup check to early_enable_iommus

[Suravee Suthikulpanit via iommu]

From: Wei Huang <wei.huang2@amd.com>

Currently, iommu_init_ga() checks and disables IOMMU VAPIC support
(i.e. AMD AVIC support in IOMMU) when GAMSup feature bit is not set.
However it forgets to clear IRQ_POSTING_CAP from the previously set
amd_iommu_irq_ops.capability.

This triggers an invalid page fault bug during guest VM warm reboot
if AVIC is enabled since the irq_remapping_cap(IRQ_POSTING_CAP) is
incorrectly set, and crash the system with the following kernel trace.

    BUG: unable to handle page fault for address: 0000000000400dd8
    RIP: 0010:amd_iommu_deactivate_guest_mode+0x19/0xbc
    Call Trace:
     svm_set_pi_irte_mode+0x8a/0xc0 [kvm_amd]
     ? kvm_make_all_cpus_request_except+0x50/0x70 [kvm]
     kvm_request_apicv_update+0x10c/0x150 [kvm]
     svm_toggle_avic_for_irq_window+0x52/0x90 [kvm_amd]
     svm_enable_irq_window+0x26/0xa0 [kvm_amd]
     vcpu_enter_guest+0xbbe/0x1560 [kvm]
     ? avic_vcpu_load+0xd5/0x120 [kvm_amd]
     ? kvm_arch_vcpu_load+0x76/0x240 [kvm]
     ? svm_get_segment_base+0xa/0x10 [kvm_amd]
     kvm_arch_vcpu_ioctl_run+0x103/0x590 [kvm]
     kvm_vcpu_ioctl+0x22a/0x5d0 [kvm]
     __x64_sys_ioctl+0x84/0xc0
     do_syscall_64+0x33/0x40
     entry_SYSCALL_64_after_hwframe+0x44/0xae

Fixes by moving the initializing of AMD IOMMU interrupt remapping mode
(amd_iommu_guest_ir) earlier before setting up the
amd_iommu_irq_ops.capability with appropriate IRQ_POSTING_CAP flag.

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index c97961451ac5..ea3330ed545d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -867,13 +867,6 @@ static int iommu_init_ga(struct amd_iommu *iommu)
 	int ret = 0;
 
 #ifdef CONFIG_IRQ_REMAP
-	/* Note: We have already checked GASup from IVRS table.
-	 *       Now, we need to make sure that GAMSup is set.
-	 */
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
-	    !iommu_feature(iommu, FEATURE_GAM_VAPIC))
-		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
-
 	ret = iommu_init_ga_log(iommu);
 #endif /* CONFIG_IRQ_REMAP */
 
@@ -2490,6 +2483,14 @@ static void early_enable_iommus(void)
 	}
 
 #ifdef CONFIG_IRQ_REMAP
+	/*
+	 * Note: We have already checked GASup from IVRS table.
+	 *       Now, we need to make sure that GAMSup is set.
+	 */
+	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
+	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
+		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
+
 	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
 		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
 #endif
-- 
2.17.1

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

[PATCH 3/3] iommu/amd: Remove iommu_init_ga()

[Suravee Suthikulpanit]

Since the function has been simplified and only call iommu_init_ga_log(),
remove the function and replace with iommu_init_ga_log() instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index ea3330ed545d..5ec683675ff0 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -827,9 +827,9 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
 	return 0;
 }
 
-#ifdef CONFIG_IRQ_REMAP
 static int iommu_init_ga_log(struct amd_iommu *iommu)
 {
+#ifdef CONFIG_IRQ_REMAP
 	u64 entry;
 
 	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
@@ -859,18 +859,9 @@ static int iommu_init_ga_log(struct amd_iommu *iommu)
 err_out:
 	free_ga_log(iommu);
 	return -EINVAL;
-}
-#endif /* CONFIG_IRQ_REMAP */
-
-static int iommu_init_ga(struct amd_iommu *iommu)
-{
-	int ret = 0;
-
-#ifdef CONFIG_IRQ_REMAP
-	ret = iommu_init_ga_log(iommu);
+#else
+	return 0;
 #endif /* CONFIG_IRQ_REMAP */
-
-	return ret;
 }
 
 static int __init alloc_cwwb_sem(struct amd_iommu *iommu)
@@ -1852,7 +1843,7 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	if (iommu_feature(iommu, FEATURE_PPR) && alloc_ppr_log(iommu))
 		return -ENOMEM;
 
-	ret = iommu_init_ga(iommu);
+	ret = iommu_init_ga_log(iommu);
 	if (ret)
 		return ret;
 
-- 
2.17.1

[PATCH 3/3] iommu/amd: Remove iommu_init_ga()

[Suravee Suthikulpanit via iommu]

Since the function has been simplified and only call iommu_init_ga_log(),
remove the function and replace with iommu_init_ga_log() instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index ea3330ed545d..5ec683675ff0 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -827,9 +827,9 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
 	return 0;
 }
 
-#ifdef CONFIG_IRQ_REMAP
 static int iommu_init_ga_log(struct amd_iommu *iommu)
 {
+#ifdef CONFIG_IRQ_REMAP
 	u64 entry;
 
 	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
@@ -859,18 +859,9 @@ static int iommu_init_ga_log(struct amd_iommu *iommu)
 err_out:
 	free_ga_log(iommu);
 	return -EINVAL;
-}
-#endif /* CONFIG_IRQ_REMAP */
-
-static int iommu_init_ga(struct amd_iommu *iommu)
-{
-	int ret = 0;
-
-#ifdef CONFIG_IRQ_REMAP
-	ret = iommu_init_ga_log(iommu);
+#else
+	return 0;
 #endif /* CONFIG_IRQ_REMAP */
-
-	return ret;
 }
 
 static int __init alloc_cwwb_sem(struct amd_iommu *iommu)
@@ -1852,7 +1843,7 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	if (iommu_feature(iommu, FEATURE_PPR) && alloc_ppr_log(iommu))
 		return -ENOMEM;
 
-	ret = iommu_init_ga(iommu);
+	ret = iommu_init_ga_log(iommu);
 	if (ret)
 		return ret;
 
-- 
2.17.1

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

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Suthikulpanit, Suravee]

Joerg,

Here is an dditional tags for this series:

Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")

Are there any concerns with this series?

Thanks
Suravee

On 8/20/2021 3:29 PM, Suravee Suthikulpanit wrote:
> This bug is triggered when rebooting VM on a system which
> SVM AVIC is enabled but IOMMU AVIC is disabled in the BIOS.
> 
> The series reworks interrupt remapping intialiation to
> check for IOMMU AVIC support (GAMSup) at earlier stage using
> EFR provided by IVRS table instead of the PCI MMIO register,
> which is available after PCI support for IOMMU is initialized.
> This helps avoid having to disable and clean up the already
> initialized interrupt-remapping-related parameter.
> 
> Thanks,
> Suravee
> 
> Suravee Suthikulpanit (2):
>    iommu/amd: Introduce helper function to check feature bit on all
>      IOMMUs
>    iommu/amd: Remove iommu_init_ga()
> 
> Wei Huang (1):
>    iommu/amd: Relocate GAMSup check to early_enable_iommus
> 
>   drivers/iommu/amd/init.c | 45 ++++++++++++++++++++++------------------
>   1 file changed, 25 insertions(+), 20 deletions(-)
> 

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Suthikulpanit, Suravee via iommu]

Joerg,

Here is an dditional tags for this series:

Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")

Are there any concerns with this series?

Thanks
Suravee

On 8/20/2021 3:29 PM, Suravee Suthikulpanit wrote:
> This bug is triggered when rebooting VM on a system which
> SVM AVIC is enabled but IOMMU AVIC is disabled in the BIOS.
> 
> The series reworks interrupt remapping intialiation to
> check for IOMMU AVIC support (GAMSup) at earlier stage using
> EFR provided by IVRS table instead of the PCI MMIO register,
> which is available after PCI support for IOMMU is initialized.
> This helps avoid having to disable and clean up the already
> initialized interrupt-remapping-related parameter.
> 
> Thanks,
> Suravee
> 
> Suravee Suthikulpanit (2):
>    iommu/amd: Introduce helper function to check feature bit on all
>      IOMMUs
>    iommu/amd: Remove iommu_init_ga()
> 
> Wei Huang (1):
>    iommu/amd: Relocate GAMSup check to early_enable_iommus
> 
>   drivers/iommu/amd/init.c | 45 ++++++++++++++++++++++------------------
>   1 file changed, 25 insertions(+), 20 deletions(-)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Joerg Roedel]

Hi Suravee,

On Tue, Aug 31, 2021 at 12:10:27PM -0500, Suthikulpanit, Suravee wrote:
> Here is an dditional tags for this series:
> 
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> 
> Are there any concerns with this series?

No concerns, please add the tag and re-post when -rc1 is out. I will it
queue for -rc2 then.

Thanks,

	Joerg

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Joerg Roedel]

Hi Suravee,

On Tue, Aug 31, 2021 at 12:10:27PM -0500, Suthikulpanit, Suravee wrote:
> Here is an dditional tags for this series:
> 
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> 
> Are there any concerns with this series?

No concerns, please add the tag and re-post when -rc1 is out. I will it
queue for -rc2 then.

Thanks,

	Joerg

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

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Suthikulpanit, Suravee]

On 9/2/2021 12:38 AM, Joerg Roedel wrote:
> Hi Suravee,
> 
> On Tue, Aug 31, 2021 at 12:10:27PM -0500, Suthikulpanit, Suravee wrote:
>> Here is an dditional tags for this series:
>>
>> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
>>
>> Are there any concerns with this series?
> 
> No concerns, please add the tag and re-post when -rc1 is out. I will it
> queue for -rc2 then.
> 
> Thanks,
> 
> 	Joerg
> 

I'll do that.

Thanks,
Suravee

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Suthikulpanit, Suravee via iommu]

On 9/2/2021 12:38 AM, Joerg Roedel wrote:
> Hi Suravee,
> 
> On Tue, Aug 31, 2021 at 12:10:27PM -0500, Suthikulpanit, Suravee wrote:
>> Here is an dditional tags for this series:
>>
>> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
>>
>> Are there any concerns with this series?
> 
> No concerns, please add the tag and re-post when -rc1 is out. I will it
> queue for -rc2 then.
> 
> Thanks,
> 
> 	Joerg
> 

I'll do that.

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

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Joerg Roedel]

Hi Suravee,

On Thu, Sep 02, 2021 at 09:13:00AM -0700, Suthikulpanit, Suravee wrote:
> I'll do that.

New plan: I queued them now and added the Fixes tag. Will send a
pull-request tomorrow to get them upstream together with a couple of
other fixes.

Regards,

	Joerg

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Joerg Roedel]

Hi Suravee,

On Thu, Sep 02, 2021 at 09:13:00AM -0700, Suthikulpanit, Suravee wrote:
> I'll do that.

New plan: I queued them now and added the Fixes tag. Will send a
pull-request tomorrow to get them upstream together with a couple of
other fixes.

Regards,

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

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Joerg Roedel]

Okay, after this triggered a defconfig compile warning, I squashed patch
1 and 2 into one and also #ifdef'ed check_feature_on_all_iommus(). The
result is here:

From c3811a50addd23b9bb5a36278609ee1638debcf6 Mon Sep 17 00:00:00 2001
From: Wei Huang <wei.huang2@amd.com>
Date: Fri, 20 Aug 2021 15:29:55 -0500
Subject: [PATCH] iommu/amd: Relocate GAMSup check to early_enable_iommus

Currently, iommu_init_ga() checks and disables IOMMU VAPIC support
(i.e. AMD AVIC support in IOMMU) when GAMSup feature bit is not set.
However it forgets to clear IRQ_POSTING_CAP from the previously set
amd_iommu_irq_ops.capability.

This triggers an invalid page fault bug during guest VM warm reboot
if AVIC is enabled since the irq_remapping_cap(IRQ_POSTING_CAP) is
incorrectly set, and crash the system with the following kernel trace.

    BUG: unable to handle page fault for address: 0000000000400dd8
    RIP: 0010:amd_iommu_deactivate_guest_mode+0x19/0xbc
    Call Trace:
     svm_set_pi_irte_mode+0x8a/0xc0 [kvm_amd]
     ? kvm_make_all_cpus_request_except+0x50/0x70 [kvm]
     kvm_request_apicv_update+0x10c/0x150 [kvm]
     svm_toggle_avic_for_irq_window+0x52/0x90 [kvm_amd]
     svm_enable_irq_window+0x26/0xa0 [kvm_amd]
     vcpu_enter_guest+0xbbe/0x1560 [kvm]
     ? avic_vcpu_load+0xd5/0x120 [kvm_amd]
     ? kvm_arch_vcpu_load+0x76/0x240 [kvm]
     ? svm_get_segment_base+0xa/0x10 [kvm_amd]
     kvm_arch_vcpu_ioctl_run+0x103/0x590 [kvm]
     kvm_vcpu_ioctl+0x22a/0x5d0 [kvm]
     __x64_sys_ioctl+0x84/0xc0
     do_syscall_64+0x33/0x40
     entry_SYSCALL_64_after_hwframe+0x44/0xae

Fixes by moving the initializing of AMD IOMMU interrupt remapping mode
(amd_iommu_guest_ir) earlier before setting up the
amd_iommu_irq_ops.capability with appropriate IRQ_POSTING_CAP flag.

[joro:	Squashed the two patches and limited
	check_features_on_all_iommus() to CONFIG_IRQ_REMAP
	to fix a compile warning.]

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Link: https://lore.kernel.org/r/20210820202957.187572-2-suravee.suthikulpanit@amd.com
Link: https://lore.kernel.org/r/20210820202957.187572-3-suravee.suthikulpanit@amd.com
Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/init.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index bdcf167b4afe..4e753d1860b3 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -297,6 +297,22 @@ int amd_iommu_get_num_iommus(void)
 	return amd_iommus_present;
 }
 
+#ifdef CONFIG_IRQ_REMAP
+static bool check_feature_on_all_iommus(u64 mask)
+{
+	bool ret = false;
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		ret = iommu_feature(iommu, mask);
+		if (!ret)
+			return false;
+	}
+
+	return true;
+}
+#endif
+
 /*
  * For IVHD type 0x11/0x40, EFR is also available via IVHD.
  * Default to IVHD EFR since it is available sooner
@@ -853,13 +869,6 @@ static int iommu_init_ga(struct amd_iommu *iommu)
 	int ret = 0;
 
 #ifdef CONFIG_IRQ_REMAP
-	/* Note: We have already checked GASup from IVRS table.
-	 *       Now, we need to make sure that GAMSup is set.
-	 */
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
-	    !iommu_feature(iommu, FEATURE_GAM_VAPIC))
-		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
-
 	ret = iommu_init_ga_log(iommu);
 #endif /* CONFIG_IRQ_REMAP */
 
@@ -2479,6 +2488,14 @@ static void early_enable_iommus(void)
 	}
 
 #ifdef CONFIG_IRQ_REMAP
+	/*
+	 * Note: We have already checked GASup from IVRS table.
+	 *       Now, we need to make sure that GAMSup is set.
+	 */
+	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
+	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
+		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
+
 	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
 		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
 #endif
-- 
2.33.0

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Joerg Roedel]

Okay, after this triggered a defconfig compile warning, I squashed patch
1 and 2 into one and also #ifdef'ed check_feature_on_all_iommus(). The
result is here:

From c3811a50addd23b9bb5a36278609ee1638debcf6 Mon Sep 17 00:00:00 2001
From: Wei Huang <wei.huang2@amd.com>
Date: Fri, 20 Aug 2021 15:29:55 -0500
Subject: [PATCH] iommu/amd: Relocate GAMSup check to early_enable_iommus

Currently, iommu_init_ga() checks and disables IOMMU VAPIC support
(i.e. AMD AVIC support in IOMMU) when GAMSup feature bit is not set.
However it forgets to clear IRQ_POSTING_CAP from the previously set
amd_iommu_irq_ops.capability.

This triggers an invalid page fault bug during guest VM warm reboot
if AVIC is enabled since the irq_remapping_cap(IRQ_POSTING_CAP) is
incorrectly set, and crash the system with the following kernel trace.

    BUG: unable to handle page fault for address: 0000000000400dd8
    RIP: 0010:amd_iommu_deactivate_guest_mode+0x19/0xbc
    Call Trace:
     svm_set_pi_irte_mode+0x8a/0xc0 [kvm_amd]
     ? kvm_make_all_cpus_request_except+0x50/0x70 [kvm]
     kvm_request_apicv_update+0x10c/0x150 [kvm]
     svm_toggle_avic_for_irq_window+0x52/0x90 [kvm_amd]
     svm_enable_irq_window+0x26/0xa0 [kvm_amd]
     vcpu_enter_guest+0xbbe/0x1560 [kvm]
     ? avic_vcpu_load+0xd5/0x120 [kvm_amd]
     ? kvm_arch_vcpu_load+0x76/0x240 [kvm]
     ? svm_get_segment_base+0xa/0x10 [kvm_amd]
     kvm_arch_vcpu_ioctl_run+0x103/0x590 [kvm]
     kvm_vcpu_ioctl+0x22a/0x5d0 [kvm]
     __x64_sys_ioctl+0x84/0xc0
     do_syscall_64+0x33/0x40
     entry_SYSCALL_64_after_hwframe+0x44/0xae

Fixes by moving the initializing of AMD IOMMU interrupt remapping mode
(amd_iommu_guest_ir) earlier before setting up the
amd_iommu_irq_ops.capability with appropriate IRQ_POSTING_CAP flag.

[joro:	Squashed the two patches and limited
	check_features_on_all_iommus() to CONFIG_IRQ_REMAP
	to fix a compile warning.]

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Link: https://lore.kernel.org/r/20210820202957.187572-2-suravee.suthikulpanit@amd.com
Link: https://lore.kernel.org/r/20210820202957.187572-3-suravee.suthikulpanit@amd.com
Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd/init.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index bdcf167b4afe..4e753d1860b3 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -297,6 +297,22 @@ int amd_iommu_get_num_iommus(void)
 	return amd_iommus_present;
 }
 
+#ifdef CONFIG_IRQ_REMAP
+static bool check_feature_on_all_iommus(u64 mask)
+{
+	bool ret = false;
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		ret = iommu_feature(iommu, mask);
+		if (!ret)
+			return false;
+	}
+
+	return true;
+}
+#endif
+
 /*
  * For IVHD type 0x11/0x40, EFR is also available via IVHD.
  * Default to IVHD EFR since it is available sooner
@@ -853,13 +869,6 @@ static int iommu_init_ga(struct amd_iommu *iommu)
 	int ret = 0;
 
 #ifdef CONFIG_IRQ_REMAP
-	/* Note: We have already checked GASup from IVRS table.
-	 *       Now, we need to make sure that GAMSup is set.
-	 */
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
-	    !iommu_feature(iommu, FEATURE_GAM_VAPIC))
-		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
-
 	ret = iommu_init_ga_log(iommu);
 #endif /* CONFIG_IRQ_REMAP */
 
@@ -2479,6 +2488,14 @@ static void early_enable_iommus(void)
 	}
 
 #ifdef CONFIG_IRQ_REMAP
+	/*
+	 * Note: We have already checked GASup from IVRS table.
+	 *       Now, we need to make sure that GAMSup is set.
+	 */
+	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
+	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
+		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
+
 	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
 		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
 #endif
-- 
2.33.0

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

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Wei Huang]

Thanks. We did verify the correctness of this patch: we didn't see host
crash with guest reboot when this patch is applied.

Tested-by: Terry Bowman <terry.bowman@amd.com>

Thanks,
-Wei

On 9/9/21 6:17 AM, Joerg Roedel wrote:
> Okay, after this triggered a defconfig compile warning, I squashed patch
> 1 and 2 into one and also #ifdef'ed check_feature_on_all_iommus(). The
> result is here:
> 
> From c3811a50addd23b9bb5a36278609ee1638debcf6 Mon Sep 17 00:00:00 2001
> From: Wei Huang <wei.huang2@amd.com>
> Date: Fri, 20 Aug 2021 15:29:55 -0500
> Subject: [PATCH] iommu/amd: Relocate GAMSup check to early_enable_iommus
> 
> Currently, iommu_init_ga() checks and disables IOMMU VAPIC support
> (i.e. AMD AVIC support in IOMMU) when GAMSup feature bit is not set.
> However it forgets to clear IRQ_POSTING_CAP from the previously set
> amd_iommu_irq_ops.capability.
> 
> This triggers an invalid page fault bug during guest VM warm reboot
> if AVIC is enabled since the irq_remapping_cap(IRQ_POSTING_CAP) is
> incorrectly set, and crash the system with the following kernel trace.
> 
>     BUG: unable to handle page fault for address: 0000000000400dd8
>     RIP: 0010:amd_iommu_deactivate_guest_mode+0x19/0xbc
>     Call Trace:
>      svm_set_pi_irte_mode+0x8a/0xc0 [kvm_amd]
>      ? kvm_make_all_cpus_request_except+0x50/0x70 [kvm]
>      kvm_request_apicv_update+0x10c/0x150 [kvm]
>      svm_toggle_avic_for_irq_window+0x52/0x90 [kvm_amd]
>      svm_enable_irq_window+0x26/0xa0 [kvm_amd]
>      vcpu_enter_guest+0xbbe/0x1560 [kvm]
>      ? avic_vcpu_load+0xd5/0x120 [kvm_amd]
>      ? kvm_arch_vcpu_load+0x76/0x240 [kvm]
>      ? svm_get_segment_base+0xa/0x10 [kvm_amd]
>      kvm_arch_vcpu_ioctl_run+0x103/0x590 [kvm]
>      kvm_vcpu_ioctl+0x22a/0x5d0 [kvm]
>      __x64_sys_ioctl+0x84/0xc0
>      do_syscall_64+0x33/0x40
>      entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Fixes by moving the initializing of AMD IOMMU interrupt remapping mode
> (amd_iommu_guest_ir) earlier before setting up the
> amd_iommu_irq_ops.capability with appropriate IRQ_POSTING_CAP flag.
> 
> [joro:	Squashed the two patches and limited
> 	check_features_on_all_iommus() to CONFIG_IRQ_REMAP
> 	to fix a compile warning.]
> 
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Link: https://lore.kernel.org/r/20210820202957.187572-2-suravee.suthikulpanit@amd.com
> Link: https://lore.kernel.org/r/20210820202957.187572-3-suravee.suthikulpanit@amd.com
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/init.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index bdcf167b4afe..4e753d1860b3 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -297,6 +297,22 @@ int amd_iommu_get_num_iommus(void)
>  	return amd_iommus_present;
>  }
>  
> +#ifdef CONFIG_IRQ_REMAP
> +static bool check_feature_on_all_iommus(u64 mask)
> +{
> +	bool ret = false;
> +	struct amd_iommu *iommu;
> +
> +	for_each_iommu(iommu) {
> +		ret = iommu_feature(iommu, mask);
> +		if (!ret)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +#endif
> +
>  /*
>   * For IVHD type 0x11/0x40, EFR is also available via IVHD.
>   * Default to IVHD EFR since it is available sooner
> @@ -853,13 +869,6 @@ static int iommu_init_ga(struct amd_iommu *iommu)
>  	int ret = 0;
>  
>  #ifdef CONFIG_IRQ_REMAP
> -	/* Note: We have already checked GASup from IVRS table.
> -	 *       Now, we need to make sure that GAMSup is set.
> -	 */
> -	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> -	    !iommu_feature(iommu, FEATURE_GAM_VAPIC))
> -		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> -
>  	ret = iommu_init_ga_log(iommu);
>  #endif /* CONFIG_IRQ_REMAP */
>  
> @@ -2479,6 +2488,14 @@ static void early_enable_iommus(void)
>  	}
>  
>  #ifdef CONFIG_IRQ_REMAP
> +	/*
> +	 * Note: We have already checked GASup from IVRS table.
> +	 *       Now, we need to make sure that GAMSup is set.
> +	 */
> +	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> +	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
> +		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> +
>  	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
>  		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
>  #endif
> 

Re: [PATCH 0/3] iommu/amd: Fix unable to handle page fault due to AVIC

[Wei Huang via iommu]

Thanks. We did verify the correctness of this patch: we didn't see host
crash with guest reboot when this patch is applied.

Tested-by: Terry Bowman <terry.bowman@amd.com>

Thanks,
-Wei

On 9/9/21 6:17 AM, Joerg Roedel wrote:
> Okay, after this triggered a defconfig compile warning, I squashed patch
> 1 and 2 into one and also #ifdef'ed check_feature_on_all_iommus(). The
> result is here:
> 
> From c3811a50addd23b9bb5a36278609ee1638debcf6 Mon Sep 17 00:00:00 2001
> From: Wei Huang <wei.huang2@amd.com>
> Date: Fri, 20 Aug 2021 15:29:55 -0500
> Subject: [PATCH] iommu/amd: Relocate GAMSup check to early_enable_iommus
> 
> Currently, iommu_init_ga() checks and disables IOMMU VAPIC support
> (i.e. AMD AVIC support in IOMMU) when GAMSup feature bit is not set.
> However it forgets to clear IRQ_POSTING_CAP from the previously set
> amd_iommu_irq_ops.capability.
> 
> This triggers an invalid page fault bug during guest VM warm reboot
> if AVIC is enabled since the irq_remapping_cap(IRQ_POSTING_CAP) is
> incorrectly set, and crash the system with the following kernel trace.
> 
>     BUG: unable to handle page fault for address: 0000000000400dd8
>     RIP: 0010:amd_iommu_deactivate_guest_mode+0x19/0xbc
>     Call Trace:
>      svm_set_pi_irte_mode+0x8a/0xc0 [kvm_amd]
>      ? kvm_make_all_cpus_request_except+0x50/0x70 [kvm]
>      kvm_request_apicv_update+0x10c/0x150 [kvm]
>      svm_toggle_avic_for_irq_window+0x52/0x90 [kvm_amd]
>      svm_enable_irq_window+0x26/0xa0 [kvm_amd]
>      vcpu_enter_guest+0xbbe/0x1560 [kvm]
>      ? avic_vcpu_load+0xd5/0x120 [kvm_amd]
>      ? kvm_arch_vcpu_load+0x76/0x240 [kvm]
>      ? svm_get_segment_base+0xa/0x10 [kvm_amd]
>      kvm_arch_vcpu_ioctl_run+0x103/0x590 [kvm]
>      kvm_vcpu_ioctl+0x22a/0x5d0 [kvm]
>      __x64_sys_ioctl+0x84/0xc0
>      do_syscall_64+0x33/0x40
>      entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Fixes by moving the initializing of AMD IOMMU interrupt remapping mode
> (amd_iommu_guest_ir) earlier before setting up the
> amd_iommu_irq_ops.capability with appropriate IRQ_POSTING_CAP flag.
> 
> [joro:	Squashed the two patches and limited
> 	check_features_on_all_iommus() to CONFIG_IRQ_REMAP
> 	to fix a compile warning.]
> 
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Link: https://lore.kernel.org/r/20210820202957.187572-2-suravee.suthikulpanit@amd.com
> Link: https://lore.kernel.org/r/20210820202957.187572-3-suravee.suthikulpanit@amd.com
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd/init.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index bdcf167b4afe..4e753d1860b3 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -297,6 +297,22 @@ int amd_iommu_get_num_iommus(void)
>  	return amd_iommus_present;
>  }
>  
> +#ifdef CONFIG_IRQ_REMAP
> +static bool check_feature_on_all_iommus(u64 mask)
> +{
> +	bool ret = false;
> +	struct amd_iommu *iommu;
> +
> +	for_each_iommu(iommu) {
> +		ret = iommu_feature(iommu, mask);
> +		if (!ret)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +#endif
> +
>  /*
>   * For IVHD type 0x11/0x40, EFR is also available via IVHD.
>   * Default to IVHD EFR since it is available sooner
> @@ -853,13 +869,6 @@ static int iommu_init_ga(struct amd_iommu *iommu)
>  	int ret = 0;
>  
>  #ifdef CONFIG_IRQ_REMAP
> -	/* Note: We have already checked GASup from IVRS table.
> -	 *       Now, we need to make sure that GAMSup is set.
> -	 */
> -	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> -	    !iommu_feature(iommu, FEATURE_GAM_VAPIC))
> -		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> -
>  	ret = iommu_init_ga_log(iommu);
>  #endif /* CONFIG_IRQ_REMAP */
>  
> @@ -2479,6 +2488,14 @@ static void early_enable_iommus(void)
>  	}
>  
>  #ifdef CONFIG_IRQ_REMAP
> +	/*
> +	 * Note: We have already checked GASup from IVRS table.
> +	 *       Now, we need to make sure that GAMSup is set.
> +	 */
> +	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> +	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
> +		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> +
>  	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
>  		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
>  #endif
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu