Re: [GIT PULL] IOMMU fix for 5.10 (-final)
Hi All, On 12/10/20 1:50 AM, Will Deacon wrote: On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote: On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix for a fix, where the size of the interrupt remapping table was increased but a related constant for the size of the interrupt table was forgotten. Pulled. Thanks. However, why didn't this then add some sanity checking for the two different #defines to be in sync? IOW, something like #define AMD_IOMMU_IRQ_TABLE_SHIFT 9 #define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) #define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) or whatever. Hmm? This looks like a worthwhile change to me, but I don't have any hardware so I've been very reluctant to make even "obvious" driver changes here. Suravee -- please can you post a patch implementing the above? I'll send one out ASAP. That way this won't happen again, but perhaps equally importantly the linkage will be more clear, and there won't be those random constants. Naming above is probably garbage - I assume there's some actual architectural name for that irq table length field in the DTE? The one in the spec is even better: "IntTabLen". Will Thanks, Suravee
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 9, 2020 at 12:18 PM Linus Torvalds wrote: > > On Wed, Dec 9, 2020 at 11:12 AM Jerry Snitselaar wrote: > > > > Since the field in the device table entry format expects it to be n > > where there are 2^n entries in the table I guess it should be: > > > > #define DTE_IRQ_TABLE_LEN 9 > > #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) > > No, that "DTE_IRQ_TABLE_LEN" is not the size shift - it's the size > shift value in that DTE field, which is shifted up by 1. > > That's why the current code does that > >#define DTE_IRQ_TABLE_LEN (9ULL << 1) > > there.. > > Which was why I suggested that new #define that is the *actual* shift > value, and then the DTE thing and the MAX_IRQS_PER_TABLE values would > depend on that. > >Linus > Yes, when I read it my head was translating it as setting them both to 512 and then I forgot that it gets shifted over 1. Which considering I was the once who noticed the original problem of it still being 8 was a nice brain fart. This should be fixed like you suggest.
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 9, 2020 at 11:12 AM Jerry Snitselaar wrote: > > Since the field in the device table entry format expects it to be n > where there are 2^n entries in the table I guess it should be: > > #define DTE_IRQ_TABLE_LEN 9 > #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) No, that "DTE_IRQ_TABLE_LEN" is not the size shift - it's the size shift value in that DTE field, which is shifted up by 1. That's why the current code does that #define DTE_IRQ_TABLE_LEN (9ULL << 1) there.. Which was why I suggested that new #define that is the *actual* shift value, and then the DTE thing and the MAX_IRQS_PER_TABLE values would depend on that. Linus
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 9, 2020 at 12:12 PM Jerry Snitselaar wrote: > > > Will Deacon @ 2020-12-09 11:50 MST: > > > On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote: > >> On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: > >> > > >> > Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix > >> > for a fix, where the size of the interrupt remapping table was increased > >> > but a related constant for the size of the interrupt table was forgotten. > >> > >> Pulled. > > > > Thanks. > > > >> However, why didn't this then add some sanity checking for the two > >> different #defines to be in sync? > >> > >> IOW, something like > >> > >>#define AMD_IOMMU_IRQ_TABLE_SHIFT 9 > >> > >>#define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) > >>#define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) > > Since the field in the device table entry format expects it to be n > where there are 2^n entries in the table I guess it should be: > > #define DTE_IRQ_TABLE_LEN 9 > #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) > No, ignore that. I'm being stupid. > >> > >> or whatever. Hmm? > > > > This looks like a worthwhile change to me, but I don't have any hardware > > so I've been very reluctant to make even "obvious" driver changes here. > > > > Suravee -- please can you post a patch implementing the above? > > > >> That way this won't happen again, but perhaps equally importantly the > >> linkage will be more clear, and there won't be those random constants. > >> > >> Naming above is probably garbage - I assume there's some actual > >> architectural name for that irq table length field in the DTE? > > > > The one in the spec is even better: "IntTabLen". > > > > Will > > ___ > > iommu mailing list > > io...@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/iommu >
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
Will Deacon @ 2020-12-09 11:50 MST: > On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote: >> On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: >> > >> > Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix >> > for a fix, where the size of the interrupt remapping table was increased >> > but a related constant for the size of the interrupt table was forgotten. >> >> Pulled. > > Thanks. > >> However, why didn't this then add some sanity checking for the two >> different #defines to be in sync? >> >> IOW, something like >> >>#define AMD_IOMMU_IRQ_TABLE_SHIFT 9 >> >>#define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) >>#define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) Since the field in the device table entry format expects it to be n where there are 2^n entries in the table I guess it should be: #define DTE_IRQ_TABLE_LEN 9 #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) >> >> or whatever. Hmm? > > This looks like a worthwhile change to me, but I don't have any hardware > so I've been very reluctant to make even "obvious" driver changes here. > > Suravee -- please can you post a patch implementing the above? > >> That way this won't happen again, but perhaps equally importantly the >> linkage will be more clear, and there won't be those random constants. >> >> Naming above is probably garbage - I assume there's some actual >> architectural name for that irq table length field in the DTE? > > The one in the spec is even better: "IntTabLen". > > Will > ___ > iommu mailing list > io...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote: > On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: > > > > Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix > > for a fix, where the size of the interrupt remapping table was increased > > but a related constant for the size of the interrupt table was forgotten. > > Pulled. Thanks. > However, why didn't this then add some sanity checking for the two > different #defines to be in sync? > > IOW, something like > >#define AMD_IOMMU_IRQ_TABLE_SHIFT 9 > >#define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) >#define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) > > or whatever. Hmm? This looks like a worthwhile change to me, but I don't have any hardware so I've been very reluctant to make even "obvious" driver changes here. Suravee -- please can you post a patch implementing the above? > That way this won't happen again, but perhaps equally importantly the > linkage will be more clear, and there won't be those random constants. > > Naming above is probably garbage - I assume there's some actual > architectural name for that irq table length field in the DTE? The one in the spec is even better: "IntTabLen". Will
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
The pull request you sent on Wed, 9 Dec 2020 14:12:38 +: > git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git tags/iommu-fixes has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/ca4bbdaf171604841f77648a2877e2e43db69b71 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: > > Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix > for a fix, where the size of the interrupt remapping table was increased > but a related constant for the size of the interrupt table was forgotten. Pulled. However, why didn't this then add some sanity checking for the two different #defines to be in sync? IOW, something like #define AMD_IOMMU_IRQ_TABLE_SHIFT 9 #define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) #define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) or whatever. Hmm? That way this won't happen again, but perhaps equally importantly the linkage will be more clear, and there won't be those random constants. Naming above is probably garbage - I assume there's some actual architectural name for that irq table length field in the DTE? Linus
[GIT PULL] IOMMU fix for 5.10 (-final)
Hi Linus, Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix for a fix, where the size of the interrupt remapping table was increased but a related constant for the size of the interrupt table was forgotten. Cheers, Will --->8 The following changes since commit d76b42e92780c3587c1a998a3a943b501c137553: iommu/vt-d: Don't read VCCAP register unless it exists (2020-11-26 14:50:24 +) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git tags/iommu-fixes for you to fetch changes up to 4165bf015ba9454f45beaad621d16c516d5c5afe: iommu/amd: Set DTE[IntTabLen] to represent 512 IRTEs (2020-12-07 11:00:24 +) iommu fix for 5.10 - Fix interrupt table length definition for AMD IOMMU Suravee Suthikulpanit (1): iommu/amd: Set DTE[IntTabLen] to represent 512 IRTEs drivers/iommu/amd/amd_iommu_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
