On 20.08.2025 14:28, Mykyta Poturai wrote:
> From: Luca Fancellu <luca.fance...@arm.com>
>
> Currently pci_add_device is called from hypercalls requested by Dom0
> to add pci devices and when the device has no domain associated with
> it, it is assumed that hardware_domain is the owner.
>
> On the dom0less scenario, the enumeration is performed by the
> firmware and Xen at boot time might want to assign some pci devices
> to guests, so it has to firstly add the device and then assign it to
> the final guest.
>
> Modify pci_add_device to have the owner domain passed as a parameter
> to the function, so that when it is called from the hypercall the
> owner would be the caller domain, otherwise when Xen is calling it,
> it would be another domain since hw domain could not be there
> (dom0less guests without Dom0 use case).
>
> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
> Signed-off-by: Mykyta Poturai <mykyta_potu...@epam.com>
> ---
> (cherry picked from commit f0c85d9043f7c9402e85b73361c8a13c683428ca from
> the downstream branch poc/pci-passthrough from
> https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git)
>
> v1->v2:
> * remove dom_io check
> * fixup pci_add_device parameters
> * use current->domain instead of hardware_domain
What I'm missing (as per my v1 comment) is discussion of the hardware_domain
-> current->domain change, including the XSM aspect. Because of the XSM aspect,
please also Cc the XSM maintainer going forward (I'm adding him here as well).
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -654,8 +654,9 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned
> int pos,
> return is64bits ? 2 : 1;
> }
>
> -int pci_add_device(u16 seg, u8 bus, u8 devfn,
> - const struct pci_dev_info *info, nodeid_t node)
> +int pci_add_device(uint16_t seg, uint8_t bus, uint8_t devfn,
> + const struct pci_dev_info *info, nodeid_t node,
> + struct domain *d)
> {
> struct pci_seg *pseg;
> struct pci_dev *pdev;
> @@ -663,6 +664,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> const char *type;
> int ret;
>
> + if ( !d )
> + return -EINVAL;
This should't be needed. Very remotely ASSERT(d) could be added here, but
we don't normally do so elsewhere.
> @@ -767,9 +771,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> ret = 0;
> if ( !pdev->domain )
> {
> - pdev->domain = hardware_domain;
> - write_lock(&hardware_domain->pci_lock);
> - list_add(&pdev->domain_list, &hardware_domain->pdev_list);
> + pdev->domain = d;
> + write_lock(&d->pci_lock);
> + list_add(&pdev->domain_list, &pdev->domain->pdev_list);
Why pdev->domain instead of the shorter and more efficient d?
Jan
