[PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[Sairaj Kodilkar]

This series provides few bug fixes for emulated AMD IOMMU. The series is based
on top of qemu upstream master commit d922088eb4ba.

Patch 1: The code was using wrong DTE field to determine interrupt passthrough.
         Hence replaced it with correct field according to [1].

Patch 2: Current code sets the PCI capability BAR low and high to the
         lower and upper 16 bits of AMDVI_BASE_ADDR respectively, which is
         wrong. Instead use 32 bit mask to set the PCI capability BAR low and
         high.
         The guest IOMMU driver works with current qemu code because it uses
         base address from the IVRS table and not the one provided by
         PCI capability.

Sairaj Kodilkar (2):
  amd_iommu: Use correct DTE field for interrupt passthrough
  amd_iommu: Use correct bitmask to set capability BAR

 hw/i386/amd_iommu.c | 10 +++++-----
 hw/i386/amd_iommu.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] amd_iommu: Use correct DTE field for interrupt passthrough

[Sairaj Kodilkar]

Interrupt passthrough is determine by the bits 191,190,187-184.
These bits are part of the 3rd quad word (i.e. index 2) in DTE. Hence
replace dte[3] by dte[2].

Fixes: b44159fe0 ("x86_iommu/amd: Add interrupt remap support when VAPIC is not enabled")
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 6b13ce894b1a..98f1209a3818 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1309,15 +1309,15 @@ static int amdvi_int_remap_msi(AMDVIState *iommu,
         ret = -AMDVI_IR_ERR;
         break;
     case AMDVI_IOAPIC_INT_TYPE_NMI:
-        pass = dte[3] & AMDVI_DEV_NMI_PASS_MASK;
+        pass = dte[2] & AMDVI_DEV_NMI_PASS_MASK;
         trace_amdvi_ir_delivery_mode("nmi");
         break;
     case AMDVI_IOAPIC_INT_TYPE_INIT:
-        pass = dte[3] & AMDVI_DEV_INT_PASS_MASK;
+        pass = dte[2] & AMDVI_DEV_INT_PASS_MASK;
         trace_amdvi_ir_delivery_mode("init");
         break;
     case AMDVI_IOAPIC_INT_TYPE_EINT:
-        pass = dte[3] & AMDVI_DEV_EINT_PASS_MASK;
+        pass = dte[2] & AMDVI_DEV_EINT_PASS_MASK;
         trace_amdvi_ir_delivery_mode("eint");
         break;
     default:
-- 
2.34.1

[PATCH 2/2] amd_iommu: Use correct bitmask to set capability BAR

[Sairaj Kodilkar]

AMD IOMMU provides the base address of control registers through
IVRS table and PCI capability. Since this base address is of 64 bit,
use 32 bits mask (instead of 16 bits) to set BAR low and high.

Fixes: d29a09ca68 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 4 ++--
 hw/i386/amd_iommu.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 98f1209a3818..044fe432567d 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1593,9 +1593,9 @@ static void amdvi_pci_realize(PCIDevice *pdev, Error **errp)
     /* reset AMDVI specific capabilities, all r/o */
     pci_set_long(pdev->config + s->capab_offset, AMDVI_CAPAB_FEATURES);
     pci_set_long(pdev->config + s->capab_offset + AMDVI_CAPAB_BAR_LOW,
-                 AMDVI_BASE_ADDR & ~(0xffff0000));
+                 AMDVI_BASE_ADDR & MAKE_64BIT_MASK(14, 18));
     pci_set_long(pdev->config + s->capab_offset + AMDVI_CAPAB_BAR_HIGH,
-                (AMDVI_BASE_ADDR & ~(0xffff)) >> 16);
+                AMDVI_BASE_ADDR >> 32);
     pci_set_long(pdev->config + s->capab_offset + AMDVI_CAPAB_RANGE,
                  0xff000000);
     pci_set_long(pdev->config + s->capab_offset + AMDVI_CAPAB_MISC, 0);
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index e0dac4d9a96c..28125130c6fc 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -187,7 +187,7 @@
         AMDVI_CAPAB_FLAG_HTTUNNEL |  AMDVI_CAPAB_EFR_SUP)
 
 /* AMDVI default address */
-#define AMDVI_BASE_ADDR 0xfed80000
+#define AMDVI_BASE_ADDR 0xfed80000ULL
 
 /* page management constants */
 #define AMDVI_PAGE_SHIFT 12
-- 
2.34.1

