On Tue, Jun 23, 2026 at 05:57:39PM -0400, Alejandro Jimenez wrote: > > > 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 > >> <[email protected]> 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 <[email protected]> > >>> Suggested-by: Peter Maydell <[email protected]> > >>> Signed-off-by: Alejandro Jimenez <[email protected]> > >>> --- > >>> 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.
Doesn't matter to me :) > >> 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 <[email protected]> > >> > >> thanks > >> -- PMM > > > >
