On 7/16/2025 8:20 PM, Ethan MILON wrote:
On 7/16/25 09:31, Sairaj Kodilkar wrote:
Current event logging code is broken, because of following issues
1. The code uses '|' instead of '&' to test the bit field, which causes
vIOMMU to generate overflow interrupt for every log entry.
2. Code does not update the eventlog tail MMIO register after adding an
entry to the buffer, because of which guest cannot process new
entries (as head == tail means buffer is empty).
3. Compares eventlog tail (which is byte offset in the buffer) to
eventlog length (which is number of maximum entries in the buffer).
This causes vIOMMU to generate only fix number of event logs, after
which it keeps on generating overflow interrupts, without
actually resetting the log buffer.
4. Updates ComWaitInt instead of EventLogInt bitfield in Status
register. Guest checks this field to see if there are new event log
entries in the buffer.
You missed one issue, the head and tail should be reset when updating
the base pointer.
@@ -707,6 +711,10 @@ static inline void amdvi_handle_evtbase_write(AMDVIState
*s)
s->evtlog = val & AMDVI_MMIO_EVTLOG_BASE_MASK;
s->evtlog_len = 1UL << (amdvi_readq(s, AMDVI_MMIO_EVTLOG_SIZE_BYTE)
& AMDVI_MMIO_EVTLOG_SIZE_MASK);
+ /* clear tail and head pointer to 0 when event base is updated */
+ s->evtlog_tail = s->evtlog_head = 0;
+ amdvi_writeq_raw(s, AMDVI_MMIO_EVENT_TAIL, s->evtlog_tail);
+ amdvi_writeq_raw(s, AMDVI_MMIO_EVENT_HEAD, s->evtlog_head);
}
Hi Ethan
Thanks for pointing this out. Will update it !
static inline void amdvi_handle_evttail_write(AMDVIState *s)
Moreover in the spec at 2.5.1 Event Log Restart Procedure, it is
written "The IOMMU event logging is disabled after system reset
and when the event log overflows."
Should we implement this behavior or the overflow flag is enough ?
I think overflow flag is enough, Because code discards the new entries
when the buffer overflows (which is equivalent to event logging being
disabled).
Fix above issues, so that guest can process event log entries.
Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Sairaj Kodilkar <sarun...@amd.com>
Reviewed-by: Vasant Hegde <vasant.he...@amd.com>
---
hw/i386/amd_iommu.c | 20 ++++++++++++++++----
hw/i386/amd_iommu.h | 1 +
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index e0f4220b8f25..a34062153194 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -172,7 +172,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
uint64_t val)
/* OR a 64-bit register with a 64-bit value */
s/OR/AND
Ack.
static bool amdvi_test_mask(AMDVIState *s, hwaddr addr, uint64_t val)
{
- return amdvi_readq(s, addr) | val;
+ return amdvi_readq(s, addr) & val;
}
/* OR a 64-bit register with a 64-bit value storing result in the register */
@@ -201,16 +201,26 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
}
}
+static uint32_t get_next_eventlog_entry(AMDVIState *s)
+{
+ uint32_t evtlog_size = s->evtlog_len * AMDVI_EVENT_LEN;
+ return (s->evtlog_tail + AMDVI_EVENT_LEN) % evtlog_size;
+}
+
static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
{
+ uint32_t evtlog_tail_next;
+
/* event logging not enabled */
if (!s->evtlog_enabled || amdvi_test_mask(s, AMDVI_MMIO_STATUS,
AMDVI_MMIO_STATUS_EVT_OVF)) {
return;
}
+ evtlog_tail_next = get_next_eventlog_entry(s);
+
/* event log buffer full */
- if (s->evtlog_tail >= s->evtlog_len) {
+ if (evtlog_tail_next == s->evtlog_head) {
amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVT_OVF);
/* generate interrupt */
amdvi_generate_msi_interrupt(s);
@@ -222,8 +232,10 @@ static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
trace_amdvi_evntlog_fail(s->evtlog, s->evtlog_tail);
}
- s->evtlog_tail += AMDVI_EVENT_LEN;
- amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_COMP_INT);
+ s->evtlog_tail = evtlog_tail_next;
+ amdvi_writeq(s, AMDVI_MMIO_EVENT_TAIL, s->evtlog_tail);
+
+ amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVENT_INT);
amdvi_generate_msi_interrupt(s);
We should take into account the s->evtlog_intr flag before generating
the interrupt.
And I think we could refactor amdvi_assign_orq inside
amdvi_generate_msi_interrupt, so we could do:
amdvi_generate_msi_interrupt(s, AMDVI_MMIO_STATUS_EVENT_INT); for
example.
yes we should do definitely do that ! Thanks for pointing it out.
Also I don't want to change 'amdvi_generate_msi_interrupt()' as it is
being used for few other things in future cleanup patches.
But what I can do it add a check in `amdvi_log_event()`.
Thanks
Sairaj
Thanks,
Ethan
}
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 62641b779ca3..3dd4e7e3e8b8 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -111,6 +111,7 @@
#define AMDVI_MMIO_STATUS_CMDBUF_RUN (1 << 4)
#define AMDVI_MMIO_STATUS_EVT_RUN (1 << 3)
#define AMDVI_MMIO_STATUS_COMP_INT (1 << 2)
+#define AMDVI_MMIO_STATUS_EVENT_INT (1 << 1)
#define AMDVI_MMIO_STATUS_EVT_OVF (1 << 0)
#define AMDVI_CMDBUF_ID_BYTE 0x07
--
2.34.1
