Re: [PATCH 1/2] iommu/amd: Simplify and Consolidate Virtual APIC (AVIC) Enablement

From: Jerry Snitselaar
Date: Tue Jul 26 2022 - 11:28:14 EST

On Tue, Jul 26, 2022 at 08:43:47AM -0500, Suravee Suthikulpanit wrote:
> Currently, enabling AVIC requires individually detect and enable GAM and
> GALOG features on each IOMMU, which is difficult to keep track on
> multi-IOMMU system, where the features needs to be enabled system-wide.
>
> In addition, these features do not need to be enabled in early stage.
> It can be delayed until after amd_iommu_init_pci().
>
> Therefore, consolidate logic for detecting and enabling IOMMU GAM and
> GALOG features into a helper function, enable_iommus_vapic(), which uses
> the check_feature_on_all_iommus() helper function to ensure system-wide
> support of the features before enabling them, and postpone until after
> amd_iommu_init_pci().
>
> The new function also check and clean up feature enablement residue from
> previous boot (e.g. in case of booting into kdump kernel), which triggers
> a WARN_ON (shown below) introduced by the commit a8d4a37d1bb9 ("iommu/amd:
> Restore GA log/tail pointer on host resume") in iommu_ga_log_enable().
>
> [ 7.731955] ------------[ cut here ]------------
> [ 7.736575] WARNING: CPU: 0 PID: 1 at drivers/iommu/amd/init.c:829 iommu_ga_log_enable.isra.0+0x16f/0x190
> [ 7.746135] Modules linked in:
> [ 7.749193] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W -------- --- 5.19.0-0.rc7.53.eln120.x86_64 #1
> [ 7.759706] Hardware name: Dell Inc. PowerEdge R7525/04D5GJ, BIOS 2.1.6 03/09/2021
> [ 7.767274] RIP: 0010:iommu_ga_log_enable.isra.0+0x16f/0x190
> [ 7.772931] Code: 20 20 00 00 8b 00 f6 c4 01 74 da 48 8b 44 24 08 65 48 2b 04 25 28 00 00 00 75 13 48 83 c4 10 5b 5d e9 f5 00 72 00 0f 0b eb e1 <0f> 0b eb dd e8 f8 66 42 00 48 8b 15 f1 85 53 01 e9 29 ff ff ff 48
> [ 7.791679] RSP: 0018:ffffc90000107d20 EFLAGS: 00010206
> [ 7.796905] RAX: ffffc90000780000 RBX: 0000000000000100 RCX: ffffc90000780000
> [ 7.804038] RDX: 0000000000000001 RSI: ffffc90000780000 RDI: ffff8880451f9800
> [ 7.811170] RBP: ffff8880451f9800 R08: ffffffffffffffff R09: 0000000000000000
> [ 7.818303] R10: 0000000000000000 R11: 0000000000000000 R12: 0008000000000000
> [ 7.825435] R13: ffff8880462ea900 R14: 0000000000000021 R15: 0000000000000000
> [ 7.832572] FS: 0000000000000000(0000) GS:ffff888054a00000(0000) knlGS:0000000000000000
> [ 7.840657] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 7.846400] CR2: ffff888054dff000 CR3: 0000000053210000 CR4: 0000000000350eb0
> [ 7.853533] Call Trace:
> [ 7.855979] <TASK>
> [ 7.858085] amd_iommu_enable_interrupts+0x180/0x270
> [ 7.863051] ? iommu_setup+0x271/0x271
> [ 7.866803] state_next+0x197/0x2c0
> [ 7.870295] ? iommu_setup+0x271/0x271
> [ 7.874049] iommu_go_to_state+0x24/0x2c
> [ 7.877976] amd_iommu_init+0xf/0x29
> [ 7.881554] pci_iommu_init+0xe/0x36
> [ 7.885133] do_one_initcall+0x44/0x200
> [ 7.888975] do_initcalls+0xc8/0xe1
> [ 7.892466] kernel_init_freeable+0x14c/0x199
> [ 7.896826] ? rest_init+0xd0/0xd0
> [ 7.900231] kernel_init+0x16/0x130
> [ 7.903723] ret_from_fork+0x22/0x30
> [ 7.907306] </TASK>
> [ 7.909497] ---[ end trace 0000000000000000 ]---
>
> Fixes: commit a8d4a37d1bb9 ("iommu/amd: Restore GA log/tail pointer on host resume")
> Reported-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> Cc: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx> (maintainer:IOMMU DRIVERS)
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>

Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>

