diff mbox series

iommu/amd: Fix "Guest Virtual APIC Table Root Pointer" configuration in IRTE

Message ID 20230331061723.10337-1-kvijayab@amd.com
State New
Headers
Series iommu/amd: Fix "Guest Virtual APIC Table Root Pointer" configuration in IRTE |

Commit Message

Kishon Vijay Abraham I March 31, 2023, 6:17 a.m. UTC 
  commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC
(de-)activation code") while refactoring guest virtual APIC
activation/de-activation code, stored information for activate/de-activate
in "struct amd_ir_data". It used 32-bit integer data type for storing the
"Guest Virtual APIC Table Root Pointer" (ga_root_ptr), though the
"ga_root_ptr" is actually a 40-bit field in IRTE (Interrupt Remapping
Table Entry).

This causes interrupts from PCIe devices to not reach the guest in the case
of PCIe passthrough with SME (Secure Memory Encryption) enabled as _SME_
bit in the "ga_root_ptr" is lost before writing it to the IRTE.

Fix it by using 64-bit data type for storing the "ga_root_ptr".

Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code")
Cc: stable@vger.kernel.org # v5.4+
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Suravee Suthikulpanit April 3, 2023, 4:29 a.m. UTC | #1 
Kishon,

On 3/31/2023 1:17 PM, Kishon Vijay Abraham I wrote:
> commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC
> (de-)activation code") while refactoring guest virtual APIC
> activation/de-activation code, stored information for activate/de-activate
> in "struct amd_ir_data". It used 32-bit integer data type for storing the
> "Guest Virtual APIC Table Root Pointer" (ga_root_ptr), though the
> "ga_root_ptr" is actually a 40-bit field in IRTE (Interrupt Remapping
> Table Entry).
> 
> This causes interrupts from PCIe devices to not reach the guest in the case
> of PCIe passthrough with SME (Secure Memory Encryption) enabled as _SME_
> bit in the "ga_root_ptr" is lost before writing it to the IRTE.
> 
> Fix it by using 64-bit data type for storing the "ga_root_ptr".
> 
> Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code")
> Cc: stable@vger.kernel.org # v5.4+
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>

Please also add
Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

Thanks,
Suravee
Joao Martins April 3, 2023, 10:11 a.m. UTC | #2 
On 03/04/2023 05:29, Suthikulpanit, Suravee wrote:
> Kishon,
> 
> On 3/31/2023 1:17 PM, Kishon Vijay Abraham I wrote:
>> commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC
>> (de-)activation code") while refactoring guest virtual APIC
>> activation/de-activation code, stored information for activate/de-activate
>> in "struct amd_ir_data". It used 32-bit integer data type for storing the
>> "Guest Virtual APIC Table Root Pointer" (ga_root_ptr), though the
>> "ga_root_ptr" is actually a 40-bit field in IRTE (Interrupt Remapping
>> Table Entry).
>>
>> This causes interrupts from PCIe devices to not reach the guest in the case
>> of PCIe passthrough with SME (Secure Memory Encryption) enabled as _SME_
>> bit in the "ga_root_ptr" is lost before writing it to the IRTE.
>>
>> Fix it by using 64-bit data type for storing the "ga_root_ptr".
>>
>> Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation
>> code")
>> Cc: stable@vger.kernel.org # v5.4+
>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
> 
> Please also add
> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  drivers/iommu/amd/amd_iommu_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 3d684190b4d5..990614b8a1fe 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -1001,7 +1001,7 @@ struct amd_ir_data {
>  	 */
>  	struct irq_cfg *cfg;
>  	int ga_vector;
> -	int ga_root_ptr;
> +	u64 ga_root_ptr;
>  	int ga_tag;
>  };

Nice catch. While at it I noticed the following:

* ga_tag: The conversion of uint (in kvm as they use proper u32 for gatag) to
int will miss a bit thus at least VM IDs with the msb set will be stripped from
vCPU wake ups? Fortunately the 24 bits for VCPU ID wouldn't be affected. But
ga_tag should probably be a u32.

