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

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

Hi Joao,

Sorry. I missed to Cc you in V2 version.

Can you please look into
https://lore.kernel.org/linux-iommu/20230619133008.6221-1-vasant.hegde@amd.com/


On 6/20/2023 8:31 PM, Joao Martins wrote:
> 
> 
> 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"
>>  


.../...

>> @@ -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 (...)

Makes sense. Will fix it.

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

If I move it to below function doesnot work as next patch configures separate
interrupt for GA log which calls this function directly.

-Vasant