[v3] amd_iommu: Fixes to align with AMDVi specification

[PATCH v3 0/7] amd_iommu: Fixes to align with AMDVi specification

Posted by Alejandro Jimenez 5 months, 2 weeks ago

The main reason for sending this new revision so soon is that v2 included a
duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
removing the old patch. Apologies for the mistake.

Additional changes in v3:
- Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/Miscellaneous/).
- Added 'Fixes:' tag to [PATCH 5/7].
- Added Vasant's R-b to patches 4,5,7.

Thank you,
Alejandro

v2:
https://lore.kernel.org/qemu-devel/20250528221725.3554040-1-alejandro.j.jimenez@oracle.com/

v1:
https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/


Alejandro Jimenez (7):
  amd_iommu: Fix Miscellaneous Information Register 0 offsets
  amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
  amd_iommu: Update bitmasks representing DTE reserved fields
  amd_iommu: Fix masks for various IOMMU MMIO Registers
  amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
  amd_iommu: Fix the calculation for Device Table size
  amd_iommu: Remove duplicated definitions

 hw/i386/amd_iommu.c | 15 ++++++------
 hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
 2 files changed, 37 insertions(+), 37 deletions(-)


base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
-- 
2.43.5

Re: [PATCH v3 0/7] amd_iommu: Fixes to align with AMDVi specification

Posted by Ethan MILON 5 months ago

Hi,

Is this series the right place to include the following minor fix?

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 0775c..18d30e1 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
uint64_t val)
 {
     uint64_t romask = ldq_le_p(&s->romask[addr]);
     uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
-    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
+    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
     stq_le_p(&s->mmior[addr],
             ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
 }

This corrects the type of oldval to match the return type of ldq_le_p().

Thanks,
Ethan

On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> The main reason for sending this new revision so soon is that v2 included a
> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
> removing the old patch. Apologies for the mistake.
> 
> Additional changes in v3:
> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/Miscellaneous/).
> - Added 'Fixes:' tag to [PATCH 5/7].
> - Added Vasant's R-b to patches 4,5,7.
> 
> Thank you,
> Alejandro
> 
> v2:
> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1-alejandro.j.jimenez@oracle.com/
> 
> v1:
> https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/
> 
> 
> Alejandro Jimenez (7):
>   amd_iommu: Fix Miscellaneous Information Register 0 offsets
>   amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
>   amd_iommu: Update bitmasks representing DTE reserved fields
>   amd_iommu: Fix masks for various IOMMU MMIO Registers
>   amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
>   amd_iommu: Fix the calculation for Device Table size
>   amd_iommu: Remove duplicated definitions
> 
>  hw/i386/amd_iommu.c | 15 ++++++------
>  hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
>  2 files changed, 37 insertions(+), 37 deletions(-)
> 
> 
> base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
> --
> 2.43.5
> 
>

Re: [PATCH v3 0/7] amd_iommu: Fixes to align with AMDVi specification

Posted by Alejandro Jimenez 5 months ago

Hi Ethan,

On 6/12/25 4:36 AM, Ethan MILON wrote:
> Hi,
> 
> Is this series the right place to include the following minor fix?
> 

I would defer this change for two reasons:

1) This series has been reviewed and tested already. I was hoping it 
would be included on the Jun 1st pull but I sent v3 too late for that. I 
think it is ready so I would like to leave it as is unless there are any 
objections ...


> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 0775c..18d30e1 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
> uint64_t val)
>   {
>       uint64_t romask = ldq_le_p(&s->romask[addr]);
>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
> -    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
> +    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>       stq_le_p(&s->mmior[addr],
>               ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>   }
> 
> This corrects the type of oldval to match the return type of ldq_le_p().
> 

2) This fix is needed, but it is likely better as part of additional 
changes that are needed to cleanup/fix the XTSup support. i.e. there are 
unhandled writes to the 0x170, 0x178, and 0x180 MMIO offsets, and those 
depend on MMIO 0x18[IntCapXTEn]=1. I think the truncation of oldval that 
you found is causing XTEn and IntCapXTEn bits on the control registers 
to be ignored, but ultimately things are not broken enough (yet). In 
other words, I think there is a lot more work to do in here, and it is 
something I am looking into.

I suspect Vasant might have spotted this problem already, so he might 
even have some fixes queued up...

That being said, if you want to send a patch with your S-b I'll add it 
to this series.

Alejandro

