Re: [PATCH v6 02/24] iommu/amd: Introduce EVENT_TYPE_GUEST_EVENT_FAULT
From: Suthikulpanit, Suravee
Date: Thu Oct 22 2026 - 01:32:41 EST
Vasant,
On 10/19/2026 1:10 PM, Vasant Hegde wrote:
On 9/30/2026 2:53 PM, Suravee Suthikulpanit wrote:
INSERT_GUEST_EVENT with non-zero reserved bits logs
GUEST_EVENT_FAULT followed by the original guest event.
Consume both event-log slots, wrapping via
evt_buf+(head+EVTLOG_ENTRY_SIZE)%size, and dump the guest
payload raw so it is not decoded as a host event.
Wait until occupancy is at least two entries, not merely
until live tail is not the pair offset. Re-read the
event-log tail each poll iteration so a return of 2 cannot
advance head past a stale cached tail and walk empty slots
with the type-0 erratum retry.
Retry a type-0 guest slot up to LOOP_TIMEOUT, the same way
erratum 732 waits for the primary slot. Return 2 only when
that slot is already in [head, live_tail). Return 1 if the
pair never appears in tail. Rate-limit the guest-triggered
logs.
Log an invalid guest-event when the pair slot is still
missing or type 0 after that wait, instead of stalling
the event log.
As per spec:
GUEST_EVENT_FAULT event is always written in pairs with a guest event entry
immediately after it into the event log.
Why not add one additional check in iommu_poll_events() such that:> if event at head is GUEST_EVENT_FAULT and actual fault event is
not-yet-written by HW just return.
If subsequent event is also written then handle the event.
and increase head pointer accordingly.
I would like to keep the code for handling GUEST_EVENT_FAULT (including the logic for checking guest event pair entry) in iommu_print_event() to simplify the code readability.
Originally, I try to handle the case where the guest event pair may arrive late, but that adds complexity to the tail polling logic. For v7, I can simplify the logic to just report GUEST_EVENT_FAULT event with missing guest eventinstead then.
....
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 43a5690721af..2df81e29519e 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -942,8 +942,18 @@ static void amd_iommu_report_ppr_err(struct amd_iommu *iommu, volatile u32 *even
pci_dev_put(pdev);
}
-static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
+static u32 evtlog_used(u32 head, u32 tail)
{
+ return (tail + amd_iommu_evtlog_size - head) % amd_iommu_evtlog_size;
+}
+
+/*
+ * Decode one event-log slot (or a GUEST_EVENT_FAULT pair).
+ * Returns the number of slots consumed.
+ */
+static unsigned int iommu_print_event(struct amd_iommu *iommu, u32 head)
+{
+ void *__evt = iommu->evt_buf + head;
struct device *dev = iommu->iommu.dev;
int type, devid, flags;
volatile u32 *event = __evt;
@@ -965,7 +975,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
/* Did we hit the erratum? */
if (++count == LOOP_TIMEOUT) {
pr_err("No event written to event log\n");
- return;
+ return 1;
This will cause head to progress event though there is no event.
Actually, there is an event (since the head != tail) but with (type == 0 : invalid event). So, we need to discard the entry and increment head.
}
udelay(1);
goto retry;
@@ -975,7 +985,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
if (type == EVENT_TYPE_IO_FAULT) {
amd_iommu_report_page_fault(iommu, devid, pasid, address, flags, vflags);
- return;
+ return 1;
}
switch (type) {
@@ -1023,9 +1033,90 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
case EVENT_TYPE_INV_PPR_REQ:
amd_iommu_report_ppr_err(iommu, event, devid, address, flags);
break;
+ case EVENT_TYPE_GUEST_EVENT_FAULT:
+ {
+ u16 gid = FIELD_GET(EVENT_GID_MASK, event[1]);
+ u32 next = (head + EVTLOG_ENTRY_SIZE) % amd_iommu_evtlog_size;
+ u32 *guest;
+ u32 live_tail, w0, w1, w2, w3;
+ int guest_type, wait;
+
+ dev_err_ratelimited(dev, "Event logged [GUEST_EVENT_FAULT gid=%#x flags=0x%04x vflags=%#x]\n",
+ gid, flags, vflags);
+
+ /*
+ * GUEST_EVENT_FAULT is paired with the original guest
+ * event in the next log slot (possibly wrapped). The
+ * poll-time tail can predate that slot. Wait until
+ * occupancy is at least two entries, then retry a
+ * type-0 pair slot the same way erratum 732 waits
+ * for DMA visibility. Return 2 only when that slot
+ * is already in [head, live_tail). iommu_poll_events()
+ * re-reads tail each iteration so head cannot skip
+ * past tail.
+ */
+ wait = 0;
+ for (;;) {
+ live_tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
+ if (evtlog_used(head, live_tail) >=
+ 2 * EVTLOG_ENTRY_SIZE)
+ break;
+ if (++wait == LOOP_TIMEOUT) {
I think we should avoid avoid this loop in interrupt handler path.
I am removing this loop.
+ dev_err_ratelimited(dev, "Event logged [GUEST_EVENT_FAULT invalid guest-event (does not exist)]\n");
+ if (!amd_iommu_snp_en)
+ memset(__evt, 0, EVTLOG_ENTRY_SIZE);
+ return 1;
+ }
+ udelay(1);
+ }
+
+ guest = (u32 *)(iommu->evt_buf + next);
+ wait = 0;
+ for (;;) {
+ w1 = READ_ONCE(guest[1]);
+ guest_type = (w1 >> EVENT_TYPE_SHIFT) & EVENT_TYPE_MASK;
+ if (guest_type)
+ break;
+ if (++wait == LOOP_TIMEOUT) {
+ w0 = READ_ONCE(guest[0]);
+ w2 = READ_ONCE(guest[2]);
+ w3 = READ_ONCE(guest[3]);
+ dev_err_ratelimited(dev, "Event logged [GUEST_EVENT_FAULT invalid guest-event %08x %08x %08x %08x]\n",
+ w0, w1, w2, w3);
+ if (!amd_iommu_snp_en) {
+ memset(__evt, 0, EVTLOG_ENTRY_SIZE);
+ memset(guest, 0, EVTLOG_ENTRY_SIZE);
+ }
+ /* Occupancy already includes this slot.
+ * Skip it so a late DMA fill is not
+ * decoded as a host event.
+ */
+ return 2;
+ }
+ udelay(1);
+ }
+
+ w0 = READ_ONCE(guest[0]);
+ w2 = READ_ONCE(guest[2]);
+ w3 = READ_ONCE(guest[3]);
+ dev_err_ratelimited(dev, "Event logged [GUEST_EVENT_FAULT guest-event %08x %08x %08x %08x]\n",
+ w0, w1, w2, w3);
+
+ /*
+ * Erratum 732: clear both pair slots so a stale guest
+ * event is not decoded on the next poll. The second
+ * slot may wrap to the start of the ring. Skip on SNP;
+ * that buffer is not writable.
+ */
+ if (!amd_iommu_snp_en) {
+ memset(__evt, 0, EVTLOG_ENTRY_SIZE);
+ memset(guest, 0, EVTLOG_ENTRY_SIZE);
+ }
+ return 2;
+ }
case EVENT_TYPE_VIOMMU_HW_ERR:
{
- u16 gid = event[0] & 0xFFFF;
+ u16 gid = FIELD_GET(EVENT_GID_MASK, event[0]);
u8 src = (event[0] >> 16) & 0x3;
dev_err(dev, "Event logged [VIOMMU_HW_ERR gid=%#x address=%#llx src=%#x flags=0x%04x vflags=%#x]\n",
@@ -1044,24 +1135,27 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
* SNP enabled system.
*/
if (!amd_iommu_snp_en)
- memset(__evt, 0, 4 * sizeof(u32));
+ memset(__evt, 0, EVTLOG_ENTRY_SIZE);
+
+ return 1;
}
static void iommu_poll_events(struct amd_iommu *iommu)
{
u32 head, tail;
+ unsigned int n;
head = readl(iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
- tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
-
- while (head != tail) {
- iommu_print_event(iommu, iommu->evt_buf + head);
+ for (;;) {
+ tail = readl(iommu->mmio_base + MMIO_EVT_TAIL_OFFSET);
We should read tail outside the loop. Otherwise we may stuck in interrupt
handler context for long time and cause softlockups.
I am simplifying this logic.
Thanks,
Suravee