[PATCH] iommu/amd: Do not clear event/ppr log buffer when snp is enabled

[PATCH] iommu/amd: Do not clear event/ppr log buffer when snp is enabled

[Vasant Hegde]

From: Tom Lendacky <thomas.lendacky@amd.com>

Current code clears event log and ppr log entry after processing it due
to hardware errata ([1]  erratum #732, #733). We do not have hardware
issue on SNP enabled system.

When SNP is enabled, the event logs, PPR log and completion wait buffer
are read-only to the host (see SNP FW ABI spec [2]). Clearing those entry
will result in a kernel #PF for an RMP violation. Hence do not clear
event and ppr log entry after processing it.

[1] http://developer.amd.com/wordpress/media/2012/10/48931_15h_Mod_10h-1Fh_Rev_Guide.pdf
[2] https://www.amd.com/system/files/TechDocs/56860.pdf

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/iommu.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cbeaab55c0db..acc5e5e4f9e2 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -667,7 +667,13 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 			event[0], event[1], event[2], event[3]);
 	}
 
-	memset(__evt, 0, 4 * sizeof(u32));
+	if (!amd_iommu_snp_en) {
+		/*
+		 * To detect the hardware errata 732 we need to clear the
+		 * entry back to zero.
+		 */
+		memset(__evt, 0, 4 * sizeof(u32));
+	}
 }
 
 static void iommu_poll_events(struct amd_iommu *iommu)
@@ -736,10 +742,11 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu)
 		entry[1] = raw[1];
 
 		/*
-		 * To detect the hardware bug we need to clear the entry
-		 * back to zero.
+		 * To detect the hardware errata 733 we need to clear the
+		 * entry back to zero.
 		 */
-		raw[0] = raw[1] = 0UL;
+		if (!amd_iommu_snp_en)
+			raw[0] = raw[1] = 0UL;
 
 		/* Update head pointer of hardware ring-buffer */
 		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
-- 
2.31.1

Re: [PATCH] iommu/amd: Do not clear event/ppr log buffer when snp is enabled

[Joerg Roedel]

On Thu, Jan 05, 2023 at 09:18:04AM +0000, Vasant Hegde wrote:
> -	memset(__evt, 0, 4 * sizeof(u32));
> +	if (!amd_iommu_snp_en) {
> +		/*
> +		 * To detect the hardware errata 732 we need to clear the
> +		 * entry back to zero.
> +		 */
> +		memset(__evt, 0, 4 * sizeof(u32));
> +	}

This needs a comment that the buffers are not writeable with SNP
enabled.

>  }
>  
>  static void iommu_poll_events(struct amd_iommu *iommu)
> @@ -736,10 +742,11 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu)
>  		entry[1] = raw[1];
>  
>  		/*
> -		 * To detect the hardware bug we need to clear the entry
> -		 * back to zero.
> +		 * To detect the hardware errata 733 we need to clear the
> +		 * entry back to zero.
>  		 */
> -		raw[0] = raw[1] = 0UL;
> +		if (!amd_iommu_snp_en)
> +			raw[0] = raw[1] = 0UL;

Same here.

>  
>  		/* Update head pointer of hardware ring-buffer */
>  		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
> -- 
> 2.31.1
> 

Re: [PATCH] iommu/amd: Do not clear event/ppr log buffer when snp is enabled

[Vasant Hegde]

Joerg,


On 1/13/2023 9:23 PM, Joerg Roedel wrote:
> On Thu, Jan 05, 2023 at 09:18:04AM +0000, Vasant Hegde wrote:
>> -	memset(__evt, 0, 4 * sizeof(u32));
>> +	if (!amd_iommu_snp_en) {
>> +		/*
>> +		 * To detect the hardware errata 732 we need to clear the
>> +		 * entry back to zero.
>> +		 */
>> +		memset(__evt, 0, 4 * sizeof(u32));
>> +	}
> 
> This needs a comment that the buffers are not writeable with SNP
> enabled.

Sure. I will expand above comment and will post v2.


> 
>>  }
>>  
>>  static void iommu_poll_events(struct amd_iommu *iommu)
>> @@ -736,10 +742,11 @@ static void iommu_poll_ppr_log(struct amd_iommu *iommu)
>>  		entry[1] = raw[1];
>>  
>>  		/*
>> -		 * To detect the hardware bug we need to clear the entry
>> -		 * back to zero.
>> +		 * To detect the hardware errata 733 we need to clear the
>> +		 * entry back to zero.
>>  		 */
>> -		raw[0] = raw[1] = 0UL;
>> +		if (!amd_iommu_snp_en)
>> +			raw[0] = raw[1] = 0UL;
> 
> Same here.

Sure.

-Vasant