[PATCH 0/2] amd_iommu: Fix Coverity issues from 11.1 pull request

[PATCH 0/2] amd_iommu: Fix Coverity issues from 11.1 pull request

[Alejandro Jimenez]

Address two Coverity issues reported during scan/review of changes included in
the recent pull request:
https://lore.kernel.org/all/cover.1781463125.git.mst@redhat.com/

- Fix return types of page walk helpers so they return int type status values.
- Fully initialize a local X86IOMMUIrq structure used when building MSI messages
in the XT enabled case, avoiding later use of an uninitialized field.

A larger fix for the bitfield and host endianness correctness issue Peter
Maydell pointed out will be sent separately.

Thank you,

Alejandro

Alejandro Jimenez (2):
  amd_iommu: Return int from page walk status helpers
  amd_iommu: Fully initialize XT interrupt message state

 hw/i386/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: b83371668192a705b878e909c5ae9c1233cbd5fb
-- 
2.47.3

[PATCH 1/2] amd_iommu: Return int from page walk status helpers

[Alejandro Jimenez]

fetch_pte() returns a status code 0 on success, and (small) negative values
on failure. The PTE value itself is returned via an output parameter.
amdvi_get_top_pt_level_and_perms() follows the same return convention.

Both functions currently return uint64_t, which means any negative error
values are returned as unsigned and then converted back to int by the
callers. This does not cause any issues in the current implementation, but
Coverity flags the type mismatch and potential overflow.

Make both helpers return int, so the type matches what the return variable
is (0 on success, small negative value on failure), and also the type used
by all callers to store their return values.

No functional changes are intended.

Fixes: a1c97c395729 ("amd_iommu: Sync shadow page tables on page invalidation")
Fixes: 786550e2d38a ("amd_iommu: Follow root pointer before page walk and use 1-based levels")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 79216fb305..0d273fd33d 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -659,7 +659,7 @@ static uint64_t large_pte_page_size(uint64_t pte)
  *   - IOVA exceeds the address width supported by DTE[Mode]
  * In all such cases a page walk must be aborted.
  */
