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

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

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
> 
>