* ga_vector: this in principle should be u8 per spec, maybe it is OK to stay as
is to prevent compiler warnings as IOMMU intremap struct vcpu_data is using a u32.

	Joao
Kishon Vijay Abraham I April 4, 2023, 5:50 a.m. UTC | #3 
Hi Joao,

On 4/3/2023 3:41 PM, Joao Martins wrote:
> On 03/04/2023 05:29, Suthikulpanit, Suravee wrote:
>> Kishon,
>>
>> On 3/31/2023 1:17 PM, Kishon Vijay Abraham I wrote:
>>> commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC
>>> (de-)activation code") while refactoring guest virtual APIC
>>> activation/de-activation code, stored information for activate/de-activate
>>> in "struct amd_ir_data". It used 32-bit integer data type for storing the
>>> "Guest Virtual APIC Table Root Pointer" (ga_root_ptr), though the
>>> "ga_root_ptr" is actually a 40-bit field in IRTE (Interrupt Remapping
>>> Table Entry).
>>>
>>> This causes interrupts from PCIe devices to not reach the guest in the case
>>> of PCIe passthrough with SME (Secure Memory Encryption) enabled as _SME_
>>> bit in the "ga_root_ptr" is lost before writing it to the IRTE.
>>>
>>> Fix it by using 64-bit data type for storing the "ga_root_ptr".
>>>
>>> Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation
>>> code")
>>> Cc: stable@vger.kernel.org # v5.4+
>>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
>>
>> Please also add
>> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   drivers/iommu/amd/amd_iommu_types.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index 3d684190b4d5..990614b8a1fe 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -1001,7 +1001,7 @@ struct amd_ir_data {
>>   	 */
>>   	struct irq_cfg *cfg;
>>   	int ga_vector;
>> -	int ga_root_ptr;
>> +	u64 ga_root_ptr;
>>   	int ga_tag;
>>   };
> 
> Nice catch. While at it I noticed the following:
> 
> * ga_tag: The conversion of uint (in kvm as they use proper u32 for gatag) to
> int will miss a bit thus at least VM IDs with the msb set will be stripped from
> vCPU wake ups? Fortunately the 24 bits for VCPU ID wouldn't be affected. But
> ga_tag should probably be a u32.
> 
> * ga_vector: this in principle should be u8 per spec, maybe it is OK to stay as
> is to prevent compiler warnings as IOMMU intremap struct vcpu_data is using a u32.

Thank you for reviewing. Will add fix for other data types in my v2 of 
this patch.

Regards,
Kishon
Kishon Vijay Abraham I April 4, 2023, 5:51 a.m. UTC | #4 
Hi Suravee,

On 4/3/2023 9:59 AM, Suthikulpanit, Suravee wrote:
> Kishon,
> 
> On 3/31/2023 1:17 PM, Kishon Vijay Abraham I wrote:
>> commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC
>> (de-)activation code") while refactoring guest virtual APIC
>> activation/de-activation code, stored information for 
>> activate/de-activate
>> in "struct amd_ir_data". It used 32-bit integer data type for storing the
>> "Guest Virtual APIC Table Root Pointer" (ga_root_ptr), though the
>> "ga_root_ptr" is actually a 40-bit field in IRTE (Interrupt Remapping
>> Table Entry).
>>
>> This causes interrupts from PCIe devices to not reach the guest in the 
>> case
>> of PCIe passthrough with SME (Secure Memory Encryption) enabled as _SME_
>> bit in the "ga_root_ptr" is lost before writing it to the IRTE.
>>
>> Fix it by using 64-bit data type for storing the "ga_root_ptr".
>>
>> Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC 
>> (de-)activation code")
>> Cc: stable@vger.kernel.org # v5.4+
>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
> 
> Please also add
> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

sure, will add it in v2 of this patch.

Regards,
Kishon
diff mbox series

Patch

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 3d684190b4d5..990614b8a1fe 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -1001,7 +1001,7 @@  struct amd_ir_data {
 	 */
 	struct irq_cfg *cfg;
 	int ga_vector;
-	int ga_root_ptr;
+	u64 ga_root_ptr;
 	int ga_tag;
 };