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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>> ---
>>   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