Re: [PATCH 02/24] iommu/amd: Introduce vIOMMU-specific events and event

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

On 8/10/2026 2:12 PM, Vasant Hegde wrote:
> Hi Suravee,
> 
> 
> On 7/27/2026 6:58 PM, Suravee Suthikulpanit wrote:
>> Adding support for new vIOMMU events:
>>    * Guest Event Fault event
>>    * vIOMMU Hardware Error event
>>
>> Also, adding support for the additional vIOMMU related flags
>> in existing events.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   drivers/iommu/amd/amd_iommu_types.h |  7 ++++
>>   drivers/iommu/amd/iommu.c           | 58 ++++++++++++++++++++++-------
>>   2 files changed, 52 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index d2c64e2e9f05..4df6a50128de 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -149,6 +149,9 @@
>>   #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
> 
> Better `EVENT_TYPE_GUEST_FAULT` ?

I want to keep this consistent with the IOMMU spec "GUEST_EVENT_FAULT" 
event.

> 
>> +#define EVENT_TYPE_VIOMMU_HW_ERR	0xc
>> +
>>   #define EVENT_TYPE_RMP_FAULT	0xd
>>   #define EVENT_TYPE_RMP_HW_ERR	0xe
>>   #define EVENT_DEVID_MASK	0xffff
>> @@ -261,6 +264,10 @@
>>   #define EVTLOG_SIZE_MAX		SZ_512K /* 32K entries */
>>   #define EVTLOG_LEN_MASK_MAX	(0xFULL << EVTLOG_SIZE_SHIFT)
>>   
>> +/* Constants for IO_PAGE_FAULT event */
>> +#define IO_PAGE_FAULT_VFLAGS_SHIFT	27
>> +#define IO_PAGE_FAULT_VFLAGS_MASK	GENMASK_ULL(27, 23)
> 
> You mean  (31, 27) ?

I'll fix the mask and remove the shift since it's no longer needed.

> 
>> +
>>   /* Constants for PPR Log handling */
>>   #define PPRLOG_ENTRY_SIZE	0x10
>>   #define PPRLOG_SIZE_SHIFT	56
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 4b4dd20ebec6..50f26c8123f3 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -854,7 +854,7 @@ static void amd_iommu_report_rmp_fault(struct amd_iommu *iommu, volatile u32 *ev
>>   
>>   static void amd_iommu_report_page_fault(struct amd_iommu *iommu,
>>   					u16 devid, u16 domain_id,
>> -					u64 address, int flags)
>> +					u64 address, int flags, u8 vflags)
>>   {
>>   	struct iommu_dev_data *dev_data = NULL;
>>   	struct pci_dev *pdev;
>> @@ -889,13 +889,13 @@ static void amd_iommu_report_page_fault(struct amd_iommu *iommu,
>>   		}
>>   
>>   		if (__ratelimit(&dev_data->rs)) {
>> -			pci_err(pdev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%llx flags=0x%04x]\n",
>> -				domain_id, address, flags);
>> +			pci_err(pdev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%llx flags=0x%04x vflags=%#x]\n",
>> +				domain_id, address, flags, vflags);
>>   		}
>>   	} else {
>> -		pr_err_ratelimited("Event logged [IO_PAGE_FAULT device=%04x:%02x:%02x.%x domain=0x%04x address=0x%llx flags=0x%04x]\n",
>> +		pr_err_ratelimited("Event logged [IO_PAGE_FAULT device=%04x:%02x:%02x.%x domain=0x%04x address=0x%llx flags=0x%04x vflags=%#x]\n",
>>   			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>> -			domain_id, address, flags);
>> +			domain_id, address, flags, vflags);
>>   	}
>>   
>>   out:
>> @@ -932,29 +932,42 @@ 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);
>> +		u8 vflags = FIELD_GET(IO_PAGE_FAULT_VFLAGS_MASK, event[0]);
> 
> Move this assignment to up where we extract all other fields from event so that
> we don't need to repeat multiple times?

Since the vflags is only available in certain commands only. I'd rather 
keep them separate for better code readability.

>> +
>> +		amd_iommu_report_page_fault(iommu, devid, pasid, address, flags, vflags);
>>   		return;
>>   	}
>>   
>>   	switch (type) {
>>   	case EVENT_TYPE_ILL_DEV:
>> -		dev_err(dev, "Event logged [ILLEGAL_DEV_TABLE_ENTRY device=%04x:%02x:%02x.%x pasid=0x%05x address=0x%llx flags=0x%04x]\n",
>> +	{
>> +		u8 vflags = FIELD_GET(IO_PAGE_FAULT_VFLAGS_MASK, event[0]);
>> +
>> +		dev_err(dev, "Event logged [ILLEGAL_DEV_TABLE_ENTRY deice=%04x:%02x:%02x.%x pasid=0x%05x address=0x%llx flags=0x%04x vflags=%#x]\n",
>>   			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>> -			pasid, address, flags);
>> +			pasid, address, flags, vflags);
>>   		dev_err(dev, "Control Reg : 0x%llx\n", ctrl);
>>   		dump_dte_entry(iommu, devid);
>>   		break;
>> +	}
>>   	case EVENT_TYPE_DEV_TAB_ERR:
>> -		dev_err(dev, "Event logged [DEV_TAB_HARDWARE_ERROR device=%04x:%02x:%02x.%x "
>> -			"address=0x%llx flags=0x%04x]\n",
>> +	{
>> +		u8 vflags = FIELD_GET(IO_PAGE_FAULT_VFLAGS_MASK, event[0]);
>> +
>> +		dev_err(dev, "Event logged [DEV_TAB_HARDWARE_ERROR device=%04x:%02x:%02x.%x address=%#llx flags=%#04x vlfags=%#x]\n",
>>   			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>> -			address, flags);
>> +			address, flags, vflags);
>>   		break;
>> +	}
>>   	case EVENT_TYPE_PAGE_TAB_ERR:
>> -		dev_err(dev, "Event logged [PAGE_TAB_HARDWARE_ERROR device=%04x:%02x:%02x.%x pasid=0x%04x address=0x%llx flags=0x%04x]\n",
>> +	{
>> +		u8 vflags = FIELD_GET(IO_PAGE_FAULT_VFLAGS_MASK, event[0]);
>> +
>> +		dev_err(dev, "Event logged [PAGE_TAB_HARDWARE_ERROR device=%04x:%02x:%02x.%x pasid=0x%04x address=0x%llx flags=0x%04x vflags=%#x]\n",
>>   			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>> -			pasid, address, flags);
>> +			pasid, address, flags, vflags);
>>   		break;
>> +	}
>>   	case EVENT_TYPE_ILL_CMD:
>>   		dev_err(dev, "Event logged [ILLEGAL_COMMAND_ERROR address=0x%llx]\n", address);
>>   		dump_command(address);
>> @@ -986,6 +999,25 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
>>   			iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
>>   			pasid, address, flags, tag);
>>   		break;
>> +	case EVENT_TYPE_GUEST_EVENT_FAULT:
>> +	{
>> +		u8 gid = event[1] & 0xFFFF;
> 
> As discussed offline, s/u8/u16/. Also may be add a macro to get the gid from event?

Note. Fixed in v5.

Thanks

> -Vasant