| Message ID | 20231109182716.367119-6-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
Hi Stewart, On 09/11/2023 18:27, Stewart Hildebrand wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> In general, we should avoid blank commit. In this case, I think some details would be useful about the implementation. Some of the details I am interested in is how the logic works and why we don't handle the same quarantine options as x86? > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > --- > v5->v6: > * check for hardware_domain == NULL (dom0less test case) > * locking: assign pdev->domain before list_add() > > v4->v5: > * assign device to pdev->domain (usually dom0) by default in add_device() hook > * deassign from hwdom > * rebase on top of ("dynamic node programming using overlay dtbo") series > * remove TODO in comment about device prints > * add TODO regarding locking > * fixup after dropping ("xen/arm: Move is_protected flag to struct device") > > v3->v4: > * add new device_is_protected check in add_device hook to match SMMUv3 and > IPMMU-VMSA drivers > > v2->v3: > * invoke iommu_add_pci_sideband_ids() from add_device hook > > v1->v2: > * ignore add_device/assign_device/reassign_device calls for phantom functions > (i.e. devfn != pdev->devfn) > > downstream->v1: > * wrap unused function in #ifdef 0 > * remove the remove_device() stub since it was submitted separately to the list > [XEN][PATCH v6 12/19] xen/smmu: Add remove_device callback for smmu_iommu ops > https://lists.xenproject.org/archives/html/xen-devel/2023-05/msg00204.html > * arm_smmu_(de)assign_dev: return error instead of crashing system > * update condition in arm_smmu_reassign_dev > * style fixup > * add && !is_hardware_domain(d) into condition in arm_smmu_assign_dev() > > (cherry picked from commit 0c11a7f65f044c26d87d1e27ac6283ef1f9cfb7a from > the downstream branch spider-master from > https://github.com/xen-troops/xen.git) > --- > xen/drivers/passthrough/arm/smmu.c | 199 ++++++++++++++++++++++++----- > 1 file changed, 169 insertions(+), 30 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index 11fc1d22ef0a..24d1c0353025 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -131,11 +131,21 @@ enum irqreturn { > > typedef enum irqreturn irqreturn_t; > > -/* Device logger functions > - * TODO: Handle PCI > - */ > -#define dev_print(dev, lvl, fmt, ...) \ > - printk(lvl "smmu: %s: " fmt, dt_node_full_name(dev_to_dt(dev)), ## __VA_ARGS__) > +/* Device logger functions */ > +#ifndef CONFIG_HAS_PCI > +#define dev_print(dev, lvl, fmt, ...) \ > + printk(lvl "smmu: %s: " fmt, dev_name(dev), ## __VA_ARGS__) > +#else > +#define dev_print(dev, lvl, fmt, ...) ({ \ > + if ( !dev_is_pci((dev)) ) \ > + printk(lvl "smmu: %s: " fmt, dev_name((dev)), ## __VA_ARGS__); \ > + else \ > + { \ > + struct pci_dev *pdev = dev_to_pci((dev)); \ > + printk(lvl "smmu: %pp: " fmt, &pdev->sbdf, ## __VA_ARGS__); \ > + } \ > +}) > +#endif > > #define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__) > #define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__) > @@ -187,6 +197,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev, > * Xen: PCI functions > * TODO: It should be implemented when PCI will be supported > */ > +#if 0 /* unused */ > #define to_pci_dev(dev) (NULL) > static inline int pci_for_each_dma_alias(struct pci_dev *pdev, > int (*fn) (struct pci_dev *pdev, > @@ -196,6 +207,7 @@ static inline int pci_for_each_dma_alias(struct pci_dev *pdev, > BUG(); > return 0; > } > +#endif > > /* Xen: misc */ > #define PHYS_MASK_SHIFT PADDR_BITS > @@ -631,7 +643,7 @@ struct arm_smmu_master_cfg { > for (i = 0; idx = cfg->smendx[i], i < num; ++i) > > struct arm_smmu_master { > - struct device_node *of_node; > + struct device *dev; > struct rb_node node; > struct arm_smmu_master_cfg cfg; > }; > @@ -723,7 +735,7 @@ arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg) > { > struct arm_smmu_master *master = container_of(cfg, > struct arm_smmu_master, cfg); > - return dev_iommu_fwspec_get(&master->of_node->dev); > + return dev_iommu_fwspec_get(master->dev); > } > > static void parse_driver_options(struct arm_smmu_device *smmu) > @@ -756,7 +768,7 @@ static struct device_node *dev_get_dev_node(struct device *dev) > } > > static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu, > - struct device_node *dev_node) > + struct device *dev) > { > struct rb_node *node = smmu->masters.rb_node; > > @@ -765,9 +777,9 @@ static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu, > > master = container_of(node, struct arm_smmu_master, node); > > - if (dev_node < master->of_node) > + if (dev < master->dev) > node = node->rb_left; > - else if (dev_node > master->of_node) > + else if (dev > master->dev) > node = node->rb_right; > else > return master; > @@ -802,9 +814,9 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, > = container_of(*new, struct arm_smmu_master, node); > > parent = *new; > - if (master->of_node < this->of_node) > + if (master->dev < this->dev) > new = &((*new)->rb_left); > - else if (master->of_node > this->of_node) > + else if (master->dev > this->dev) > new = &((*new)->rb_right); > else > return -EEXIST; > @@ -836,28 +848,37 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu, > struct arm_smmu_master *master; > struct device_node *dev_node = dev_get_dev_node(dev); Looking at this call, there is a Todo "Add support of PCI". If this function is now only meant to be used for platform device, then should we remove the TODO in it? > > - master = find_smmu_master(smmu, dev_node); > + master = find_smmu_master(smmu, dev); > if (master) { > dev_err(dev, > "rejecting multiple registrations for master device %s\n", > - dev_node->name); > + dev_node ? dev_node->name : ""); Printing "" looks a bit odd. But I wonder if this is actually redundant with the content printed by dev_err()? > return -EBUSY; > } > > master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL); > if (!master) > return -ENOMEM; > - master->of_node = dev_node; > + master->dev = dev; > > - /* Xen: Let Xen know that the device is protected by an SMMU */ > - dt_device_set_protected(dev_node); > + if ( !dev_is_pci(dev) ) I think a comment should stay to explain how someone known that the PCI device will be protected by an IOMMU. > + { > + if ( dt_device_is_protected(dev_node) ) > + { > + dev_err(dev, "Already added to SMMU\n"); > + return -EEXIST; > + } This checks seems to be unrelated with the rest of the patch. Can you explain? > + > + /* Xen: Let Xen know that the device is protected by an SMMU */ > + dt_device_set_protected(dev_node); > + } > > for (i = 0; i < fwspec->num_ids; ++i) { > if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && > (fwspec->ids[i] >= smmu->num_mapping_groups)) { > dev_err(dev, > "stream ID for master device %s greater than maximum allowed (%d)\n", > - dev_node->name, smmu->num_mapping_groups); > + dev_node ? dev_node->name : "", smmu->num_mapping_groups); Same remark as above for "". > return -ERANGE; > } > master->cfg.smendx[i] = INVALID_SMENDX; > @@ -872,7 +893,7 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu, > struct device_node *dev_node = dev_get_dev_node(dev); > int ret; > > - master = find_smmu_master(smmu, dev_node); > + master = find_smmu_master(smmu, dev); > if (master == NULL) { > dev_err(dev, > "No registrations found for master device %s\n", > @@ -884,8 +905,9 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu, > if (ret) > return ret; > > - /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */ > - dev_node->is_protected = false; > + if ( !dev_is_pci(dev) ) > + /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */ > + dev_node->is_protected = false; > > kfree(master); > return 0; > @@ -914,6 +936,12 @@ static int register_smmu_master(struct arm_smmu_device *smmu, > fwspec); > } > > +/* Forward declaration */ > +static int arm_smmu_assign_dev(struct domain *d, u8 devfn, > + struct device *dev, u32 flag); > +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, > + struct device *dev); > + > /* > * The driver which supports generic IOMMU DT bindings must have this > * callback implemented. > @@ -938,6 +966,22 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev) > { > struct arm_smmu_device *smmu; > struct iommu_fwspec *fwspec; > + int ret; > + > +#ifdef CONFIG_HAS_PCI > + if ( dev_is_pci(dev) ) > + { > + struct pci_dev *pdev = dev_to_pci(dev); > + int ret; > + > + if ( devfn != pdev->devfn ) > + return 0; > + > + ret = iommu_add_pci_sideband_ids(pdev); > + if ( ret < 0 ) > + iommu_fwspec_free(dev); > + } > +#endif > > fwspec = dev_iommu_fwspec_get(dev); > if (fwspec == NULL) > @@ -947,7 +991,24 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev) > if (smmu == NULL) > return -ENXIO; > > - return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); > + ret = arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); > + if ( ret ) axc> + return ret; > + > +#ifdef CONFIG_HAS_PCI > + if ( dev_is_pci(dev) ) > + { > + struct pci_dev *pdev = dev_to_pci(dev); > + > + /* > + * During PHYSDEVOP_pci_device_add, Xen does not assign the > + * device, so we must do it here. > + */ In the context of dom0less, the hardware domain may not exist at all. And even if it exists, you will first assign the device to the hardware domain and then re-assign to the guest just after. This looks wrong. > + ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0); > + } > +#endif > + > + return ret; > } > > static int arm_smmu_dt_xlate_generic(struct device *dev, > @@ -970,11 +1031,10 @@ static struct arm_smmu_device *find_smmu_for_device(struct device *dev) > { > struct arm_smmu_device *smmu; > struct arm_smmu_master *master = NULL; > - struct device_node *dev_node = dev_get_dev_node(dev); > > spin_lock(&arm_smmu_devices_lock); > list_for_each_entry(smmu, &arm_smmu_devices, list) { > - master = find_smmu_master(smmu, dev_node); > + master = find_smmu_master(smmu, dev); > if (master) > break; > } > @@ -2066,6 +2126,7 @@ static bool arm_smmu_capable(enum iommu_cap cap) > } > #endif > > +#if 0 /* Not used */ > static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *data) > { > *((u16 *)data) = alias; > @@ -2076,6 +2137,7 @@ static void __arm_smmu_release_pci_iommudata(void *data) > { > kfree(data); > } > +#endif > > static int arm_smmu_add_device(struct device *dev) > { > @@ -2083,12 +2145,13 @@ static int arm_smmu_add_device(struct device *dev) > struct arm_smmu_master_cfg *cfg; > struct iommu_group *group; > void (*releasefn)(void *data) = NULL; > - int ret; > > smmu = find_smmu_for_device(dev); > if (!smmu) > return -ENODEV; > > + /* There is no need to distinguish here, thanks to PCI-IOMMU DT bindings */ > +#if 0 > if (dev_is_pci(dev)) { > struct pci_dev *pdev = to_pci_dev(dev); > struct iommu_fwspec *fwspec; > @@ -2113,10 +2176,12 @@ static int arm_smmu_add_device(struct device *dev) > &fwspec->ids[0]); > releasefn = __arm_smmu_release_pci_iommudata; > cfg->smmu = smmu; > - } else { > + } else > +#endif > + { > struct arm_smmu_master *master; > > - master = find_smmu_master(smmu, dev->of_node); > + master = find_smmu_master(smmu, dev); > if (!master) { > return -ENODEV; > } > @@ -2784,6 +2849,61 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn, > return -ENOMEM; > } > > +#ifdef CONFIG_HAS_PCI > + if ( dev_is_pci(dev) && !is_hardware_domain(d) ) > + { > + struct pci_dev *pdev = dev_to_pci(dev); > + > + printk(XENLOG_INFO "Assigning device %04x:%02x:%02x.%u to dom%d\n", > + pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + d->domain_id); Is this a left-over debug? If not, shouldn't this happen in a caller instead because this is quite generic? > + > + if ( devfn != pdev->devfn || pdev->domain == d ) > + return 0; > + > + ASSERT(pcidevs_locked()); > + > + /* TODO: acquire pci_lock */ What's the plan for this TODO? (The commit message is blank). > +#if 0 > + write_lock(&pdev->domain->pci_lock); > +#endif > + list_del(&pdev->domain_list); > +#if 0 > + write_unlock(&pdev->domain->pci_lock); > +#endif > + > + pdev->domain = d; > + > +#if 0 > + write_lock(&d->pci_lock); > +#endif > + list_add(&pdev->domain_list, &d->pdev_list); > +#if 0 > + write_unlock(&d->pci_lock); > +#endif > + > + if ( hardware_domain ) > + { > + domain = dev_iommu_domain(dev); > + > + /* > + * Xen may not deassign the device from hwdom before > + * assigning it elsewhere. > + */ This comment leave more question than it really answer. Why can't Xen call reassign? IOW, it feels wrong to reimplement re-assign within assign. At mimimum, this should be consolidated. > + if ( domain && is_hardware_domain(domain->priv->cfg.domain) ) > + { > + ret = arm_smmu_deassign_dev(hardware_domain, devfn, dev); > + if ( ret ) > + return ret; > + } > + } > + > + /* dom_io is used as a sentinel for quarantined devices */ > + if ( d == dom_io ) > + return 0; > + } > +#endif > + > if (!dev_iommu_group(dev)) { > ret = arm_smmu_add_device(dev); > if (ret) > @@ -2833,11 +2953,30 @@ out: > return ret; > } > > -static int arm_smmu_deassign_dev(struct domain *d, struct device *dev) > +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, > + struct device *dev) > { > struct iommu_domain *domain = dev_iommu_domain(dev); > struct arm_smmu_xen_domain *xen_domain; > > +#ifdef CONFIG_HAS_PCI > + if ( dev_is_pci(dev) ) > + { > + struct pci_dev *pdev = dev_to_pci(dev); > + > + printk(XENLOG_INFO "Deassigning device %04x:%02x:%02x.%u from dom%d\n", > + pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + d->domain_id); Same questions as above for the printk. > + > + if ( devfn != pdev->devfn ) > + return 0; I don't understand this check. Are you sanity checking pdev->devfn, and if so shouldn't not this return an error? > + > + /* dom_io is used as a sentinel for quarantined devices */ > + if ( d == dom_io ) > + return 0; > + } > +#endif > + > xen_domain = dom_iommu(d)->arch.priv; > > if (!domain || domain->priv->cfg.domain != d) { > @@ -2865,13 +3004,13 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t, > int ret = 0; > > /* Don't allow remapping on other domain than hwdom */ This comment now ... > - if ( t && !is_hardware_domain(t) ) > + if ( t && !is_hardware_domain(t) && t != dom_io ) ... now doesn't match the check. You are also relaxing for everyone but don't really explain why. > return -EPERM; > > if (t == s) > return 0; > > - ret = arm_smmu_deassign_dev(s, dev); > + ret = arm_smmu_deassign_dev(s, devfn, dev); > if (ret) > return ret; > Cheers,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 11fc1d22ef0a..24d1c0353025 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -131,11 +131,21 @@ enum irqreturn { typedef enum irqreturn irqreturn_t; -/* Device logger functions - * TODO: Handle PCI - */ -#define dev_print(dev, lvl, fmt, ...) \ - printk(lvl "smmu: %s: " fmt, dt_node_full_name(dev_to_dt(dev)), ## __VA_ARGS__) +/* Device logger functions */ +#ifndef CONFIG_HAS_PCI +#define dev_print(dev, lvl, fmt, ...) \ + printk(lvl "smmu: %s: " fmt, dev_name(dev), ## __VA_ARGS__) +#else +#define dev_print(dev, lvl, fmt, ...) ({ \ + if ( !dev_is_pci((dev)) ) \ + printk(lvl "smmu: %s: " fmt, dev_name((dev)), ## __VA_ARGS__); \ + else \ + { \ + struct pci_dev *pdev = dev_to_pci((dev)); \ + printk(lvl "smmu: %pp: " fmt, &pdev->sbdf, ## __VA_ARGS__); \ + } \ +}) +#endif #define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__) #define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__) @@ -187,6 +197,7 @@ static void __iomem *devm_ioremap_resource(struct device *dev, * Xen: PCI functions * TODO: It should be implemented when PCI will be supported */ +#if 0 /* unused */ #define to_pci_dev(dev) (NULL) static inline int pci_for_each_dma_alias(struct pci_dev *pdev, int (*fn) (struct pci_dev *pdev, @@ -196,6 +207,7 @@ static inline int pci_for_each_dma_alias(struct pci_dev *pdev, BUG(); return 0; } +#endif /* Xen: misc */ #define PHYS_MASK_SHIFT PADDR_BITS @@ -631,7 +643,7 @@ struct arm_smmu_master_cfg { for (i = 0; idx = cfg->smendx[i], i < num; ++i) struct arm_smmu_master { - struct device_node *of_node; + struct device *dev; struct rb_node node; struct arm_smmu_master_cfg cfg; }; @@ -723,7 +735,7 @@ arm_smmu_get_fwspec(struct arm_smmu_master_cfg *cfg) { struct arm_smmu_master *master = container_of(cfg, struct arm_smmu_master, cfg); - return dev_iommu_fwspec_get(&master->of_node->dev); + return dev_iommu_fwspec_get(master->dev); } static void parse_driver_options(struct arm_smmu_device *smmu) @@ -756,7 +768,7 @@ static struct device_node *dev_get_dev_node(struct device *dev) } static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu, - struct device_node *dev_node) + struct device *dev) { struct rb_node *node = smmu->masters.rb_node; @@ -765,9 +777,9 @@ static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu, master = container_of(node, struct arm_smmu_master, node); - if (dev_node < master->of_node) + if (dev < master->dev) node = node->rb_left; - else if (dev_node > master->of_node) + else if (dev > master->dev) node = node->rb_right; else return master; @@ -802,9 +814,9 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, = container_of(*new, struct arm_smmu_master, node); parent = *new; - if (master->of_node < this->of_node) + if (master->dev < this->dev) new = &((*new)->rb_left); - else if (master->of_node > this->of_node) + else if (master->dev > this->dev) new = &((*new)->rb_right); else return -EEXIST; @@ -836,28 +848,37 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu, struct arm_smmu_master *master; struct device_node *dev_node = dev_get_dev_node(dev); - master = find_smmu_master(smmu, dev_node); + master = find_smmu_master(smmu, dev); if (master) { dev_err(dev, "rejecting multiple registrations for master device %s\n", - dev_node->name); + dev_node ? dev_node->name : ""); return -EBUSY; } master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL); if (!master) return -ENOMEM; - master->of_node = dev_node; + master->dev = dev; - /* Xen: Let Xen know that the device is protected by an SMMU */ - dt_device_set_protected(dev_node); + if ( !dev_is_pci(dev) ) + { + if ( dt_device_is_protected(dev_node) ) + { + dev_err(dev, "Already added to SMMU\n"); + return -EEXIST; + } + + /* Xen: Let Xen know that the device is protected by an SMMU */ + dt_device_set_protected(dev_node); + } for (i = 0; i < fwspec->num_ids; ++i) { if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && (fwspec->ids[i] >= smmu->num_mapping_groups)) { dev_err(dev, "stream ID for master device %s greater than maximum allowed (%d)\n", - dev_node->name, smmu->num_mapping_groups); + dev_node ? dev_node->name : "", smmu->num_mapping_groups); return -ERANGE; } master->cfg.smendx[i] = INVALID_SMENDX; @@ -872,7 +893,7 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu, struct device_node *dev_node = dev_get_dev_node(dev); int ret; - master = find_smmu_master(smmu, dev_node); + master = find_smmu_master(smmu, dev); if (master == NULL) { dev_err(dev, "No registrations found for master device %s\n", @@ -884,8 +905,9 @@ static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu, if (ret) return ret; - /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */ - dev_node->is_protected = false; + if ( !dev_is_pci(dev) ) + /* Protected by dt_host_lock and dtdevs_lock as caller holds these locks. */ + dev_node->is_protected = false; kfree(master); return 0; @@ -914,6 +936,12 @@ static int register_smmu_master(struct arm_smmu_device *smmu, fwspec); } +/* Forward declaration */ +static int arm_smmu_assign_dev(struct domain *d, u8 devfn, + struct device *dev, u32 flag); +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, + struct device *dev); + /* * The driver which supports generic IOMMU DT bindings must have this * callback implemented. @@ -938,6 +966,22 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev) { struct arm_smmu_device *smmu; struct iommu_fwspec *fwspec; + int ret; + +#ifdef CONFIG_HAS_PCI + if ( dev_is_pci(dev) ) + { + struct pci_dev *pdev = dev_to_pci(dev); + int ret; + + if ( devfn != pdev->devfn ) + return 0; + + ret = iommu_add_pci_sideband_ids(pdev); + if ( ret < 0 ) + iommu_fwspec_free(dev); + } +#endif fwspec = dev_iommu_fwspec_get(dev); if (fwspec == NULL) @@ -947,7 +991,24 @@ static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev) if (smmu == NULL) return -ENXIO; - return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); + ret = arm_smmu_dt_add_device_legacy(smmu, dev, fwspec); + if ( ret ) + return ret; + +#ifdef CONFIG_HAS_PCI + if ( dev_is_pci(dev) ) + { + struct pci_dev *pdev = dev_to_pci(dev); + + /* + * During PHYSDEVOP_pci_device_add, Xen does not assign the + * device, so we must do it here. + */ + ret = arm_smmu_assign_dev(pdev->domain, devfn, dev, 0); + } +#endif + + return ret; } static int arm_smmu_dt_xlate_generic(struct device *dev, @@ -970,11 +1031,10 @@ static struct arm_smmu_device *find_smmu_for_device(struct device *dev) { struct arm_smmu_device *smmu; struct arm_smmu_master *master = NULL; - struct device_node *dev_node = dev_get_dev_node(dev); spin_lock(&arm_smmu_devices_lock); list_for_each_entry(smmu, &arm_smmu_devices, list) { - master = find_smmu_master(smmu, dev_node); + master = find_smmu_master(smmu, dev); if (master) break; } @@ -2066,6 +2126,7 @@ static bool arm_smmu_capable(enum iommu_cap cap) } #endif +#if 0 /* Not used */ static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *data) { *((u16 *)data) = alias; @@ -2076,6 +2137,7 @@ static void __arm_smmu_release_pci_iommudata(void *data) { kfree(data); } +#endif static int arm_smmu_add_device(struct device *dev) { @@ -2083,12 +2145,13 @@ static int arm_smmu_add_device(struct device *dev) struct arm_smmu_master_cfg *cfg; struct iommu_group *group; void (*releasefn)(void *data) = NULL; - int ret; smmu = find_smmu_for_device(dev); if (!smmu) return -ENODEV; + /* There is no need to distinguish here, thanks to PCI-IOMMU DT bindings */ +#if 0 if (dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(dev); struct iommu_fwspec *fwspec; @@ -2113,10 +2176,12 @@ static int arm_smmu_add_device(struct device *dev) &fwspec->ids[0]); releasefn = __arm_smmu_release_pci_iommudata; cfg->smmu = smmu; - } else { + } else +#endif + { struct arm_smmu_master *master; - master = find_smmu_master(smmu, dev->of_node); + master = find_smmu_master(smmu, dev); if (!master) { return -ENODEV; } @@ -2784,6 +2849,61 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn, return -ENOMEM; } +#ifdef CONFIG_HAS_PCI + if ( dev_is_pci(dev) && !is_hardware_domain(d) ) + { + struct pci_dev *pdev = dev_to_pci(dev); + + printk(XENLOG_INFO "Assigning device %04x:%02x:%02x.%u to dom%d\n", + pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn), + d->domain_id); + + if ( devfn != pdev->devfn || pdev->domain == d ) + return 0; + + ASSERT(pcidevs_locked()); + + /* TODO: acquire pci_lock */ +#if 0 + write_lock(&pdev->domain->pci_lock); +#endif + list_del(&pdev->domain_list); +#if 0 + write_unlock(&pdev->domain->pci_lock); +#endif + + pdev->domain = d; + +#if 0 + write_lock(&d->pci_lock); +#endif + list_add(&pdev->domain_list, &d->pdev_list); +#if 0 + write_unlock(&d->pci_lock); +#endif + + if ( hardware_domain ) + { + domain = dev_iommu_domain(dev); + + /* + * Xen may not deassign the device from hwdom before + * assigning it elsewhere. + */ + if ( domain && is_hardware_domain(domain->priv->cfg.domain) ) + { + ret = arm_smmu_deassign_dev(hardware_domain, devfn, dev); + if ( ret ) + return ret; + } + } + + /* dom_io is used as a sentinel for quarantined devices */ + if ( d == dom_io ) + return 0; + } +#endif + if (!dev_iommu_group(dev)) { ret = arm_smmu_add_device(dev); if (ret) @@ -2833,11 +2953,30 @@ out: return ret; } -static int arm_smmu_deassign_dev(struct domain *d, struct device *dev) +static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, + struct device *dev) { struct iommu_domain *domain = dev_iommu_domain(dev); struct arm_smmu_xen_domain *xen_domain; +#ifdef CONFIG_HAS_PCI + if ( dev_is_pci(dev) ) + { + struct pci_dev *pdev = dev_to_pci(dev); + + printk(XENLOG_INFO "Deassigning device %04x:%02x:%02x.%u from dom%d\n", + pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn), + d->domain_id); + + if ( devfn != pdev->devfn ) + return 0; + + /* dom_io is used as a sentinel for quarantined devices */ + if ( d == dom_io ) + return 0; + } +#endif + xen_domain = dom_iommu(d)->arch.priv; if (!domain || domain->priv->cfg.domain != d) { @@ -2865,13 +3004,13 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if ( t && !is_hardware_domain(t) ) + if ( t && !is_hardware_domain(t) && t != dom_io ) return -EPERM; if (t == s) return 0; - ret = arm_smmu_deassign_dev(s, dev); + ret = arm_smmu_deassign_dev(s, devfn, dev); if (ret) return ret;