From: Shuah Khan <skhan@linuxfoundation.org> To: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org Cc: joro@8bytes.org, will@kernel.org, jsnitsel@redhat.com, pmenzel@molgen.mpg.de, Jon.Grimm@amd.com, Tj <ml.linux@elloe.vision>, Alexander Monakov <amonakov@ispras.ru>, David Coe <david.coe@live.co.uk>, Shuah Khan <skhan@linuxfoundation.org> Subject: Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test Date: Fri, 9 Apr 2021 11:10:29 -0600 [thread overview] Message-ID: <4f64e9d4-dfc5-3dec-32c9-06bfbfeafd6b@linuxfoundation.org> (raw) In-Reply-To: <6d53ded8-9ab8-7205-f17e-d0c315b770bd@linuxfoundation.org> On 4/9/21 10:37 AM, Shuah Khan wrote: > On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote: >> In early AMD desktop/mobile platforms (during 2013), when the IOMMU >> Performance Counter (PMC) support was first introduced in >> commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter >> resource management"), there was a HW bug where the counters could not >> be accessed. The result was reading of the counter always return zero. >> >> At the time, the suggested workaround was to add a test logic prior >> to initializing the PMC feature to check if the counters can be >> programmed >> and read back the same value. This has been working fine until the more >> recent desktop/mobile platforms start enabling power gating for the PMC, >> which prevents access to the counters. This results in the PMC support >> being disabled unnecesarily. > > unnecessarily > >> >> Unfortunatly, there is no documentation of since which generation > > Unfortunately, > > Rephrase suggestion: > Unfortunately, it is unclear when the PMC HW bug fixed. > >> of hardware the original PMC HW bug was fixed. Although, it was fixed >> soon after the first introduction of the PMC. Base on this, we assume > > Based > >> that the buggy platforms are less likely to be in used, and it should > > in use > >> be relatively safe to remove this legacy logic. > >> >> Link: >> https://lore.kernel.org/linux-iommu/alpine.LNX.3.20.13.2006030935570.3181@monopod.intra.ispras.ru/ >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753 >> Cc: Tj (Elloe Linux) <ml.linux@elloe.vision> >> Cc: Shuah Khan <skhan@linuxfoundation.org> >> Cc: Alexander Monakov <amonakov@ispras.ru> >> Cc: David Coe <david.coe@live.co.uk> >> Cc: Paul Menzel <pmenzel@molgen.mpg.de> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> --- >> drivers/iommu/amd/init.c | 24 +----------------------- >> 1 file changed, 1 insertion(+), 23 deletions(-) >> >> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c >> index 648cdfd03074..247cdda5d683 100644 >> --- a/drivers/iommu/amd/init.c >> +++ b/drivers/iommu/amd/init.c >> @@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct >> acpi_table_header *table) >> return 0; >> } >> -static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 >> cntr, >> - u8 fxn, u64 *value, bool is_write); >> - >> static void init_iommu_perf_ctr(struct amd_iommu *iommu) >> { >> + u64 val; >> struct pci_dev *pdev = iommu->dev; >> - u64 val = 0xabcd, val2 = 0, save_reg = 0; Why not leave this u64 val here? Having the pdev assignment as the first line makes it easier to read/follow. >> if (!iommu_feature(iommu, FEATURE_PC)) >> return; >> amd_iommu_pc_present = true; >> - /* save the value to restore, if writable */ >> - if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false)) >> - goto pc_false; >> - >> - /* Check if the performance counters can be written to */ >> - if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) || >> - (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) || >> - (val != val2)) Aha - this is going away anyway. Please ignore my comment on 1/2 about parenthesis around (val != val2) being unnecessary. >> - goto pc_false; >> - >> - /* restore */ >> - if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true)) >> - goto pc_false; >> - >> pci_info(pdev, "IOMMU performance counters supported\n"); >> val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET); >> @@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct >> amd_iommu *iommu) >> iommu->max_counters = (u8) ((val >> 7) & 0xf); >> return; >> - >> -pc_false: >> - pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n"); >> - amd_iommu_pc_present = false; >> - return; >> } >> static ssize_t amd_iommu_show_cap(struct device *dev, >> > thanks, -- Shuah
WARNING: multiple messages have this Message-ID (diff)
From: Shuah Khan <skhan@linuxfoundation.org> To: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org Cc: pmenzel@molgen.mpg.de, Alexander Monakov <amonakov@ispras.ru>, David Coe <david.coe@live.co.uk>, Jon.Grimm@amd.com, Shuah Khan <skhan@linuxfoundation.org>, Tj <ml.linux@elloe.vision>, will@kernel.org Subject: Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test Date: Fri, 9 Apr 2021 11:10:29 -0600 [thread overview] Message-ID: <4f64e9d4-dfc5-3dec-32c9-06bfbfeafd6b@linuxfoundation.org> (raw) In-Reply-To: <6d53ded8-9ab8-7205-f17e-d0c315b770bd@linuxfoundation.org> On 4/9/21 10:37 AM, Shuah Khan wrote: > On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote: >> In early AMD desktop/mobile platforms (during 2013), when the IOMMU >> Performance Counter (PMC) support was first introduced in >> commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter >> resource management"), there was a HW bug where the counters could not >> be accessed. The result was reading of the counter always return zero. >> >> At the time, the suggested workaround was to add a test logic prior >> to initializing the PMC feature to check if the counters can be >> programmed >> and read back the same value. This has been working fine until the more >> recent desktop/mobile platforms start enabling power gating for the PMC, >> which prevents access to the counters. This results in the PMC support >> being disabled unnecesarily. > > unnecessarily > >> >> Unfortunatly, there is no documentation of since which generation > > Unfortunately, > > Rephrase suggestion: > Unfortunately, it is unclear when the PMC HW bug fixed. > >> of hardware the original PMC HW bug was fixed. Although, it was fixed >> soon after the first introduction of the PMC. Base on this, we assume > > Based > >> that the buggy platforms are less likely to be in used, and it should > > in use > >> be relatively safe to remove this legacy logic. > >> >> Link: >> https://lore.kernel.org/linux-iommu/alpine.LNX.3.20.13.2006030935570.3181@monopod.intra.ispras.ru/ >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753 >> Cc: Tj (Elloe Linux) <ml.linux@elloe.vision> >> Cc: Shuah Khan <skhan@linuxfoundation.org> >> Cc: Alexander Monakov <amonakov@ispras.ru> >> Cc: David Coe <david.coe@live.co.uk> >> Cc: Paul Menzel <pmenzel@molgen.mpg.de> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> --- >> drivers/iommu/amd/init.c | 24 +----------------------- >> 1 file changed, 1 insertion(+), 23 deletions(-) >> >> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c >> index 648cdfd03074..247cdda5d683 100644 >> --- a/drivers/iommu/amd/init.c >> +++ b/drivers/iommu/amd/init.c >> @@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct >> acpi_table_header *table) >> return 0; >> } >> -static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 >> cntr, >> - u8 fxn, u64 *value, bool is_write); >> - >> static void init_iommu_perf_ctr(struct amd_iommu *iommu) >> { >> + u64 val; >> struct pci_dev *pdev = iommu->dev; >> - u64 val = 0xabcd, val2 = 0, save_reg = 0; Why not leave this u64 val here? Having the pdev assignment as the first line makes it easier to read/follow. >> if (!iommu_feature(iommu, FEATURE_PC)) >> return; >> amd_iommu_pc_present = true; >> - /* save the value to restore, if writable */ >> - if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false)) >> - goto pc_false; >> - >> - /* Check if the performance counters can be written to */ >> - if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) || >> - (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) || >> - (val != val2)) Aha - this is going away anyway. Please ignore my comment on 1/2 about parenthesis around (val != val2) being unnecessary. >> - goto pc_false; >> - >> - /* restore */ >> - if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true)) >> - goto pc_false; >> - >> pci_info(pdev, "IOMMU performance counters supported\n"); >> val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET); >> @@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct >> amd_iommu *iommu) >> iommu->max_counters = (u8) ((val >> 7) & 0xf); >> return; >> - >> -pc_false: >> - pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n"); >> - amd_iommu_pc_present = false; >> - return; >> } >> static ssize_t amd_iommu_show_cap(struct device *dev, >> > thanks, -- Shuah _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-04-09 17:10 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-09 8:58 [PATCH 0/2] iommu/amd: Revert and remove failing PMC test Suravee Suthikulpanit 2021-04-09 8:58 ` Suravee Suthikulpanit 2021-04-09 8:58 ` [PATCH 1/2] Revert "iommu/amd: Fix performance counter initialization" Suravee Suthikulpanit 2021-04-09 8:58 ` Suravee Suthikulpanit 2021-04-09 17:06 ` Shuah Khan 2021-04-09 17:06 ` Shuah Khan 2021-04-13 13:36 ` Suthikulpanit, Suravee 2021-04-13 13:36 ` Suthikulpanit, Suravee 2021-04-09 8:58 ` [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test Suravee Suthikulpanit 2021-04-09 8:58 ` Suravee Suthikulpanit 2021-04-09 16:37 ` Shuah Khan 2021-04-09 16:37 ` Shuah Khan 2021-04-09 17:10 ` Shuah Khan [this message] 2021-04-09 17:10 ` Shuah Khan 2021-04-09 20:00 ` Shuah Khan 2021-04-09 20:00 ` Shuah Khan 2021-04-09 20:19 ` Shuah Khan 2021-04-09 20:19 ` Shuah Khan 2021-04-09 20:11 ` David Coe 2021-04-09 20:11 ` David Coe 2021-04-10 8:17 ` David Coe 2021-04-10 8:17 ` David Coe 2021-04-10 10:03 ` David Coe 2021-04-10 10:03 ` David Coe 2021-04-13 13:51 ` Suthikulpanit, Suravee 2021-04-13 13:51 ` Suthikulpanit, Suravee 2021-04-14 15:33 ` David Coe 2021-04-14 15:33 ` David Coe 2021-04-15 9:28 ` Suthikulpanit, Suravee 2021-04-15 9:28 ` Suthikulpanit, Suravee 2021-04-15 14:39 ` David Coe 2021-04-15 14:39 ` David Coe 2021-04-15 16:20 ` David Coe 2021-04-15 16:20 ` David Coe 2021-04-18 19:16 ` David Coe 2021-04-18 19:16 ` David Coe 2021-04-14 22:18 ` David Coe 2021-04-14 22:18 ` David Coe 2021-04-20 8:38 ` Suthikulpanit, Suravee 2021-04-20 8:38 ` Suthikulpanit, Suravee 2021-04-20 10:33 ` Alexander Monakov 2021-04-20 10:33 ` Alexander Monakov 2021-04-13 9:38 ` David Coe 2021-04-13 9:38 ` David Coe 2021-04-15 13:41 ` [PATCH 0/2] iommu/amd: Revert and remove failing PMC test Joerg Roedel 2021-04-15 13:41 ` Joerg Roedel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4f64e9d4-dfc5-3dec-32c9-06bfbfeafd6b@linuxfoundation.org \ --to=skhan@linuxfoundation.org \ --cc=Jon.Grimm@amd.com \ --cc=amonakov@ispras.ru \ --cc=david.coe@live.co.uk \ --cc=iommu@lists.linux-foundation.org \ --cc=joro@8bytes.org \ --cc=jsnitsel@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=ml.linux@elloe.vision \ --cc=pmenzel@molgen.mpg.de \ --cc=suravee.suthikulpanit@amd.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.