On 6/22/26 4:00 PM, Michael S. Tsirkin wrote:
> On Mon, Jun 22, 2026 at 04:19:50PM +0100, Peter Maydell wrote:
>> On Sun, 14 Jun 2026 at 20:09, Michael S. Tsirkin <[email protected]> wrote:
>>>
>>> From: Sairaj Kodilkar <[email protected]>
>>>
>>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself
>>> are
>>> sent based on the programming in XT IOMMU Interrupt Control Registers in
>>> MMIO
>>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability
>>> registers.
>>> The guest programs these registers with appropriate vector and destination
>>> ID instead of writing to PCI MSI capability.
>>>
>>> Current AMD vIOMMU is capable of generating interrupts only through PCI
>>> MSI capability and does not care about xt mode. Because of this AMD
>>> vIOMMU cannot generate event log interrupts when the guest has enabled
>>> xt mode.
>>>
>>> Introduce a new flag "intcapxten" which is set when guest writes control
>>> register [IntCapXTEn] (bit 51) and use vector and destination field in
>>> the XT MMIO register (0x170) to support XT mode.
>>
>> Hi; Coverity points out a use of an uninitialized struct field here
>> (CID 1660056):
>>
>>> +static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
>>> +{
>>> + union mmio_xt_intr xt_reg;
>>> + struct X86IOMMUIrq irq;
>>
>> Here we declare irq, without a struct initializer...
>>
>>> +
>>> + 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;
>>
>> ...and here we fill in some but not all of its fields...
>>
>>> +
>>> + x86_iommu_irq_to_msi_message(&irq, msg);
>>
>> ...and here we call a function which (among other things) does this:
>> msg.__not_used = irq->msi_addr_last_bits;
>>
>> which will read the uninitialized msi_addr_last_bits field.
>>
>> Bonus extra bug report:
>>
>> +union mmio_xt_intr {
>> + uint64_t val;
>> + struct {
>> + uint64_t rsvd_1:2,
>> + destination_mode:1,
>> + rsvd_2:5,
>> + destination_lo:24,
>> + vector:8,
>> + delivery_mode:1,
>> + rsvd_3:15,
>> + destination_hi:8;
>> + };
>> +};
>>
>> Please don't use bitfields like this -- this is not portable to big
>> endian hosts. Use the extract64() function instead, or the FIELD macros.
>>
>> I suggest something like (untested):
>>
>> (at top level in a source file or header; the numbers are
>> start-bit and length, check these as I may have made typos)
>>
>> FIELD(XT_GEN_INTR, DEST_MODE, 2, 1)
>> FIELD(XT_GEN_INTR, DEST_LO, 24, 8)
>> FIELD(XT_GEN_INTR, VECTOR, 32, 8)
>> FIELD(XT_GEN_INTR, DELIVERY_MODE, 33, 1)
>> FIELD(XT_GEN_INTR, DEST_HI, 49, 8)
>>
>> and then in the function you can do:
>>
>> uint64_t xtreg = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
>> X86IOMMUIrq irq = {
>> .vector = FIELD_EX64(xtreg, XT_GEN_INTR, VECTOR),
>> .delivery_mode = FIELD_EX64(xtreg, XT_GEN_INTR, DELIVERY_MODE),
>> .dest_mode = FIELD_EX64(xtreg, XT_GEN_INTR, DEST_MODE),
>> .dest = (FIELD_X64(xtreg, XT_GEN_INTR, DEST_HI) << 24) |
>> FIELD_EX64(xtreg, XT_GEN_INTR, DEST_LO),
>> };
>>
>> which gives you a struct initializer and you can rely on the
>> whole struct being initialized.
>>
>>> +}
>>
>> thanks
>> -- PMM
>
> Indeed. Alejandro should we revert or fix up?
>
I think the uninitialized struct issue can be quickly fixed using a
designated initializer, so that would directly address CID 1660056.
For the bitfield portability in big endian host problem, the changes are a
bit more involved since there are existing definitions using the same
pattern (dating back to 2018 when IR support was introduced). So to do it
right it involves more changes than just the code touched by the current patch.
It should be relatively straightforward though, so I will work on it and
will send a patch extended Peter's suggestion to all the cases. If run into
issues/time constraints, then by EOW I'll send a simple fix with the
designated initialized approach to fix the coverity report, while I
continue on the "bonus" case.
Does that work?
Alejandro