> ---
> drivers/iommu/amd/init.c | 85 ++++++++++++++++++++++++++--------------
> 1 file changed, 55 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index d86496114ca5..4cd94d716122 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -908,11 +908,6 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
> if (!iommu->ga_log)
> return -EINVAL;
>
> - /* Check if already running */
> - status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> - if (WARN_ON(status & (MMIO_STATUS_GALOG_RUN_MASK)))
> - return 0;
> -
> entry = iommu_virt_to_phys(iommu->ga_log) | GA_LOG_SIZE_512;
> memcpy_toio(iommu->mmio_base + MMIO_GA_LOG_BASE_OFFSET,
> &entry, sizeof(entry));
> @@ -2068,10 +2063,6 @@ 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_log(iommu);
> - if (ret)
> - return ret;
> -
> if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
> pr_info("Using strict mode due to virtualization\n");
> iommu_set_dma_strict();
> @@ -2155,8 +2146,6 @@ static void print_iommu_info(void)
> }
> if (irq_remapping_enabled) {
> pr_info("Interrupt remapping enabled\n");
> - if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
> - pr_info("Virtual APIC enabled\n");
> if (amd_iommu_xt_mode == IRQ_REMAP_X2APIC_MODE)
> pr_info("X2APIC enabled\n");
> }
> @@ -2446,9 +2435,6 @@ static int iommu_init_irq(struct amd_iommu *iommu)
>
> if (iommu->ppr_log != NULL)
> iommu_feature_enable(iommu, CONTROL_PPRINT_EN);
> -
> - iommu_ga_log_enable(iommu);
> -
> return 0;
> }
>
> @@ -2678,8 +2664,6 @@ static void iommu_enable_ga(struct amd_iommu *iommu)
> #ifdef CONFIG_IRQ_REMAP
> switch (amd_iommu_guest_ir) {
> case AMD_IOMMU_GUEST_IR_VAPIC:
> - iommu_feature_enable(iommu, CONTROL_GAM_EN);
> - fallthrough;
> case AMD_IOMMU_GUEST_IR_LEGACY_GA:
> iommu_feature_enable(iommu, CONTROL_GA_EN);
> iommu->irte_ops = &irte_128_ops;
> @@ -2759,19 +2743,6 @@ static void early_enable_iommus(void)
> iommu_flush_all_caches(iommu);
> }
> }
> -
> -#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
> }
>
> static void enable_iommus_v2(void)
> @@ -2784,10 +2755,63 @@ static void enable_iommus_v2(void)
> }
> }
>
> +static void enable_iommus_vapic(void)
> +{
> +#ifdef CONFIG_IRQ_REMAP
> + u32 status, i;
> + struct amd_iommu *iommu;
> +
> + for_each_iommu(iommu) {
> + /*
> + * Disable GALog if already running. It could have been enabled
> + * in the previous boot before kdump.
> + */
> + status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> + if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
> + continue;
> +
> + iommu_feature_disable(iommu, CONTROL_GALOG_EN);
> + iommu_feature_disable(iommu, CONTROL_GAINT_EN);
> +
> + /*
> + * Need to set and poll check the GALOGRun bit to zero before
> + * we can set/ modify GA Log registers safely.
> + */
> + for (i = 0; i < LOOP_TIMEOUT; ++i) {
> + status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> + if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
> + break;
> + udelay(10);
> + }
> +
> + if (WARN_ON(i >= LOOP_TIMEOUT))
> + return;
> + }
> +
> + 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;
> + return;
> + }
> +
> + /* Enabling GAM support */
> + for_each_iommu(iommu) {
> + if (iommu_init_ga_log(iommu) ||
> + iommu_ga_log_enable(iommu))
> + return;
> +
> + iommu_feature_enable(iommu, CONTROL_GAM_EN);
> + }
> +
> + amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
> + pr_info("Virtual APIC enabled\n");
> +#endif
> +}
> +
> static void enable_iommus(void)
> {
> early_enable_iommus();
> -
> + enable_iommus_vapic();
> enable_iommus_v2();
> }
>
> @@ -3126,6 +3150,7 @@ static int __init state_next(void)
> register_syscore_ops(&amd_iommu_syscore_ops);
> ret = amd_iommu_init_pci();
> init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
> + enable_iommus_vapic();
> enable_iommus_v2();
> break;
> case IOMMU_PCI_INIT:
> --
> 2.34.1
>