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 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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Wei Huang <[email protected]>
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 <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
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
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(-)
>
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
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
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
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 <[email protected]>
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 <[email protected]>
Co-developed-by: Suravee Suthikulpanit <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/[email protected]
Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
Signed-off-by: Joerg Roedel <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> 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 <[email protected]>
> Co-developed-by: Suravee Suthikulpanit <[email protected]>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Link: https://lore.kernel.org/r/[email protected]
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> 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
>