Re: [PATCH 1/2] iommu/amd: Add separate interrupt handler for PPR and GA log

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Jerry,


On 6/14/2023 3:08 AM, Jerry Snitselaar wrote:
> On Fri, Jun 09, 2023 at 10:20:24AM +0000, Vasant Hegde wrote:
>> The AMD IOMMU has three different logs (Event, PPR and GA) and it can be
>> configured to send separate interrupt for each log type.
>>   - Event log is used whenever IOMMU reports events like IO_PAGE_FAULT,
>>     TLB_INV_TIMEOUT, etc,. During normal system operation this log is not
>>     used actively.
>>
>>   - GA log is used to record device interrupt requests that could not be
>>     immediately delivered to the target virtual processor due the fact the
>>     target was not running. This is actively used when we do device
>>     passthrough to AVIC enabled guest.
>>
>>   - PPR log is used to service the page fault request from device in Shared
>>     Virtual Addressing (SVA) mode where page table is shared by CPU and
>>     device. In this mode it will generate PPR interrupt frequently.
>>
>> Currently we have single interrupt to handle all three logs. GA log and
>> PPR log usage is increasing. Hence, split interrupt handler thread
>> into three separate interrupt handler function. Following patch enables
>> separate interrupt for PPR and GA Log.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/amd_iommu.h |  3 ++
>>  drivers/iommu/amd/iommu.c     | 98 +++++++++++++++++++----------------
>>  2 files changed, 57 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index 156f57b4f78c..e2857109e966 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -12,6 +12,9 @@
>>  #include "amd_iommu_types.h"
>>  
>>  irqreturn_t amd_iommu_int_thread(int irq, void *data);
>> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data);
>> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data);
>> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data);
>>  irqreturn_t amd_iommu_int_handler(int irq, void *data);
>>  void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
>>  void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 3c179d548ecd..d427f7e3b869 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -841,57 +841,23 @@ static inline void
>>  amd_iommu_set_pci_msi_domain(struct device *dev, struct amd_iommu *iommu) { }
>>  #endif /* !CONFIG_IRQ_REMAP */
>>  
>> -#define AMD_IOMMU_INT_MASK	\
>> -	(MMIO_STATUS_EVT_OVERFLOW_MASK | \
>> -	 MMIO_STATUS_EVT_INT_MASK | \
>> -	 MMIO_STATUS_PPR_OVERFLOW_MASK | \
>> -	 MMIO_STATUS_PPR_INT_MASK | \
>> -	 MMIO_STATUS_GALOG_OVERFLOW_MASK | \
>> -	 MMIO_STATUS_GALOG_INT_MASK)
>> -
>> -irqreturn_t amd_iommu_int_thread(int irq, void *data)
>> +static void amd_iommu_handle_irq(void *data, u32 int_mask, u32 overflow_mask,
>> +				 void (*int_handler)(struct amd_iommu *),
>> +				 void (*overflow_handler)(struct amd_iommu *))
>>  {
>>  	struct amd_iommu *iommu = (struct amd_iommu *) data;
>>  	u32 status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>> +	u32 mask = int_mask | overflow_mask;
>>  
>> -	while (status & AMD_IOMMU_INT_MASK) {
>> +	while (status & mask) {
>>  		/* Enable interrupt sources again */
>> -		writel(AMD_IOMMU_INT_MASK,
>> -			iommu->mmio_base + MMIO_STATUS_OFFSET);
>> -
>> -		if (status & MMIO_STATUS_EVT_INT_MASK) {
>> -			pr_devel("Processing IOMMU Event Log\n");
>> -			iommu_poll_events(iommu);
>> -		}
>> -
>> -		if (status & (MMIO_STATUS_PPR_INT_MASK |
>> -			      MMIO_STATUS_PPR_OVERFLOW_MASK)) {
>> -			pr_devel("Processing IOMMU PPR Log\n");
>> -			iommu_poll_ppr_log(iommu);
>> -		}
>> +		writel(mask, iommu->mmio_base + MMIO_STATUS_OFFSET);
>>  
>> -		if (status & MMIO_STATUS_PPR_OVERFLOW_MASK) {
>> -			pr_info_ratelimited("IOMMU PPR log overflow\n");
>> -			amd_iommu_restart_ppr_log(iommu);
>> -		}
>> +		if (int_handler)
>> +			int_handler(iommu);
>>  
>> -#ifdef CONFIG_IRQ_REMAP
>> -		if (status & (MMIO_STATUS_GALOG_INT_MASK |
>> -			      MMIO_STATUS_GALOG_OVERFLOW_MASK)) {
>> -			pr_devel("Processing IOMMU GA Log\n");
>> -			iommu_poll_ga_log(iommu);
>> -		}
>> -
>> -		if (status & MMIO_STATUS_GALOG_OVERFLOW_MASK) {
>> -			pr_info_ratelimited("IOMMU GA Log overflow\n");
>> -			amd_iommu_restart_ga_log(iommu);
>> -		}
>> -#endif
>> -
>> -		if (status & MMIO_STATUS_EVT_OVERFLOW_MASK) {
>> -			pr_info_ratelimited("IOMMU event log overflow\n");
>> -			amd_iommu_restart_event_logging(iommu);
>> -		}
>> +		if ((status & overflow_mask) && overflow_handler)
>> +			overflow_handler(iommu);
>>  
>>  		/*
>>  		 * Hardware bug: ERBT1312
>> @@ -908,6 +874,50 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
>>  		 */
>>  		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>>  	}
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_evtlog(int irq, void *data)
>> +{
>> +	pr_devel("Processing IOMMU Event Log\n");
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_EVT_INT_MASK,
>> +			     MMIO_STATUS_EVT_OVERFLOW_MASK,
>> +			     iommu_poll_events, amd_iommu_restart_event_logging);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
>> +{
>> +	pr_devel("Processing IOMMU PPR Log\n");
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_PPR_INT_MASK,
>> +			     MMIO_STATUS_PPR_OVERFLOW_MASK,
>> +			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +irqreturn_t amd_iommu_int_thread_galog(int irq, void *data)
>> +{
>> +
>> +	pr_devel("Processing IOMMU GA Log\n");
>> +#ifdef CONFIG_IRQ_REMAP
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
>> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK,
>> +			     iommu_poll_ga_log, amd_iommu_restart_ga_log);
>> +#else
>> +	amd_iommu_handle_irq(data, MMIO_STATUS_GALOG_INT_MASK,
>> +			     MMIO_STATUS_GALOG_OVERFLOW_MASK, NULL, NULL);
>> +#endif
> 
> Is the else needed here, since GAEn will only get set if CONFIG_IRQ_REMAP is enabled?

In the current flow we clear the the interrupt bits (including GALOG_INT and
GALOG_OVERFLOW) first and then if IRQ_REMAP is enabled then we process GA log.
To match that flow I have added else part.

But you are right. We enable GALOG only when IRQ_REMAP is enabled.
Probably I can drop else part.


-Vasant