> Thanks,
> Ethan
> 
> On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>>
>>
>> The main reason for sending this new revision so soon is that v2 included a
>> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
>> removing the old patch. Apologies for the mistake.
>>
>> Additional changes in v3:
>> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/Miscellaneous/).
>> - Added 'Fixes:' tag to [PATCH 5/7].
>> - Added Vasant's R-b to patches 4,5,7.
>>
>> Thank you,
>> Alejandro
>>
>> v2:
>> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1-alejandro.j.jimenez@oracle.com/
>>
>> v1:
>> https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/
>>
>>
>> Alejandro Jimenez (7):
>>    amd_iommu: Fix Miscellaneous Information Register 0 offsets
>>    amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
>>    amd_iommu: Update bitmasks representing DTE reserved fields
>>    amd_iommu: Fix masks for various IOMMU MMIO Registers
>>    amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
>>    amd_iommu: Fix the calculation for Device Table size
>>    amd_iommu: Remove duplicated definitions
>>
>>   hw/i386/amd_iommu.c | 15 ++++++------
>>   hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
>>   2 files changed, 37 insertions(+), 37 deletions(-)
>>
>>
>> base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
>> --
>> 2.43.5
>>
>>

Re: [PATCH v3 0/7] amd_iommu: Fixes to align with AMDVi specification

Posted by Philippe Mathieu-Daudé 5 months ago

Hi Alejandro,

On 12/6/25 22:59, Alejandro Jimenez wrote:
> Hi Ethan,
> 
> On 6/12/25 4:36 AM, Ethan MILON wrote:
>> Hi,
>>
>> Is this series the right place to include the following minor fix?
>>
> 
> I would defer this change for two reasons:
> 
> 1) This series has been reviewed and tested already. I was hoping it 
> would be included on the Jun 1st pull but I sent v3 too late for that. I 
> think it is ready so I would like to leave it as is unless there are any 
> objections ...
> 
> 
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 0775c..18d30e1 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
>> uint64_t val)
>>   {
>>       uint64_t romask = ldq_le_p(&s->romask[addr]);
>>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
>> -    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
>> +    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>>       stq_le_p(&s->mmior[addr],
>>               ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>>   }
>>
>> This corrects the type of oldval to match the return type of ldq_le_p().
>>
> 
> 2) This fix is needed, but it is likely better as part of additional 
> changes that are needed to cleanup/fix the XTSup support. i.e. there are 
> unhandled writes to the 0x170, 0x178, and 0x180 MMIO offsets, and those 
> depend on MMIO 0x18[IntCapXTEn]=1. I think the truncation of oldval that 
> you found is causing XTEn and IntCapXTEn bits on the control registers 
> to be ignored, but ultimately things are not broken enough (yet).

I agree with Ethan it is better to avoid hidden truncation, because it
just makes debugging experience harder.

If this is the expected behavior, better add a comment, or use
extract64() which makes the truncation explicit.

Regards,

Phil.

> In 
> other words, I think there is a lot more work to do in here, and it is 
> something I am looking into.
> 
> I suspect Vasant might have spotted this problem already, so he might 
> even have some fixes queued up...
> 
> That being said, if you want to send a patch with your S-b I'll add it 
> to this series.
> 
> Alejandro
> 
>> Thanks,
>> Ethan
>>
>> On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
>>> Caution: External email. Do not open attachments or click links, 
>>> unless this email comes from a known sender and you know the content 
>>> is safe.
>>>
>>>
>>> The main reason for sending this new revision so soon is that v2 
>>> included a
>>> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
>>> removing the old patch. Apologies for the mistake.
>>>
>>> Additional changes in v3:
>>> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/ 
>>> Miscellaneous/).
>>> - Added 'Fixes:' tag to [PATCH 5/7].
>>> - Added Vasant's R-b to patches 4,5,7.
>>>
>>> Thank you,
>>> Alejandro
>>>
>>> v2:
>>> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1- 
>>> alejandro.j.jimenez@oracle.com/
>>>
>>> v1:
>>> https://lore.kernel.org/all/20250311152446.45086-1- 
>>> alejandro.j.jimenez@oracle.com/
>>>
>>>
>>> Alejandro Jimenez (7):
>>>    amd_iommu: Fix Miscellaneous Information Register 0 offsets
>>>    amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
>>>    amd_iommu: Update bitmasks representing DTE reserved fields
>>>    amd_iommu: Fix masks for various IOMMU MMIO Registers
>>>    amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
>>>    amd_iommu: Fix the calculation for Device Table size
>>>    amd_iommu: Remove duplicated definitions
>>>
>>>   hw/i386/amd_iommu.c | 15 ++++++------
>>>   hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
>>>   2 files changed, 37 insertions(+), 37 deletions(-)
>>>
>>>
>>> base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
>>> -- 
>>> 2.43.5
>>>
>>>
> 
>

Re: [PATCH v3 0/7] amd_iommu: Fixes to align with AMDVi specification

Posted by Alejandro Jimenez 5 months ago

Hi Phil,

