Re: [PATCH] hw/i386/amd_iommu.c: fix amdvi_unhandled_command logging cmd ID

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 23 Feb 2026 13:49:45 -0500
Alejandro Jimenez <alejandro.j.jimenez@oracle.com> wrote:

> Hi David
> 
> On 2/23/26 8:06 AM, David Hoppenbrouwers wrote:
> > On 2/23/26 8:32 AM, Igor Mammedov wrote:  
> >> On Fri, 20 Feb 2026 19:14:37 +0100
> >> David Hoppenbrouwers <qemu@demindiro.com> wrote:
> >>  
> >>> The command ID is in cmd[0], not cmd[1].  
> >> pls add a reference to the spec,
> >> and if any what impact it does have on a guest/how it manifests.  
> > 
> > Figure 43 "Generic Command Buffer Entry Format" in
> > https://docs.amd.com/v/u/en-US/48882_3.10_PUB  
> 
> Unfortunately, the documentation links tend to be broken very often, so it
> is not a good idea to include them in commit messages. That is why I would
> opt for the longer, more verbose (but hopefully easier to find after a
> quick search) choice of listing the name of the spec with the revision,
> chapter and relevant keywords. See my proposed commit message at the end.
> 
> > But the switch() above also uses cmd[0]:
> > 
> >     switch (extract64(cmd[0], 60, 4)) {
> >     case AMDVI_CMD_COMPLETION_WAIT:
> >         amdvi_completion_wait(s, cmd);
> >         break;
> > 
> > It has no effect on the guest, only on the -trace option. It may be
> > confusing to developers if the actual command ID doesn't match what is
> > shown by -trace.
> >   
> I think it could have an effect on (some) guest too, although this is
> unlikely given the number of things that need to go wrong, but perhaps a
> test flow in a guest driver like:
> - Guest triggers an IOMMU command with an invalid opcode.
> - Guest reads the event log (incorrectly filled by the vIOMMU) and reports
> an error on mismatched opcode.
> 
> Using cmd[1] seems clearly a typo from the original code. To honor Igor's
> request to add details, I'd propose we change the commit message to:
> 
> amd_iommu: Fix opcode reported in invalid command handling
> 
> According to the AMD I/O Virtualization Technology (IOMMU) Specification
> (Rev 3.10), Section 2.4 Commands, the Generic Command Buffer Entry Format
> encodes the opcode in bits [63:60] of the command buffer.
> 
> When handling illegal opcodes, the traces for unhandled commands and event
> log info extract the opcode from an incorrect offset in the command buffer.
> Fix this issue to avoid potential confusion with mismatched opcodes in
> traces and unlikely errors in guest event processing.
> 
> Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
> Signed-off-by: David Hoppenbrouwers <qemu@demindiro.com>
> Reviewed-by: Sairaj Kodilkar <sarunkod@amd.com>

with this
Acked-by: Igor Mammedov <imammedo@redhat.com>

> 
> If that works for everyone, I can add apply it; no need to send a new revision.
> 
> Thank you,
> Alejandro
> 
> > David  
>