iommu/amd: Remove redundant values redefinitions

[XEN PATCH] iommu/amd: Remove redundant values redefinitions

Posted by Teddy Astie 1 year ago

In amd_iommu_setup_domain_device, we redefine req_id and ivrs_dev
without using it the first time we read it. This is effectively dead
logic that we can refactor.

Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index f96f59440b..1511a2a099 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -147,17 +147,14 @@ static int __must_check amd_iommu_setup_domain_device(
     if ( rc )
         return rc;
 
-    req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf);
-    ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
-    sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
-                ? 0 : SET_ROOT_VALID)
-               | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
-
-    /* get device-table entry */
     req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn));
     table = iommu->dev_table.buffer;
+    /* get device-table entry */
     dte = &table[req_id];
     ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
+    sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
+                ? 0 : SET_ROOT_VALID)
+               | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
 
     if ( domain != dom_io )
     {
-- 
2.47.2



Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH] iommu/amd: Remove redundant values redefinitions

Posted by Teddy Astie 1 year ago

Le 06/02/2025 à 11:49, Teddy Astie a écrit :
> In amd_iommu_setup_domain_device, we redefine req_id and ivrs_dev
> without using it the first time we read it. This is effectively dead
> logic that we can refactor.
> 
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> ---
>   xen/drivers/passthrough/amd/pci_amd_iommu.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index f96f59440b..1511a2a099 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -147,17 +147,14 @@ static int __must_check amd_iommu_setup_domain_device(
>       if ( rc )
>           return rc;
>   
> -    req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf);
> -    ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
> -    sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
> -                ? 0 : SET_ROOT_VALID)
> -               | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
> -
> -    /* get device-table entry */
>       req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn));
>       table = iommu->dev_table.buffer;
> +    /* get device-table entry */
>       dte = &table[req_id];
>       ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
> +    sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
> +                ? 0 : SET_ROOT_VALID)
> +               | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
>   
>       if ( domain != dom_io )
>       {

Looks like there is a subtle issue with this change when mapping a 
phantom device.
In this case, we have bus,devfn not matching pdev->sbdf, but we want to 
know if there are unity regions for the actual device (not its phantom bdf).

But there is only one check for SET_ROOT_WITH_UNITY_MAP (when req_id is 
different than PCI_BDF(bus, devfn). So I am not sure how problematic it is.

Teddy
Thanks


Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH] iommu/amd: Remove redundant values redefinitions

Posted by Jan Beulich 1 year ago

On 06.02.2025 12:04, Teddy Astie wrote:
> Le 06/02/2025 à 11:49, Teddy Astie a écrit :
>> In amd_iommu_setup_domain_device, we redefine req_id and ivrs_dev
>> without using it the first time we read it. This is effectively dead
>> logic that we can refactor.
>>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>> ---
>>   xen/drivers/passthrough/amd/pci_amd_iommu.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> index f96f59440b..1511a2a099 100644
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -147,17 +147,14 @@ static int __must_check amd_iommu_setup_domain_device(
>>       if ( rc )
>>           return rc;
>>   
>> -    req_id = get_dma_requestor_id(iommu->seg, pdev->sbdf.bdf);
>> -    ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
>> -    sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
>> -                ? 0 : SET_ROOT_VALID)
>> -               | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
>> -
>> -    /* get device-table entry */
>>       req_id = get_dma_requestor_id(iommu->seg, PCI_BDF(bus, devfn));
>>       table = iommu->dev_table.buffer;
>> +    /* get device-table entry */
>>       dte = &table[req_id];
>>       ivrs_dev = &get_ivrs_mappings(iommu->seg)[req_id];
>> +    sr_flags = (iommu_hwdom_passthrough && is_hardware_domain(domain)
>> +                ? 0 : SET_ROOT_VALID)
>> +               | (ivrs_dev->unity_map ? SET_ROOT_WITH_UNITY_MAP : 0);
>>   
>>       if ( domain != dom_io )
>>       {
> 
> Looks like there is a subtle issue with this change when mapping a 
> phantom device.
> In this case, we have bus,devfn not matching pdev->sbdf, but we want to 
> know if there are unity regions for the actual device (not its phantom bdf).

Which is the whole reason why req_id and ivrs_dev are calculated twice.
Note how the fix for XSA-400 deliberately added this 2nd instance.

Jan