[RESEND PATCH v2] iommu/amd: Fix extended features logging
print_iommu_info prints the EFR register and then the decoded list of
features on a separate line:
pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
PPR X2APIC NX GT IA GA PC GA_vAPIC
The second line is emitted via 'pr_cont', which causes it to have a
different ('warn') loglevel compared to the previous line ('info').
Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
from the pci_info format string, but this doesn't work, as pci_info
calls implicitly append a newline anyway.
Printing the decoded features on the same line would make it quite long.
Instead, change pci_info() to pr_info() to omit PCI bus location info,
which is also shown in the preceding message. This results in:
pci :00:00.2: AMD-Vi: Found IOMMU cap 0x40
AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC
GA_vAPIC
AMD-Vi: Interrupt remapping enabled
Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
divergent log levels")
Link:
https://lore.kernel.org/lkml/alpine.lnx.2.20.13.2104112326460.11...@monopod.intra.ispras.ru
Signed-off-by: Alexander Monakov
Cc: Paul Menzel
Cc: Joerg Roedel
Cc: Suravee Suthikulpanit
Cc: iommu@lists.linux-foundation.org
---
drivers/iommu/amd/init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 429a4baa3aee..8f0eb865119a 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1954,8 +1954,8 @@ static void print_iommu_info(void)
pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pci_info(pdev, "Extended features (%#llx):",
-iommu->features);
+ pr_info("Extended features (%#llx):", iommu->features);
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
if (iommu_feature(iommu, (1ULL << i)))
pr_cont(" %s", feat_str[i]);
base-commit: 9f4ad9e425a1d3b6a34617b8ea226d56a119a717
--
2.30.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On Tue, 20 Apr 2021, Suthikulpanit, Suravee wrote:
> David / Joerg,
>
> On 4/10/2021 5:03 PM, David Coe wrote:
> >
> > The immediately obvious difference is the with the enormous count seen on
> > mem_dte_mis on the older Ryzen 2400G. Will do some RTFM but anyone
> > with comments and insight?
> >
> > 841,689,151,202,939 amd_iommu_0/mem_dte_mis/ (33.44%)
> >
> > Otherwise, all seems to running smoothly (especially for a distribution
> > still in β). Bravo and many thanks all!
>
> The initial hypothesis is that the issue happens only when users specify more
> number of events than
> the available counters, which Perf will time-multiplex the events onto the
> counters.
>
> Looking at the Perf and AMD IOMMU PMU multiplexing logic, it requires:
> 1. Stop the counter (i.e. set CSOURCE to zero to stop counting)
> 2. Save the counter value of the current event
> 3. Reload the counter value of the new event (previously saved)
> 4. Start the counter (i.e. set CSOURCE to count new events)
>
> The problem here is that when the driver writes zero to CSOURCE register in
> step 1, this would enable power-gating,
> which prevents access to the counter and result in writing/reading value in
> step 2 and 3.
>
> I have found a system that reproduced this case (w/ unusually large number of
> count), and debug the issue further.
> As a hack, I have tried skipping step 1, and it seems to eliminate this issue.
> However, this is logically incorrect,
> and might result in inaccurate data depending on the events.
>
> Here are the options:
> 1. Continue to look for workaround for this issue.
> 2. Find a way to disable event time-multiplexing (e.g. only limit the number
> of counters to 8)
>if power gating is enabled on the platform.
> 3. Back to the original logic where we had the pre-init check of the counter
> vlues, which is still the safest choice
>at the moment unless
If the "power-gated" counter only ignores writes, but yields correct values on
reads, you don't need to change its value.
0. When initializing the counter, save its value as 'raw_counter_value'.
When switching:
1. Set CSOURCE to zero (pauses the counter).
2. Read current counter value ('new_value').
3. Add '(new_value - raw_counter_value) & mask' to the current event count;
where 'mask' is '(1ULL << 48) - 1' to account for overflow correctly.
4. Assign 'raw_counter_value = new_value'.
5. Set CSOURCE to new configuration value.
Modifying the hardware counter value is only needed to get overflow interrupts,
but there's no code to handle them on Linux (since the interrupts are
asynchronous, and given the nature of the events, I don't see how it would be
useful).
Alexander___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/amd: Fix extended features logging
On Sun, 11 Apr 2021, Joe Perches wrote:
> > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> > solution
> >
> > drivers/iommu/amd/init.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > index 596d0c413473..62913f82a21f 100644
> > --- a/drivers/iommu/amd/init.c
> > +++ b/drivers/iommu/amd/init.c
> > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
> > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
> >
> >
> > if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> > - pci_info(pdev, "Extended features (%#llx):",
> > -iommu->features);
> > + pr_info("Extended features (%#llx):", iommu->features);
> > +
> > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> > if (iommu_feature(iommu, (1ULL << i)))
> > pr_cont(" %s", feat_str[i]);
>
> How about avoiding all of this by using a temporary buffer
> and a single pci_info.
I think it is mostly up to the maintainers, but from my perspective, it's not
good to conflate such a simple bugfix with the substantial rewrite you are
proposing (which also increases code complexity).
My two-line patch is a straightforward fix to a bug that people already agreed
needs to be fixed (just the previous attempt turned out to be insufficient). If
there's a desire to eliminate pr_cont calls (which I wouldn't support in this
instance), the rewrite can go in separately from the bugfix.
A major problem with landing a simple bugfix together with a rewrite in a big
patch is that if a rewrite causes a problem, the whole patch gets reverted and
we end up without a trivial bugfix.
And, once again: can we please not emit the feature list via pci_info, the line
is long enough already even without the pci bus location info.
Joerg, are you satisfied with my v2 patch, are you waiting for anything before
picking it up?
Alexander
>
> Miscellanea:
> o Move the feat_str and i declarations into the if block for locality
>
> ---
> drivers/iommu/amd/init.c | 29 ++---
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 321f5906e6ed..0d219044726e 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu
> *iommu)
>
> static void print_iommu_info(void)
> {
> - static const char * const feat_str[] = {
> - "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
> - "IA", "GA", "HE", "PC"
> - };
> struct amd_iommu *iommu;
>
> for_each_iommu(iommu) {
> struct pci_dev *pdev = iommu->dev;
> - int i;
>
> pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr);
>
> if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> - iommu->features);
> + static const char * const feat_str[] = {
> + "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
> + "IA", "GA", "HE", "PC"
> + };
> + int i;
> + char features[128] = "";
> + int len = 0;
> +
> for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> - if (iommu_feature(iommu, (1ULL << i)))
> - pr_cont(" %s", feat_str[i]);
> + if (!iommu_feature(iommu, BIT_ULL(i)))
> + continue;
> + len += scnprintf(features + len,
> + sizeof(features) - len,
> + " %s", feat_str[i]);
> }
>
> if (iommu->features & FEATURE_GAM_VAPIC)
> - pr_cont(" GA_vAPIC");
> + len += scnprintf(features + len,
> + sizeof(features) - len,
> + " %s", "GA_vPIC");
>
> - pr_cont("\n");
> + pci_info(pdev, "Extended features (%#llx):%s\n",
> + iommu->features, features);
> }
> }
> if (irq_remapping_enabled) {
>
>
> ___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/amd: Fix extended features logging
print_iommu_info prints the EFR register and then the decoded list of
features on a separate line:
pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
PPR X2APIC NX GT IA GA PC GA_vAPIC
The second line is emitted via 'pr_cont', which causes it to have a
different ('warn') loglevel compared to the previous line ('info').
Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
from the pci_info format string, but this doesn't work, as pci_info
calls implicitly append a newline anyway.
Printing the decoded features on the same line would make it quite long.
Instead, change pci_info() to pr_info() to omit PCI bus location info,
which is shown in the preceding message anyway. This results in:
pci :00:00.2: AMD-Vi: Found IOMMU cap 0x40
AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC
GA_vAPIC
AMD-Vi: Interrupt remapping enabled
Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
divergent log levels")
Link:
https://lore.kernel.org/lkml/alpine.lnx.2.20.13.2104112326460.11...@monopod.intra.ispras.ru
Signed-off-by: Alexander Monakov
Cc: Paul Menzel
Cc: Joerg Roedel
Cc: Suravee Suthikulpanit
Cc: iommu@lists.linux-foundation.org
---
v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
solution
drivers/iommu/amd/init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 596d0c413473..62913f82a21f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pci_info(pdev, "Extended features (%#llx):",
-iommu->features);
+ pr_info("Extended features (%#llx):", iommu->features);
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
if (iommu_feature(iommu, (1ULL << i)))
pr_cont(" %s", feat_str[i]);
--
2.30.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix extended features logging
On Sun, 11 Apr 2021, John Ogness wrote:
> > pr_info("") will also prepend 'AMD-Vi:' to the feature list, which is
> > nice.
>
> I'd rather fix dev_info callers to allow pr_cont and then fix any code
> that is using this workaround.
Note that existing two users of pr_info("") that I found are not using that
as a workaround: efi.c is using that when it announced a list of EFI tables:
efi: ACPI=0xadffe000 ACPI 2.0=0xadffe014 TPMFinalLog=0xadf2d000 ESRT=0xab70d618
SMBIOS=0xab70b000 SMBIOS 3.0=0xab709000 RNG=0xab707b98 TPMEventLog=0x9602c018
and uvesafb.c similarly uses it to print a list of conditionally-present
strings. In both cases it is really a standalone message, not a continuation
for something previously printed.
In contrast, my patch deals with a continuation line. I wouldn't really like
the decoded feature list to appear on the same line as the previous message,
because it would end up quite long:
[0.266332] pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
[0.266334] AMD-Vi: PPR X2APIC NX GT IA GA PC GA_vAPIC
I think a good compromise is to change the first line from pci_info to pr_info,
losing the pci bus address. I'll send a v2.
Alexander
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix extended features logging
On Sun, 11 Apr 2021, Joe Perches wrote: > (cc'ing the printk maintainers) > [snip] > > This shouldn't be necessary. > If this is true then a lot of output logging code broke. See also my response to Paul at https://lore.kernel.org/lkml/alpine.lnx.2.20.13.2104111410340.11...@monopod.intra.ispras.ru/ Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix extended features logging
On Sun, 11 Apr 2021, Paul Menzel wrote:
> > The second line is emitted via 'pr_cont', which causes it to have a
> > different ('warn') loglevel compared to the previous line ('info').
> >
> > Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> > from the pci_info format string, but this doesn't work, as pci_info
> > calls implicitly append a newline anyway.
>
> Hmm, did I really screw that up during my testing? I am sorry about that.
>
> I tried to wrap my head around, where the newline is implicitly appended, and
> only found the definitions below.
>
> include/linux/pci.h:#define pci_info(pdev, fmt, arg...)
> dev_info(&(pdev)->dev, fmt, ##arg)
>
> include/linux/dev_printk.h:#define dev_info(dev, fmt, ...)
> \
> include/linux/dev_printk.h: _dev_info(dev, dev_fmt(fmt),
> ##__VA_ARGS__)
>
> include/linux/dev_printk.h:__printf(2, 3) __cold
> include/linux/dev_printk.h:void _dev_info(const struct device *dev, const
> char *fmt, ...);
>
> include/linux/compiler_attributes.h:#define __printf(a, b)
> __attribute__((__format__(printf, a, b)))
Yeah, it's not obvious: it happens in kernel/printk/printk.c:vprintk_store
where it does
if (dev_info)
lflags |= LOG_NEWLINE;
It doesn't seem to be documented; I think it prevents using pr_cont with
"rich" printk facilities that go via _dev_info.
I suspect it quietly changed in commit c313af145b9bc ("printk() - isolate
KERN_CONT users from ordinary complete lines").
> In the discussion *smpboot: CPU numbers printed as warning* [1] John wrote:
>
> > It is supported to provide loglevels for CONT messages. The loglevel is
> > then only used if the append fails:
> >
> > pr_cont(KERN_INFO "message part");
> >
> > I don't know if we want to go down that path. But it is supported.
Yeah, I saw that, but decided to go with the 'pr_info("")' solution, because
it is less magic, and already used in two other drivers.
pr_info("") will also prepend 'AMD-Vi:' to the feature list, which is nice.
Best regards,
Alexander
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/amd: Fix extended features logging
print_iommu_info prints the EFR register and then the decoded list of
features on a separate line:
pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
PPR X2APIC NX GT IA GA PC GA_vAPIC
The second line is emitted via 'pr_cont', which causes it to have a
different ('warn') loglevel compared to the previous line ('info').
Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
from the pci_info format string, but this doesn't work, as pci_info
calls implicitly append a newline anyway.
Restore the newline, and call pr_info with empty format string to set
the loglevel for subsequent pr_cont calls. The same solution is used in
EFI and uvesafb drivers.
Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
divergent log levels")
Signed-off-by: Alexander Monakov
Cc: Paul Menzel
Cc: Joerg Roedel
Cc: Suravee Suthikulpanit
Cc: iommu@lists.linux-foundation.org
---
drivers/iommu/amd/init.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 596d0c413473..a25e241eff1c 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1929,8 +1929,11 @@ static void print_iommu_info(void)
pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pci_info(pdev, "Extended features (%#llx):",
+ pci_info(pdev, "Extended features (%#llx):\n",
iommu->features);
+
+ pr_info("");
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
if (iommu_feature(iommu, (1ULL << i)))
pr_cont(" %s", feat_str[i]);
--
2.30.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] Revert "iommu/amd: Fix performance counter initialization"
On Wed, 3 Mar 2021, Suravee Suthikulpanit wrote: > > Additionally, alternative proposed solutions [1] were not considered or > > discussed. > > > > [1]:https://lore.kernel.org/linux-iommu/alpine.lnx.2.20.13.2006030935570.3...@monopod.intra.ispras.ru/ > > This check has been introduced early on to detect a HW issue for > certain platforms in the past, where the performance counters are not > accessible and would result in silent failure when try to use the > counters. This is considered legacy code, and can be removed if we > decide to no longer provide sanity check for such case. Which platforms? There is no such information in the code or the commit messages that introduced this. According to AMD's documentation, presence of performance counters is indicated by "PCSup" bit in the "EFR" register. I don't think the driver should second-guess that. If there were platforms where the CPU or the firmware lied to the OS (EFR[PCSup] was 1, but counters were not present), I think that should have been handled in a more explicit manner, e.g. via matching broken CPUs by cpuid. Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
On Tue, 16 Jun 2020, Suravee Suthikulpanit wrote: > > > Instead of blindly moving the code around to a spot that would just work, > > > I am trying to understand what might be required here. In this case, > > > the init_device_table_dma()should not be needed. I suspect it's the IOMMU > > > invalidate all command that's also needed here. > > > > > > I'm also checking with the HW and BIOS team. Meanwhile, could you please > > > give > > > the following change a try: > > Hello. Can you give any update please? > > > > Alexander > > > > Sorry for late reply. I have a reproducer and working with the HW team to > understand the issue. > I should be able to provide update with solution by the end of this week. Hello, hope you are doing well. Has this investigation found anything? Thanks. Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
On Tue, 16 Jun 2020, Suravee Suthikulpanit wrote: > > > On 6/1/20 4:01 PM, Alexander Monakov wrote: > > > > On Mon, 1 Jun 2020, Suravee Suthikulpanit wrote: > > > > > > > > > > Moving init_iommu_perf_ctr just after iommu_flush_all_caches > > > > > > resolves the issue. This is the earliest point in amd_iommu_init_pci > > > > > > where the call succeeds on my laptop. > > > > > According to your description, it should just need to be anywhere > > > > > after the pci_enable_device() is called for the IOMMU device, isn't > > > > > it? So, on your system, what if we just move the init_iommu_perf_ctr() > > > > > here: > > > > No, this doesn't work, as I already said in the paragraph you are > > > > responding to. See my last sentence in the quoted part. > > > > > > > > So the implication is init_device_table_dma together with subsequent > > > > cache flush is also setting up something that is necessary for counters > > > > to be writable. > > > > > > > > Alexander > > > > > > > Instead of blindly moving the code around to a spot that would just work, > > > I am trying to understand what might be required here. In this case, > > > the init_device_table_dma()should not be needed. I suspect it's the IOMMU > > > invalidate all command that's also needed here. > > > > > > I'm also checking with the HW and BIOS team. Meanwhile, could you please > > > give the following change a try: > > Hello. Can you give any update please? > > > > Alexander > > > > Sorry for late reply. I have a reproducer and working with the HW team to > understand the issue. > I should be able to provide update with solution by the end of this week. Hi, can you share any information (two more weeks have passed)? Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
On Mon, 1 Jun 2020, Suravee Suthikulpanit wrote: > Alexander > > On 6/1/20 4:01 PM, Alexander Monakov wrote: > > On Mon, 1 Jun 2020, Suravee Suthikulpanit wrote: > > > > > > Moving init_iommu_perf_ctr just after iommu_flush_all_caches resolves > > > > the issue. This is the earliest point in amd_iommu_init_pci where the > > > > call succeeds on my laptop. > > > > > > According to your description, it should just need to be anywhere after > > > the > > > pci_enable_device() is called for the IOMMU device, isn't it? So, on your > > > system, what if we just move the init_iommu_perf_ctr() here: > > > > No, this doesn't work, as I already said in the paragraph you are responding > > to. See my last sentence in the quoted part. > > > > So the implication is init_device_table_dma together with subsequent cache > > flush is also setting up something that is necessary for counters to be > > writable. > > > > Alexander > > > > Instead of blindly moving the code around to a spot that would just work, > I am trying to understand what might be required here. In this case, > the init_device_table_dma()should not be needed. I suspect it's the IOMMU > invalidate all command that's also needed here. > > I'm also checking with the HW and BIOS team. Meanwhile, could you please give > the following change a try: Hello. Can you give any update please? Alexander > diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c > index 5b81fd16f5fa..b07458cc1b0b 100644 > --- a/drivers/iommu/amd_iommu_init.c > +++ b/drivers/iommu/amd_iommu_init.c > @@ -1875,6 +1875,8 @@ static int __init amd_iommu_init_pci(void) > ret = iommu_init_pci(iommu); > if (ret) > break; > + iommu_flush_all_caches(iommu); > + init_iommu_perf_ctr(iommu); > } > > /* > -- > 2.17.1 > > Thanks, > Suravee > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
On Tue, 2 Jun 2020, Shuah Khan wrote: > I changed the logic to read config to get max banks and counters > before checking if counters are writable and tried writing to all. > The result is the same and all of them aren't writable. However, > when disable the writable check and assume they are, I can run [snip] This is similar to what I did. I also noticed that counters can be successfully used with perf if the initial check is ignored. I was considering sending a patch to remove the check and adjust the event counting logic to use counters as read-only, but after a bit more investigation I've noticed how late pci_enable_device is done, and came up with this patch. It's a path of less resistance: I'd expect maintainers to be more averse to removing the check rather than fixing it so it works as intended (even though I think the check should not be there in the first place). However: The ability to modify the counters is needed only for sampling the events (getting an interrupt when a counter overflows). There's no code to do that for these AMD IOMMU counters. A solution I would prefer is to not write to those counters at all. It would simplify or even remove a bunch of code. I can submit a corresponding patch if there's general agreement this path is ok. What do you think? Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
> Instead of blindly moving the code around to a spot that would just work, > I am trying to understand what might be required here. In this case, > the init_device_table_dma()should not be needed. I suspect it's the IOMMU > invalidate all command that's also needed here. > > I'm also checking with the HW and BIOS team. Meanwhile, could you please give > the following change a try: Yes, this also fixes the problem for me. Alexander > > diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c > index 5b81fd16f5fa..b07458cc1b0b 100644 > --- a/drivers/iommu/amd_iommu_init.c > +++ b/drivers/iommu/amd_iommu_init.c > @@ -1875,6 +1875,8 @@ static int __init amd_iommu_init_pci(void) > ret = iommu_init_pci(iommu); > if (ret) > break; > + iommu_flush_all_caches(iommu); > + init_iommu_perf_ctr(iommu); > } > > /* ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
On Mon, 1 Jun 2020, Suravee Suthikulpanit wrote: > > Moving init_iommu_perf_ctr just after iommu_flush_all_caches resolves > > the issue. This is the earliest point in amd_iommu_init_pci where the > > call succeeds on my laptop. > > According to your description, it should just need to be anywhere after the > pci_enable_device() is called for the IOMMU device, isn't it? So, on your > system, what if we just move the init_iommu_perf_ctr() here: No, this doesn't work, as I already said in the paragraph you are responding to. See my last sentence in the quoted part. So the implication is init_device_table_dma together with subsequent cache flush is also setting up something that is necessary for counters to be writable. Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Fix event counter availability check
Hi,
Adding Shuah Khan to Cc: I've noticed you've seen this issue on Ryzen 2400GE;
can you have a look at the patch? Would be nice to know if it fixes the
problem for you too.
Thanks.
Alexander
On Fri, 29 May 2020, Alexander Monakov wrote:
> The driver performs an extra check if the IOMMU's capabilities advertise
> presence of performance counters: it verifies that counters are writable
> by writing a hard-coded value to a counter and testing that reading that
> counter gives back the same value.
>
> Unfortunately it does so quite early, even before pci_enable_device is
> called for the IOMMU, i.e. when accessing its MMIO space is not
> guaranteed to work. On Ryzen 4500U CPU, this actually breaks the test:
> the driver assumes the counters are not writable, and disables the
> functionality.
>
> Moving init_iommu_perf_ctr just after iommu_flush_all_caches resolves
> the issue. This is the earliest point in amd_iommu_init_pci where the
> call succeeds on my laptop.
>
> Signed-off-by: Alexander Monakov
> Cc: Joerg Roedel
> Cc: Suravee Suthikulpanit
> Cc: iommu@lists.linux-foundation.org
> ---
>
> PS. I'm seeing another hiccup with IOMMU probing on my system:
> pci :00:00.2: can't derive routing for PCI INT A
> pci :00:00.2: PCI INT A: not connected
>
> Hopefully I can figure it out, but I'd appreciate hints.
>
> drivers/iommu/amd_iommu_init.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 5b81fd16f5fa..1b7ec6b6a282 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1788,8 +1788,6 @@ static int __init iommu_init_pci(struct amd_iommu
> *iommu)
> if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE))
> amd_iommu_np_cache = true;
>
> - init_iommu_perf_ctr(iommu);
> -
> if (is_rd890_iommu(iommu->dev)) {
> int i, j;
>
> @@ -1891,8 +1889,10 @@ static int __init amd_iommu_init_pci(void)
>
> init_device_table_dma();
>
> - for_each_iommu(iommu)
> + for_each_iommu(iommu) {
> iommu_flush_all_caches(iommu);
> + init_iommu_perf_ctr(iommu);
> + }
>
> if (!ret)
> print_iommu_info();
>
> base-commit: 75caf310d16cc5e2f851c048cd597f5437013368
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/amd: Fix event counter availability check
The driver performs an extra check if the IOMMU's capabilities advertise
presence of performance counters: it verifies that counters are writable
by writing a hard-coded value to a counter and testing that reading that
counter gives back the same value.
Unfortunately it does so quite early, even before pci_enable_device is
called for the IOMMU, i.e. when accessing its MMIO space is not
guaranteed to work. On Ryzen 4500U CPU, this actually breaks the test:
the driver assumes the counters are not writable, and disables the
functionality.
Moving init_iommu_perf_ctr just after iommu_flush_all_caches resolves
the issue. This is the earliest point in amd_iommu_init_pci where the
call succeeds on my laptop.
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: Suravee Suthikulpanit
Cc: iommu@lists.linux-foundation.org
---
PS. I'm seeing another hiccup with IOMMU probing on my system:
pci :00:00.2: can't derive routing for PCI INT A
pci :00:00.2: PCI INT A: not connected
Hopefully I can figure it out, but I'd appreciate hints.
drivers/iommu/amd_iommu_init.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 5b81fd16f5fa..1b7ec6b6a282 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1788,8 +1788,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE))
amd_iommu_np_cache = true;
- init_iommu_perf_ctr(iommu);
-
if (is_rd890_iommu(iommu->dev)) {
int i, j;
@@ -1891,8 +1889,10 @@ static int __init amd_iommu_init_pci(void)
init_device_table_dma();
- for_each_iommu(iommu)
+ for_each_iommu(iommu) {
iommu_flush_all_caches(iommu);
+ init_iommu_perf_ctr(iommu);
+ }
if (!ret)
print_iommu_info();
base-commit: 75caf310d16cc5e2f851c048cd597f5437013368
--
2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
AMD IOMMU perf counters on Zen2
Hello, I'd like to use IOMMU perf counters on a Zen 2 CPU (Ryzen 4500U, Renoir SoC). Unfortunately, init_iommu_perf_ctr fails because val2 != val, i.e. the counter appears not writable. However, if I patch the kernel to skip this check, the counters seem to increment when configured with perf tool. Do you know why the counter might appear not writable in newer CPUs? Thanks. Alexander ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/amd: fix over-read of ACPI UID from IVRS table
IVRS parsing code always tries to read 255 bytes from memory when
retrieving ACPI device path, and makes an assumption that firmware
provides a zero-terminated string. Both of those are bugs: the entry
is likely to be shorter than 255 bytes, and zero-termination is not
guaranteed.
With Acer SF314-42 firmware these issues manifest visibly in dmesg:
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1...
The first three lines show how the code over-reads adjacent table
entries into the UID, and in the last line it even reads garbage data
beyond the end of the IVRS table itself.
Since each entry has the length of the UID (uidl member of ivhd_entry
struct), use that for memcpy, and manually add a zero terminator.
Avoid zero-filling hid and uid arrays up front, and instead ensure
the uid array is always zero-terminated. No change needed for the hid
array, as it was already properly zero-terminated.
Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type
HID")
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
---
drivers/iommu/amd_iommu_init.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 6be3853a5d97..faed3d35017a 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1329,8 +1329,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
}
case IVHD_DEV_ACPI_HID: {
u16 devid;
- u8 hid[ACPIHID_HID_LEN] = {0};
- u8 uid[ACPIHID_UID_LEN] = {0};
+ u8 hid[ACPIHID_HID_LEN];
+ u8 uid[ACPIHID_UID_LEN];
int ret;
if (h->type != 0x40) {
@@ -1347,6 +1347,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
+ uid[0] = '\0';
switch (e->uidf) {
case UID_NOT_PRESENT:
@@ -1361,8 +1362,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
case UID_IS_CHARACTER:
- memcpy(uid, (u8 *)(>uid), ACPIHID_UID_LEN -
1);
- uid[ACPIHID_UID_LEN - 1] = '\0';
+ memcpy(uid, >uid, e->uidl);
+ uid[e->uidl] = '\0';
break;
default:
--
2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
