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
>> base-commit: 8333dba7326d89ce180221c4d0c6d34826a9a4a3
>> prerequisite-patch-id: df656299f90e928acad39d99e3701c5641e972d5
>