On 6/16/25 2:59 AM, Philippe Mathieu-Daudé wrote:
> Hi Alejandro,
> 
> On 12/6/25 22:59, Alejandro Jimenez wrote:
>> Hi Ethan,
>>
>> On 6/12/25 4:36 AM, Ethan MILON wrote:
>>> Hi,
>>>
>>> Is this series the right place to include the following minor fix?
>>>
>>
>> I would defer this change for two reasons:
>>
>> 1) This series has been reviewed and tested already. I was hoping it 
>> would be included on the Jun 1st pull but I sent v3 too late for that. 
>> I think it is ready so I would like to leave it as is unless there are 
>> any objections ...
>>
>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index 0775c..18d30e1 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
>>> uint64_t val)
>>>   {
>>>       uint64_t romask = ldq_le_p(&s->romask[addr]);
>>>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
>>> -    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
>>> +    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>>>       stq_le_p(&s->mmior[addr],
>>>               ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>>>   }
>>>
>>> This corrects the type of oldval to match the return type of ldq_le_p().
>>>
>>
>> 2) This fix is needed, but it is likely better as part of additional 
>> changes that are needed to cleanup/fix the XTSup support. i.e. there 
>> are unhandled writes to the 0x170, 0x178, and 0x180 MMIO offsets, and 
>> those depend on MMIO 0x18[IntCapXTEn]=1. I think the truncation of 
>> oldval that you found is causing XTEn and IntCapXTEn bits on the 
>> control registers to be ignored, but ultimately things are not broken 
>> enough (yet).
> 
> I agree with Ethan it is better to avoid hidden truncation, because it
> just makes debugging experience harder.
> 

I agree that Ethan found a bug that must be fixed. I am answering his 
initial question of whether this series is the right place to fix it by 
pointing out that this bug uncovers that there is more to do than just 
fixing this specific error, and it could be included in a series to 
address those larger problems that I mentioned above.

On the other hand, it is probably better just to fix this specific bug 
now since it is simple enough, which is why I asked Ethan to send a 
commit and I will add it (he should get credit)

> If this is the expected behavior, better add a comment, or use
> extract64() which makes the truncation explicit.
> 

It is not the expected behavior, the truncation is a bug. It doesn't yet 
cause any issues because amdvi_writeq() is currently only called to 
handle MMIO writes for a few offsets/register, mostly to 
AMDVI_MMIO_CONTROL, and the romask for the offset is 0. This means that 
the bug doesn't really change the value that is ultimately written to 
the emulated MMIO register, but it could cause problems in the future.

Thank you,
Alejandro


> Regards,
> 
> Phil.
> 
>> In other words, I think there is a lot more work to do in here, and it 
>> is something I am looking into.
>>
>> I suspect Vasant might have spotted this problem already, so he might 
>> even have some fixes queued up...
>>
>> That being said, if you want to send a patch with your S-b I'll add it 
>> to this series.
>>
>> Alejandro
>>
>>> Thanks,
>>> Ethan
>>>
>>> On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
>>>> Caution: External email. Do not open attachments or click links, 
>>>> unless this email comes from a known sender and you know the content 
>>>> is safe.
>>>>
>>>>
>>>> The main reason for sending this new revision so soon is that v2 
>>>> included a
>>>> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
>>>> removing the old patch. Apologies for the mistake.
>>>>
>>>> Additional changes in v3:
>>>> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/ 
>>>> Miscellaneous/).
>>>> - Added 'Fixes:' tag to [PATCH 5/7].
>>>> - Added Vasant's R-b to patches 4,5,7.
>>>>
>>>> Thank you,
>>>> Alejandro
>>>>
>>>> v2:
>>>> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1- 
>>>> alejandro.j.jimenez@oracle.com/
>>>>
>>>> v1:
>>>> https://lore.kernel.org/all/20250311152446.45086-1- 
>>>> alejandro.j.jimenez@oracle.com/
>>>>
>>>>
>>>> Alejandro Jimenez (7):
>>>>    amd_iommu: Fix Miscellaneous Information Register 0 offsets
>>>>    amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
>>>>    amd_iommu: Update bitmasks representing DTE reserved fields
>>>>    amd_iommu: Fix masks for various IOMMU MMIO Registers
>>>>    amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from 
>>>> DTE
>>>>    amd_iommu: Fix the calculation for Device Table size
>>>>    amd_iommu: Remove duplicated definitions
>>>>
>>>>   hw/i386/amd_iommu.c | 15 ++++++------
>>>>   hw/i386/amd_iommu.h | 59 +++++++++++++++++++++ 
>>>> +-----------------------
>>>>   2 files changed, 37 insertions(+), 37 deletions(-)
>>>>
>>>>
>>>> base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
>>>> -- 
>>>> 2.43.5
>>>>
>>>>
>>
>>
>