Re: [PATCH 1/2] iommu/amd: Generalize log overflow handling

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

Hi Joao,


On 6/20/2023 8:21 PM, Joao Martins wrote:
> 
> 
> On 09/06/2023 11:17, Vasant Hegde wrote:
>> IOMMU has three different logs (Event, GA and PPR log) and each uses
>> different buffer for logging. Once buffer becomes full IOMMU generates
>> overflow interrupt and we have to restart the logs. Log restart procedure
>> is same for all three logs except it uses different bits in 'MMIO Offset
>> 2020h IOMMU Status Register'. Hence lets move common code to generic
>> function and individual log overflow handler will call generic function
>> with appropriate parameters.
>>
>> Also rename MMIO_STATUS_EVT_OVERFLOW_INT_MASK as
>> MMIO_STATUS_EVT_OVERFLOW_MASK.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> 
> Really nice rework, fwiw:
> 
> 	Reviewed-by: Joao Martins <joao.m.martins@oracle.com>

I did post V2 version, but forgot to CC you. Sorry for that.
Can you please look into V2?

V2 ->
https://lore.kernel.org/linux-iommu/20230619132346.6021-1-vasant.hegde@amd.com/



> Just a though below (you don't need to address it, just an idea)
> 
>> ---
>>  drivers/iommu/amd/amd_iommu_types.h |  3 +-
>>  drivers/iommu/amd/init.c            | 51 ++++++++++++++++++-----------
>>  drivers/iommu/amd/iommu.c           |  4 +--
>>  3 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index 364fdaa52e74..318b84cf47f6 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -120,9 +120,10 @@
>>  #define PASID_MASK		0x0000ffff
>>  
>>  /* MMIO status bits */
>> -#define MMIO_STATUS_EVT_OVERFLOW_INT_MASK	BIT(0)
>> +#define MMIO_STATUS_EVT_OVERFLOW_MASK		BIT(0)
>>  #define MMIO_STATUS_EVT_INT_MASK		BIT(1)
>>  #define MMIO_STATUS_COM_WAIT_INT_MASK		BIT(2)
>> +#define MMIO_STATUS_EVT_RUN_MASK		BIT(3)
>>  #define MMIO_STATUS_PPR_INT_MASK		BIT(6)
>>  #define MMIO_STATUS_GALOG_RUN_MASK		BIT(8)
>>  #define MMIO_STATUS_GALOG_OVERFLOW_MASK		BIT(9)
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index c2d80a4e5fb0..3c21e9333899 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -748,38 +748,51 @@ static int __init alloc_command_buffer(struct amd_iommu *iommu)
>>  	return iommu->cmd_buf ? 0 : -ENOMEM;
>>  }
>>  
>> +/*
>> + * Interrupt handler has processed all pending events and adjusted head
>> + * and tail pointer. Reset overflow mask and restart logging again.
>> + */
>> +static void amd_iommu_restart_log(struct amd_iommu *iommu, const char *evt_type,
>> +				  u8 cntrl_intr, u8 cntrl_log,
>> +				  u32 status_run_mask, u32 status_overflow_mask)
>> +{
>> +	u32 status;
>> +
>> +	status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
>> +	if (status & status_run_mask)
>> +		return;
>> +
>> +	pr_info_ratelimited("IOMMU %s log restarting\n", evt_type);
>> +
>> +	iommu_feature_disable(iommu, cntrl_log);
>> +	iommu_feature_disable(iommu, cntrl_intr);
>> +
>> +	writel(status_overflow_mask, iommu->mmio_base + MMIO_STATUS_OFFSET);
>> +
>> +	iommu_feature_enable(iommu, cntrl_intr);
>> +	iommu_feature_enable(iommu, cntrl_log);
>> +}
>> +
>>  /*
>>   * This function restarts event logging in case the IOMMU experienced
>>   * an event log buffer overflow.
>>   */
>>  void amd_iommu_restart_event_logging(struct amd_iommu *iommu)
>>  {
>> -	iommu_feature_disable(iommu, CONTROL_EVT_LOG_EN);
>> -	iommu_feature_enable(iommu, CONTROL_EVT_LOG_EN);
>> +	amd_iommu_restart_log(iommu, "Event", CONTROL_EVT_INT_EN,
>> +			      CONTROL_EVT_LOG_EN, MMIO_STATUS_EVT_RUN_MASK,
>> +			      MMIO_STATUS_EVT_OVERFLOW_MASK);
>>  }
>>  
> 
> If CONTROL_EVT_{INT,LOG} was instead EVT{INT,LOG} (like it's PPR and GA
> counterparts), I do wonder if a macro is better placed here considering that the
> only think that changes is the string name and whether it's EVT, PPR or GA is
> used. But perhaps that might be too obscure.

Sure. I am touching some of the init code in next series. I will try to fix it
as part of that.

-Vasant