Re: [PATCH v6 02/24] iommu/amd: Introduce EVENT_TYPE_GUEST_EVENT_FAULT

[Vasant Hegde]

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.



> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
>  drivers/iommu/amd/amd_iommu_types.h |   2 +
>  drivers/iommu/amd/iommu.c           | 116 +++++++++++++++++++++++++---
>  2 files changed, 107 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index f26a9059a356..704b4c8acdc5 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -138,6 +138,7 @@
>  #define EVENT_TYPE_IOTLB_INV_TO	0x7
>  #define EVENT_TYPE_INV_DEV_REQ	0x8
>  #define EVENT_TYPE_INV_PPR_REQ	0x9
> +#define EVENT_TYPE_GUEST_EVENT_FAULT	0xb
>  #define EVENT_TYPE_VIOMMU_HW_ERR	0xc
>  
>  #define EVENT_TYPE_RMP_FAULT	0xd
> @@ -255,6 +256,7 @@
>  
>  /* Constants for vIOMMU event fields */
>  #define EVENT_VFLAGS_MASK		GENMASK_ULL(31, 27)
> +#define EVENT_GID_MASK			GENMASK(15, 0)
>  
>  /* Constants for PPR Log handling */
>  #define PPRLOG_ENTRY_SIZE	0x10
> 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.

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

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


-Vasant

> +		if (head == tail)
> +			break;
> +		n = iommu_print_event(iommu, head);
>  
>  		/* Update head pointer of hardware ring-buffer */
> -		head = (head + EVTLOG_ENTRY_SIZE) % amd_iommu_evtlog_size;
> +		head = (head + n * EVTLOG_ENTRY_SIZE) % amd_iommu_evtlog_size;
>  		writel(head, iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
>  	}
> -
>  }
>  
>  #ifdef CONFIG_IRQ_REMAP