On 5/13/2026 8:11 PM, Alejandro Jimenez wrote:
On 5/13/26 12:38 AM, Sairaj Kodilkar wrote:
On 5/12/2026 8:30 PM, Alejandro Jimenez wrote:
When processing a command, amdvi_cmdbuf_run() increments cmdbuf_head and
writes it to the emulated MMIO register space before checking whether it
has reached the end of the command buffer.
If the incremented value reaches the end of the buffer and the tail pointer
is zero, the loop exits and the COMMAND_HEAD offset still contains an
unwrapped value. There are no errors in command processing since internal
cmdbuf_head state is always correctly updated, but the spec defines the
CmdHeadPtr field in MMIO Offset 2000h Command Buffer Head Pointer Register
as RW i.e. guest-visible, so it should be kept consistent.
Wrap cmdbuf_head before updating COMMAND_HEAD so the MMIO-visible register
always matches the internal command buffer head pointer position.
Cc: [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 197e452e3c..5d6a405263 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1475,12 +1475,12 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
trace_amdvi_command_exec(s->cmdbuf_head, s->cmdbuf_tail, s-
cmdbuf);
amdvi_cmdbuf_exec(s);
s->cmdbuf_head += AMDVI_COMMAND_SIZE;
- amdvi_writeq_raw(s, AMDVI_MMIO_COMMAND_HEAD, s->cmdbuf_head);
/* wrap head pointer */
if (s->cmdbuf_head >= s->cmdbuf_len * AMDVI_COMMAND_SIZE) {
s->cmdbuf_head = 0;
}
+ amdvi_writeq_raw(s, AMDVI_MMIO_COMMAND_HEAD, s->cmdbuf_head);
}
}
Hi Alejandro,
Maybe we can clean it a little bit by using modulo operation
s->cmdbuf_head = (s->cmdbuf_head + AMDVI_COMMAND_SIZE) % s->cmdbuf_len *
AMDVI_COMMAND_SIZE;
In short, I find the "if" code easier to understand.
It keeps it readable and avoids a number of potential issues. e.g. the
approach above using modulo is missing important parenthesis around the
second block:
s->cmdbuf_head = (s->cmdbuf_head + AMDVI_COMMAND_SIZE) %
(s->cmdbuf_len * AMDVI_COMMAND_SIZE);
even with that corrected, it could still break if changes in other code end
up setting invalid values in cmdbuf_head and/or cmdbuf_len e.g. if
cmdbuf_len is incorrectly set to 0 for some reason, then the modulo
operation has 0 as an operand which is undefined IIUC.
We could also use masking like Paolo's recent patch that inspired this change:
s->cmdbuf_head = (s->cmdbuf_head + AMDVI_COMMAND_SIZE) &
(s->cmdbuf_len * AMDVI_COMMAND_SIZE - 1);
but I think similar issues as with the modulo apply.
Out of curiosity I generated assembly for the two cases and for the "if" it
uses:
cmpq, cmovnb
while for the modulo code it uses:
divq
So I think division might actually be slower than the compare and
conditional move.
Alejandro
This will eliminate the if condition. Thanks Sairaj
Hi Alejandro,
Thanks for explaining it in details.
Reviewed-by: Sairaj Kodilkar <[email protected]>
base-commit: 8333dba7326d89ce180221c4d0c6d34826a9a4a3
prerequisite-patch-id: df656299f90e928acad39d99e3701c5641e972d5
