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 +0000)
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 +0000)
----------------------------------------------------------------
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(-)
On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote:
> On Wed, Dec 9, 2020 at 6:12 AM Will Deacon <[email protected]> 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
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 <[email protected]> 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
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Wed, Dec 9, 2020 at 12:12 PM Jerry Snitselaar <[email protected]> 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 <[email protected]> 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
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
On Wed, Dec 9, 2020 at 11:12 AM Jerry Snitselaar <[email protected]> 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
On Wed, Dec 9, 2020 at 6:12 AM Will Deacon <[email protected]> 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
On Wed, Dec 9, 2020 at 12:18 PM Linus Torvalds
<[email protected]> wrote:
>
> On Wed, Dec 9, 2020 at 11:12 AM Jerry Snitselaar <[email protected]> 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.
The pull request you sent on Wed, 9 Dec 2020 14:12:38 +0000:
> 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
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 <[email protected]> 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