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