On 24.09.2025 09:59, Mykyta Poturai wrote:
> @@ -133,6 +134,12 @@ void arch_iommu_domain_destroy(struct domain *d)
> {
> }
>
> +static int iommu_add_hwdom_pci_device(u8 devfn, struct pci_dev *pdev)
> +{
> + const struct domain_iommu *hd = dom_iommu(hardware_domain);
> + return iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
Nit (style): Blank line please between declaration(s) and statement(s). And
blank line please also ahead of the main return statement of a function.
> @@ -142,6 +149,8 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> if ( iommu_hwdom_reserved == 1 )
> printk(XENLOG_WARNING "map-reserved dom0-iommu option is not
> supported on ARM\n");
> iommu_hwdom_reserved = 0;
> +
> + setup_hwdom_pci_devices(d, iommu_add_hwdom_pci_device);
With this function being __hwdom_init, why is iommu_add_hwdom_pci_device()
not also given that attribute?
As to cf_check I don't know what the Arm policy is: My suggestion would be
to put that attribute wherever it is (potentially) needed.
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -20,6 +20,7 @@
> #include <xen/pci_ids.h>
> #include <xen/list.h>
> #include <xen/prefetch.h>
> +#include <xen/iocap.h>
> #include <xen/iommu.h>
> #include <xen/irq.h>
> #include <xen/param.h>
> @@ -1040,6 +1041,12 @@ enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
> return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
> }
>
> +static bool pdev_is_endpoint(struct pci_dev *pdev)
__hwdom_init? And parameter pointer-to-const?
> +{
> + enum pdev_type type = pdev_type(pdev->seg, pdev->bus, pdev->devfn);
> + return type == DEV_TYPE_PCIe_ENDPOINT || type == DEV_TYPE_PCI;
> +}
> +
> /*
> * find the upstream PCIe-to-PCI/PCIX bridge or PCI legacy bridge
> * return 0: the device is integrated PCI device or PCIe
> @@ -1255,7 +1262,7 @@ static void __hwdom_init setup_one_hwdom_device(const
> struct setup_hwdom *ctxt,
> struct pci_dev *pdev)
> {
> u8 devfn = pdev->devfn;
> - int err;
> + int err, i, rc;
i clearly wants to be of an unsigned type. And rc, afaics, can have its scope
limited to the loop body.
> @@ -1276,6 +1283,34 @@ static void __hwdom_init setup_one_hwdom_device(const
> struct setup_hwdom *ctxt,
> if ( err )
> printk(XENLOG_ERR "setup of vPCI for d%d failed: %d\n",
> ctxt->d->domain_id, err);
> +
> + if ( !hwdom_uses_vpci() )
> + return;
> +
> + for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS; i += rc )
> + {
> + uint64_t addr, size;
> + uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +
> + if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE)
> + == PCI_BASE_ADDRESS_SPACE_IO )
Nit (style): Operator placement again.
> + {
> + rc = 1;
> + continue;
> + }
> +
> + rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
> + (i == PCI_HEADER_NORMAL_NR_BARS - 1)
> + ? PCI_BAR_LAST : 0);
Nit (style): Indentation again.
> +
> + if ( !size )
> + continue;
> +
> + err = iomem_permit_access(hardware_domain, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
And again.
> + if ( err )
> + break;
> + }
> }
This change supports my comment on the earlier patch, regarding the option
of doing here some of what needs doing, rather than by another round of
iterating all devices.
> @@ -1294,6 +1329,9 @@ static int __hwdom_init cf_check
> _setup_hwdom_pci_devices(
> if ( !pdev )
> continue;
>
> + if ( hwdom_uses_vpci() && !pdev_is_endpoint(pdev) )
> + continue;
> +
> if ( !pdev->domain )
> {
> pdev->domain = ctxt->d;
This is (logically) wrong: On x86 PVH Dom0 uses vPCI but wants all devices
to be handed to it. _This_ may be a place where has_vpci_bridge() might
make sense to use (simply by its name, that is).
Jan