Re: [PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[Arun Kodilkar, Sairaj]

Hi all,

Accidentally added v2 tag. Please ignore it, this is version 1.

On 2/7/2025 10:23 AM, Sairaj Kodilkar wrote:
> This series provides few bug fixes for emulated AMD IOMMU. The series is based
> on top of qemu upstream master commit d922088eb4ba.
> 
> Patch 1: The code was using wrong DTE field to determine interrupt passthrough.
>           Hence replaced it with correct field according to [1].
> 
> Patch 2: Current code sets the PCI capability BAR low and high to the
>           lower and upper 16 bits of AMDVI_BASE_ADDR respectively, which is
>           wrong. Instead use 32 bit mask to set the PCI capability BAR low and
>           high.
>           The guest IOMMU driver works with current qemu code because it uses
>           base address from the IVRS table and not the one provided by
>           PCI capability.
> 
> Sairaj Kodilkar (2):
>    amd_iommu: Use correct DTE field for interrupt passthrough
>    amd_iommu: Use correct bitmask to set capability BAR
> 
>   hw/i386/amd_iommu.c | 10 +++++-----
>   hw/i386/amd_iommu.h |  2 +-
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 

Re: [PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[Michael Tokarev]

07.02.2025 07:53, Sairaj Kodilkar wrote:
> This series provides few bug fixes for emulated AMD IOMMU. The series is based
> on top of qemu upstream master commit d922088eb4ba.
> 
> Patch 1: The code was using wrong DTE field to determine interrupt passthrough.
>           Hence replaced it with correct field according to [1].
> 
> Patch 2: Current code sets the PCI capability BAR low and high to the
>           lower and upper 16 bits of AMDVI_BASE_ADDR respectively, which is
>           wrong. Instead use 32 bit mask to set the PCI capability BAR low and
>           high.
>           The guest IOMMU driver works with current qemu code because it uses
>           base address from the IVRS table and not the one provided by
>           PCI capability.
> 
> Sairaj Kodilkar (2):
>    amd_iommu: Use correct DTE field for interrupt passthrough
>    amd_iommu: Use correct bitmask to set capability BAR
> 
>   hw/i386/amd_iommu.c | 10 +++++-----
>   hw/i386/amd_iommu.h |  2 +-
>   2 files changed, 6 insertions(+), 6 deletions(-)

Is this qemu-stable material (current series: 7.2, 8.2, 9.2)?

3684717b74 "amd_iommu: Use correct bitmask to set capability BAR" does
not apply to 7.2, since v8.0.0-10-g6291a28645 "hw/i386/amd_iommu: Explicit
use of AMDVI_BASE_ADDR in amdvi_init" in not in 7.2, but the change can be
adjusted for 7.2 easily, or 6291a28645 can be picked up too.

Thanks,

/mjt

Re: [PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[Vasant Hegde]

Hi Michael,


On 2/25/2025 2:17 PM, Michael Tokarev wrote:
> 07.02.2025 07:53, Sairaj Kodilkar wrote:
>> This series provides few bug fixes for emulated AMD IOMMU. The series is based
>> on top of qemu upstream master commit d922088eb4ba.
>>
>> Patch 1: The code was using wrong DTE field to determine interrupt passthrough.
>>           Hence replaced it with correct field according to [1].
>>
>> Patch 2: Current code sets the PCI capability BAR low and high to the
>>           lower and upper 16 bits of AMDVI_BASE_ADDR respectively, which is
>>           wrong. Instead use 32 bit mask to set the PCI capability BAR low and
>>           high.
>>           The guest IOMMU driver works with current qemu code because it uses
>>           base address from the IVRS table and not the one provided by
>>           PCI capability.
>>
>> Sairaj Kodilkar (2):
>>    amd_iommu: Use correct DTE field for interrupt passthrough
>>    amd_iommu: Use correct bitmask to set capability BAR
>>
>>   hw/i386/amd_iommu.c | 10 +++++-----
>>   hw/i386/amd_iommu.h |  2 +-
>>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> Is this qemu-stable material (current series: 7.2, 8.2, 9.2)?

Linux kernel doesn't use these changes. So its fine. But I believe we care for
other OS as well? if yes then better to backport.

> 
> 3684717b74 "amd_iommu: Use correct bitmask to set capability BAR" does
> not apply to 7.2, since v8.0.0-10-g6291a28645 "hw/i386/amd_iommu: Explicit
> use of AMDVI_BASE_ADDR in amdvi_init" in not in 7.2, but the change can be
> adjusted for 7.2 easily, or 6291a28645 can be picked up too.

How is this works? You will pick it up -OR- you want us to backport and send it
to stable mailing list?

-Vasant

Re: [PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[Michael Tokarev]

26.02.2025 15:53, Vasant Hegde wrote:
> Hi Michael,

Hi!

> On 2/25/2025 2:17 PM, Michael Tokarev wrote:
...>> Is this qemu-stable material (current series: 7.2, 8.2, 9.2)?
> 
> Linux kernel doesn't use these changes. So its fine. But I believe we care for
> other OS as well? if yes then better to backport.

Yes, we definitely care about other OSes.  There are numerous possible
other questions though.  For example, how relevant these changes are
for older 7.2.x series, where AMD IOMMU is in less current state (missing
all further development) so might not be as relevant anymore.

>> 3684717b74 "amd_iommu: Use correct bitmask to set capability BAR" does
>> not apply to 7.2, since v8.0.0-10-g6291a28645 "hw/i386/amd_iommu: Explicit
>> use of AMDVI_BASE_ADDR in amdvi_init" in not in 7.2, but the change can be
>> adjusted for 7.2 easily, or 6291a28645 can be picked up too.
> 
> How is this works? You will pick it up -OR- you want us to backport and send it
> to stable mailing list?

This is just a data point, nothing more.  Indicating that for 7.2, it needs some
more work.  I picked it up for 7.2 already: https://gitlab.com/mjt0k/qemu/-/tree/staging-7.2
But this is more mechanical way, maybe you, who know this area much better than
me, prefer other way, like picking up already mentioned commit 6291a28645.
Or maybe it isn't worth the effort for 7.2 anyway, provided the issue isn't
that important and it needs any additional work to back-port.

If you especially care about some older stable releases and think one or
another change really needs to be there *and* needs some backporting work,
you might do a backport yourself or give some notes for me to do that.

It's always a trade-off between "importance" of the change, age of the
stable series, the amount of work needed for backporting, and possibility
of breakage.  For less-important or less-used stuff, even thinking about
this tradeoff is already too much work ;)

Thanks,

/mjt

Re: [PATCH v2 0/2] Emulated AMD IOMMU cleanup and fixes

[Vasant Hegde]

Hi,


On 2/26/2025 8:52 PM, Michael Tokarev wrote:
> 26.02.2025 15:53, Vasant Hegde wrote:
>> Hi Michael,
> 
> Hi!
> 
>> On 2/25/2025 2:17 PM, Michael Tokarev wrote:
> ...>> Is this qemu-stable material (current series: 7.2, 8.2, 9.2)?
>>
>> Linux kernel doesn't use these changes. So its fine. But I believe we care for
>> other OS as well? if yes then better to backport.
> 
> Yes, we definitely care about other OSes.  There are numerous possible
> other questions though.  For example, how relevant these changes are
> for older 7.2.x series, where AMD IOMMU is in less current state (missing
> all further development) so might not be as relevant anymore.
> 
>>> 3684717b74 "amd_iommu: Use correct bitmask to set capability BAR" does
>>> not apply to 7.2, since v8.0.0-10-g6291a28645 "hw/i386/amd_iommu: Explicit
>>> use of AMDVI_BASE_ADDR in amdvi_init" in not in 7.2, but the change can be
>>> adjusted for 7.2 easily, or 6291a28645 can be picked up too.
>>
>> How is this works? You will pick it up -OR- you want us to backport and send it
>> to stable mailing list?
> 
> This is just a data point, nothing more.  Indicating that for 7.2, it needs some
> more work.  I picked it up for 7.2 already: https://gitlab.com/mjt0k/qemu/-/
> tree/staging-7.2

Thanks. Looks good.

> But this is more mechanical way, maybe you, who know this area much better than
> me, prefer other way, like picking up already mentioned commit 6291a28645.
> Or maybe it isn't worth the effort for 7.2 anyway, provided the issue isn't
> that important and it needs any additional work to back-port.
> 
> If you especially care about some older stable releases and think one or
> another change really needs to be there *and* needs some backporting work,
> you might do a backport yourself or give some notes for me to do that.
> 
> It's always a trade-off between "importance" of the change, age of the
> stable series, the amount of work needed for backporting, and possibility
> of breakage.  For less-important or less-used stuff, even thinking about
> this tradeoff is already too much work ;)

:-)

Thanks for detailed explanation. Next time, if its not applying cleanly we can
backport it and give it to you.

-Vasant