-static uint64_t amdvi_get_top_pt_level_and_perms(hwaddr address, uint64_t dte,
+static int amdvi_get_top_pt_level_and_perms(hwaddr address, uint64_t dte,
                                                  uint8_t *top_level,
                                                  IOMMUAccessFlags *dte_perms)
 {
@@ -702,7 +702,7 @@ static uint64_t amdvi_get_top_pt_level_and_perms(hwaddr address, uint64_t dte,
  *      page table walk. This means that the DTE has valid data, but one of the
  *      lower level entries in the Page Table could not be read.
  */
-static uint64_t fetch_pte(AMDVIAddressSpace *as, hwaddr address, uint64_t dte,
+static int fetch_pte(AMDVIAddressSpace *as, hwaddr address, uint64_t dte,
                           uint64_t *pte, hwaddr *page_size)
 {
     uint64_t pte_addr;
-- 
2.47.3

[PATCH 2/2] amd_iommu: Fully initialize XT interrupt message state

[Alejandro Jimenez]

When IOMMU x2APIC interrupts generation (IntCapXTEn) is enabled,
amdvi_build_xt_msi_msg() builds an MSI message using the relevant values in
the XT IOMMU General Interrupt Control Register.

Initialize the local X86IOMMUIrq structure with zero for all fields. This
ensures that X86IOMMUIrq fields not set in the XT register (e.g.
msi_addr_last_bits) are initialized before x86_iommu_irq_to_msi_message()
consumes them.

Remove the redundant 'struct' keyword in X86IOMMUIrq irq declaration.

CID: 1660056
Fixes: cf0210df65aa ("amd_iommu: Generate XT interrupts when xt support is enabled")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 0d273fd33d..9005dc7aab 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -195,7 +195,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
 static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
 {
     union mmio_xt_intr xt_reg;
-    struct X86IOMMUIrq irq;
+    X86IOMMUIrq irq = { 0 };
 
     xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
 
-- 
2.47.3

Re: [PATCH 1/2] amd_iommu: Return int from page walk status helpers

[Peter Maydell]

On Tue, 23 Jun 2026 at 01:58, Alejandro Jimenez
<alejandro.j.jimenez@oracle.com> wrote:
>
> fetch_pte() returns a status code 0 on success, and (small) negative values
> on failure. The PTE value itself is returned via an output parameter.
> amdvi_get_top_pt_level_and_perms() follows the same return convention.
>
> Both functions currently return uint64_t, which means any negative error
> values are returned as unsigned and then converted back to int by the
> callers. This does not cause any issues in the current implementation, but
> Coverity flags the type mismatch and potential overflow.
>
> Make both helpers return int, so the type matches what the return variable
> is (0 on success, small negative value on failure), and also the type used
> by all callers to store their return values.
>
> No functional changes are intended.
>
> Fixes: a1c97c395729 ("amd_iommu: Sync shadow page tables on page invalidation")
> Fixes: 786550e2d38a ("amd_iommu: Follow root pointer before page walk and use 1-based levels")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] amd_iommu: Fully initialize XT interrupt message state

[Peter Maydell]

On Tue, 23 Jun 2026 at 01:58, Alejandro Jimenez
<alejandro.j.jimenez@oracle.com> wrote:
>
> When IOMMU x2APIC interrupts generation (IntCapXTEn) is enabled,
> amdvi_build_xt_msi_msg() builds an MSI message using the relevant values in
> the XT IOMMU General Interrupt Control Register.
>
> Initialize the local X86IOMMUIrq structure with zero for all fields. This
> ensures that X86IOMMUIrq fields not set in the XT register (e.g.
> msi_addr_last_bits) are initialized before x86_iommu_irq_to_msi_message()
> consumes them.
>
> Remove the redundant 'struct' keyword in X86IOMMUIrq irq declaration.
>
> CID: 1660056
> Fixes: cf0210df65aa ("amd_iommu: Generate XT interrupts when xt support is enabled")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  hw/i386/amd_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 0d273fd33d..9005dc7aab 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -195,7 +195,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>  static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
>  {
>      union mmio_xt_intr xt_reg;
> -    struct X86IOMMUIrq irq;
> +    X86IOMMUIrq irq = { 0 };

This is fine for the Coverity problem, but you do also need to
get rid of the bitfield-union as a separate issue.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/2] amd_iommu: Fully initialize XT interrupt message state

[Michael S. Tsirkin]

On Tue, Jun 23, 2026 at 09:44:10AM +0100, Peter Maydell wrote:
> On Tue, 23 Jun 2026 at 01:58, Alejandro Jimenez
> <alejandro.j.jimenez@oracle.com> wrote:
> >
> > When IOMMU x2APIC interrupts generation (IntCapXTEn) is enabled,
> > amdvi_build_xt_msi_msg() builds an MSI message using the relevant values in
> > the XT IOMMU General Interrupt Control Register.
> >
> > Initialize the local X86IOMMUIrq structure with zero for all fields. This
> > ensures that X86IOMMUIrq fields not set in the XT register (e.g.
> > msi_addr_last_bits) are initialized before x86_iommu_irq_to_msi_message()
> > consumes them.
> >
> > Remove the redundant 'struct' keyword in X86IOMMUIrq irq declaration.
> >
> > CID: 1660056
> > Fixes: cf0210df65aa ("amd_iommu: Generate XT interrupts when xt support is enabled")
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> > ---
> >  hw/i386/amd_iommu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> > index 0d273fd33d..9005dc7aab 100644
> > --- a/hw/i386/amd_iommu.c
> > +++ b/hw/i386/amd_iommu.c
> > @@ -195,7 +195,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
> >  static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
> >  {
> >      union mmio_xt_intr xt_reg;
> > -    struct X86IOMMUIrq irq;
> > +    X86IOMMUIrq irq = { 0 };

Why don't we just initialize everything at the declaration site?

union mmio_xt_intr xt_reg = { .val = ... };
X86IOMMUIrq irq = { .... };

> This is fine for the Coverity problem, but you do also need to
> get rid of the bitfield-union as a separate issue.

Ideal task for AI actually.

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM

Re: [PATCH 2/2] amd_iommu: Fully initialize XT interrupt message state

[Alejandro Jimenez]

On 6/23/26 2:37 PM, Michael S. Tsirkin wrote:
> On Tue, Jun 23, 2026 at 09:44:10AM +0100, Peter Maydell wrote:
>> On Tue, 23 Jun 2026 at 01:58, Alejandro Jimenez
>> <alejandro.j.jimenez@oracle.com> wrote:
>>>
>>> When IOMMU x2APIC interrupts generation (IntCapXTEn) is enabled,
>>> amdvi_build_xt_msi_msg() builds an MSI message using the relevant values in
>>> the XT IOMMU General Interrupt Control Register.
>>>
>>> Initialize the local X86IOMMUIrq structure with zero for all fields. This
>>> ensures that X86IOMMUIrq fields not set in the XT register (e.g.
>>> msi_addr_last_bits) are initialized before x86_iommu_irq_to_msi_message()
>>> consumes them.
>>>
>>> Remove the redundant 'struct' keyword in X86IOMMUIrq irq declaration.
>>>
>>> CID: 1660056
>>> Fixes: cf0210df65aa ("amd_iommu: Generate XT interrupts when xt support is enabled")
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>> ---
>>>  hw/i386/amd_iommu.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index 0d273fd33d..9005dc7aab 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -195,7 +195,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>>>  static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
>>>  {
>>>      union mmio_xt_intr xt_reg;
>>> -    struct X86IOMMUIrq irq;
>>> +    X86IOMMUIrq irq = { 0 };
> 
> Why don't we just initialize everything at the declaration site?
> 
> union mmio_xt_intr xt_reg = { .val = ... };
> X86IOMMUIrq irq = { .... };
> 

The above makes sense, but this commit is essentially moot because my
follow up getting rid of the bitfield usage overrides the whole thing. i.e.
an upcoming change in v2 will do:

 static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
 {
-    union mmio_xt_intr xt_reg;
-    X86IOMMUIrq irq = { 0 };
+    uint64_t xt_reg = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);

-    xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
-
-    irq.vector = xt_reg.vector;
-    irq.delivery_mode = xt_reg.delivery_mode;
-    irq.dest_mode = xt_reg.destination_mode;
-    irq.dest = (xt_reg.destination_hi << 24) | xt_reg.destination_lo;
-    irq.trigger_mode = 0;
-    irq.redir_hint = 0;
+    X86IOMMUIrq irq = {
+        .vector = FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, VECTOR),
+        .delivery_mode = FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DELIVERY_MODE),
+        .dest_mode = FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DEST_MODE),
+        .dest = (FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DEST_HI) << 24) |
+                 FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DEST_LO),
+        .trigger_mode = 0,
+        .redir_hint = 0,
+    };

which is in line with your request.

The one reason I would think to keep this current commit is that it is
intended to close the Coverity issue (CID: 1660056). But I can always just
add that trailer to the new patch that does all of the bitfield removal
work, it would just be "less focused".

I am ready to send a v2 for this series that keeps the current two patches
and adds 2 more removing all of the bitfield usage. Or I can just drop this
current patch since it is folded into one of the new ones, and put the CID
trailer on it.

Please let me know what is the preferred way to handle it.

>> This is fine for the Coverity problem, but you do also need to
>> get rid of the bitfield-union as a separate issue.
> 
> Ideal task for AI actually.
> 

Yeah, it is pretty mechanical, but IIUC we are still not officially allowed
to use it :).

Alejandro

>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> thanks
>> -- PMM
> 
>