| Message ID | bb95c2ffee689905293f73b6b71e8f5a5e666ec0.1738838825.git.teddy.astie@vates.tech (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN] iommu/amd: Remove redundant values redefinitions | expand |
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
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
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 ) {
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(-)