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

[Joao Martins <joao.m.martins@oracle.com>]

On 09/06/2023 11:20, 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");

Similar to the overflow series you could probably move this pr_devel inside
amd_iommu_handle_irq with a const string as an added argument (...)

> +	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");

here (...)

> +	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");

here (...)

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

As you and Jerry was discussing the else is probably not needed. Although it's
probably easier to just move the ifdef into the function below (...)

> +	return IRQ_HANDLED;
> +}
> +
> +irqreturn_t amd_iommu_int_thread(int irq, void *data)
> +{
> +	amd_iommu_int_thread_evtlog(irq, data);
> +	amd_iommu_int_thread_pprlog(irq, data);
> +	amd_iommu_int_thread_galog(irq, data);
> +

... here. Provided the code was before was behaving similarly and in theory you
wouldn't even setup the galog IRQ .

>  	return IRQ_HANDLED;
>  }
>