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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

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).

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