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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

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