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@xxxxxxx>

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@xxxxxxx>
> 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@xxxxxxx>
> Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Link: https://lore.kernel.org/r/20210820202957.187572-2-suravee.suthikulpanit@xxxxxxx
> Link: https://lore.kernel.org/r/20210820202957.187572-3-suravee.suthikulpanit@xxxxxxx
> Fixes: 8bda0cfbdc1a ("iommu/amd: Detect and initialize guest vAPIC log")
> Signed-off-by: Joerg Roedel <jroedel@xxxxxxx>
> ---
>  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
>