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

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

[David Hoppenbrouwers]

The command ID is in cmd[0], not cmd[1].

Signed-off-by: David Hoppenbrouwers <qemu@demindiro.com>
---
 hw/i386/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 789e09d6f2..2570a4701c 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1456,9 +1456,9 @@ static void amdvi_cmdbuf_exec(AMDVIState *s)
         amdvi_inval_all(s, cmd);
         break;
     default:
-        trace_amdvi_unhandled_command(extract64(cmd[1], 60, 4));
+        trace_amdvi_unhandled_command(extract64(cmd[0], 60, 4));
         /* log illegal command */
-        amdvi_log_illegalcom_error(s, extract64(cmd[1], 60, 4),
+        amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
                                    s->cmdbuf + s->cmdbuf_head);
     }
 }
-- 
2.52.0

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

[Igor Mammedov]

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.

> Signed-off-by: David Hoppenbrouwers <qemu@demindiro.com>
> ---
>  hw/i386/amd_iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2..2570a4701c 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1456,9 +1456,9 @@ static void amdvi_cmdbuf_exec(AMDVIState *s)
>          amdvi_inval_all(s, cmd);
>          break;
>      default:
> -        trace_amdvi_unhandled_command(extract64(cmd[1], 60, 4));
> +        trace_amdvi_unhandled_command(extract64(cmd[0], 60, 4));
>          /* log illegal command */
> -        amdvi_log_illegalcom_error(s, extract64(cmd[1], 60, 4),
> +        amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
>                                     s->cmdbuf + s->cmdbuf_head);
>      }
>  }

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

[Sairaj Kodilkar]

On 2/20/2026 11:44 PM, David Hoppenbrouwers wrote:
> [You don't often get email from qemu@demindiro.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> The command ID is in cmd[0], not cmd[1].
>
> Signed-off-by: David Hoppenbrouwers <qemu@demindiro.com>
> ---
>   hw/i386/amd_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2..2570a4701c 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1456,9 +1456,9 @@ static void amdvi_cmdbuf_exec(AMDVIState *s)
>           amdvi_inval_all(s, cmd);
>           break;
>       default:
> -        trace_amdvi_unhandled_command(extract64(cmd[1], 60, 4));
> +        trace_amdvi_unhandled_command(extract64(cmd[0], 60, 4));
>           /* log illegal command */
> -        amdvi_log_illegalcom_error(s, extract64(cmd[1], 60, 4),
> +        amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
>                                      s->cmdbuf + s->cmdbuf_head);
>       }
>   }
> --
> 2.52.0
Reviewed-by: Sairaj Kodilkar <sarunkod@amd.com>

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

[David Hoppenbrouwers]

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

David

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

[Alejandro Jimenez]

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>

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

Thank you,
Alejandro

> David

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

[Igor Mammedov]

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  
> 

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

[David Hoppenbrouwers]

On 2/23/26 7:49 PM, Alejandro Jimenez 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>
> 
> If that works for everyone, I can add apply it; no need to send a new revision.
> 
> Thank you,
> Alejandro
> 
>> David

It is fine with me.