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: [email protected] # v5.4+
Reviewed-by: Suravee Suthikulpanit <[email protected]>
Signed-off-by: Kishon Vijay Abraham I <[email protected]>
---
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;
};
--
2.34.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: [email protected] # v5.4+
> Reviewed-by: Suravee Suthikulpanit <[email protected]>
> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
Please also add
Reported-by: Alejandro Jimenez <[email protected]>
Thanks,
Suravee
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: [email protected] # v5.4+
>> Reviewed-by: Suravee Suthikulpanit <[email protected]>
>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>
> Please also add
> Reported-by: Alejandro Jimenez <[email protected]>
> ---
> 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
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: [email protected] # v5.4+
>>> Reviewed-by: Suravee Suthikulpanit <[email protected]>
>>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>>
>> Please also add
>> Reported-by: Alejandro Jimenez <[email protected]>
>> ---
>> 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
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: [email protected] # v5.4+
>> Reviewed-by: Suravee Suthikulpanit <[email protected]>
>> Signed-off-by: Kishon Vijay Abraham I <[email protected]>
>
> Please also add
> Reported-by: Alejandro Jimenez <[email protected]>
sure, will add it in v2 of this patch.
Regards,
Kishon