| Message ID | 20230607030220.22698-3-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
Hi Stewart, On 07/06/2023 04:02, Stewart Hildebrand wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > This flag will be re-used for PCI devices by the subsequent > patches. I was expecting that we would only do PCI passthrough on boards where all the PCI devices are behind an IOMMU. So it would be a all or nothing and therefore is_protected would not be used. Do you have an example where this wouldn't be the case? Regardless that, it would be good to spell out that you also rename helpers. But see below. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > --- > v3->v4: > * move is_protected flag within struct device to reduce padding > * re-add device_is_protected checks in add_device hooks in smmu-v3.c/ipmmu-vmsa.c > * split mmu-masters check into separate patch > > v2->v3: > * no change > > v1->v2: > * no change > > downstream->v1: > * rebase > * s/dev_node->is_protected/dev_node->dev.is_protected/ in smmu.c > * s/dt_device_set_protected(dev_to_dt(dev))/device_set_protected(dev)/ in smmu-v3.c > * remove redundant device_is_protected checks in smmu-v3.c/ipmmu-vmsa.c > > (cherry picked from commit 59753aac77528a584d3950936b853ebf264b68e7 from > the downstream branch poc/pci-passthrough from > https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git) > --- > xen/arch/arm/domain_build.c | 4 ++-- > xen/arch/arm/include/asm/device.h | 14 ++++++++++++++ > xen/common/device_tree.c | 2 +- > xen/drivers/passthrough/arm/ipmmu-vmsa.c | 4 ++-- > xen/drivers/passthrough/arm/smmu-v3.c | 5 +++-- > xen/drivers/passthrough/arm/smmu.c | 2 +- > xen/drivers/passthrough/device_tree.c | 8 ++++---- > xen/include/xen/device_tree.h | 13 ------------- > 8 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 3f4558ade67f..b229bfaae712 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2524,7 +2524,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, > return res; > } > > - if ( dt_device_is_protected(dev) ) > + if ( device_is_protected(dt_to_dev(dev)) ) I would actually prefer if we keep dt_device_is_protected() and call device_is_protected(dt_to_dev(...)) within it. > { > dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); > res = iommu_assign_dt_device(d, dev); > @@ -3024,7 +3024,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, > return res; > > /* If xen_force, we allow assignment of devices without IOMMU protection. */ > - if ( xen_force && !dt_device_is_protected(node) ) > + if ( xen_force && !device_is_protected(dt_to_dev(node)) ) > return 0; > > return iommu_assign_dt_device(kinfo->d, node); > diff --git a/xen/arch/arm/include/asm/device.h b/xen/arch/arm/include/asm/device.h > index b5d451e08776..8ac807482737 100644 > --- a/xen/arch/arm/include/asm/device.h > +++ b/xen/arch/arm/include/asm/device.h > @@ -1,6 +1,8 @@ > #ifndef __ASM_ARM_DEVICE_H > #define __ASM_ARM_DEVICE_H > > +#include <xen/types.h> > + > enum device_type > { > DEV_DT, > @@ -15,6 +17,8 @@ struct dev_archdata { > struct device > { > enum device_type type; > + bool is_protected; /* Shows that device is protected by IOMMU */ > + uint8_t _pad[3]; AFAIK, a compiler would be allowed to use 8-byte for the enum. So the padding would increase the size of the structure. Therefore, I don't think you want to add an explicit padding here. Cheers,
On 6/29/23 18:22, Julien Grall wrote: > Hi Stewart, > > On 07/06/2023 04:02, Stewart Hildebrand wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> This flag will be re-used for PCI devices by the subsequent >> patches. > > I was expecting that we would only do PCI passthrough on boards where > all the PCI devices are behind an IOMMU. So it would be a all or nothing > and therefore is_protected would not be used. That makes sense. > Do you have an example where this wouldn't be the case? No. I'll drop this patch for v5 of the series.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3f4558ade67f..b229bfaae712 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2524,7 +2524,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, return res; } - if ( dt_device_is_protected(dev) ) + if ( device_is_protected(dt_to_dev(dev)) ) { dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); res = iommu_assign_dt_device(d, dev); @@ -3024,7 +3024,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, return res; /* If xen_force, we allow assignment of devices without IOMMU protection. */ - if ( xen_force && !dt_device_is_protected(node) ) + if ( xen_force && !device_is_protected(dt_to_dev(node)) ) return 0; return iommu_assign_dt_device(kinfo->d, node); diff --git a/xen/arch/arm/include/asm/device.h b/xen/arch/arm/include/asm/device.h index b5d451e08776..8ac807482737 100644 --- a/xen/arch/arm/include/asm/device.h +++ b/xen/arch/arm/include/asm/device.h @@ -1,6 +1,8 @@ #ifndef __ASM_ARM_DEVICE_H #define __ASM_ARM_DEVICE_H +#include <xen/types.h> + enum device_type { DEV_DT, @@ -15,6 +17,8 @@ struct dev_archdata { struct device { enum device_type type; + bool is_protected; /* Shows that device is protected by IOMMU */ + uint8_t _pad[3]; #ifdef CONFIG_HAS_DEVICE_TREE struct dt_device_node *of_node; /* Used by drivers imported from Linux */ #endif @@ -94,6 +98,16 @@ int device_init(struct dt_device_node *dev, enum device_class class, */ enum device_class device_get_class(const struct dt_device_node *dev); +static inline void device_set_protected(struct device *device) +{ + device->is_protected = true; +} + +static inline bool device_is_protected(const struct device *device) +{ + return device->is_protected; +} + #define DT_DEVICE_START(_name, _namestr, _class) \ static const struct device_desc __dev_desc_##_name __used \ __section(".dev.info") = { \ diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 8da105291184..7444da3e0aa5 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1910,7 +1910,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, /* By default dom0 owns the device */ np->used_by = 0; /* By default the device is not protected */ - np->is_protected = false; + np->dev.is_protected = false; INIT_LIST_HEAD(&np->domain_list); if ( new_format ) diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c index 611d9eeba5c3..a71fd76d89a3 100644 --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c @@ -1288,14 +1288,14 @@ static int ipmmu_add_device(u8 devfn, struct device *dev) if ( !to_ipmmu(dev) ) return -ENODEV; - if ( dt_device_is_protected(dev_to_dt(dev)) ) + if ( device_is_protected(dev) ) { dev_err(dev, "Already added to IPMMU\n"); return -EEXIST; } /* Let Xen know that the master device is protected by an IOMMU. */ - dt_device_set_protected(dev_to_dt(dev)); + device_set_protected(dev); dev_info(dev, "Added master device (IPMMU %s micro-TLBs %u)\n", dev_name(fwspec->iommu_dev), fwspec->num_ids); diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c index 720aa69ff23e..8842db1ec07e 100644 --- a/xen/drivers/passthrough/arm/smmu-v3.c +++ b/xen/drivers/passthrough/arm/smmu-v3.c @@ -1521,13 +1521,14 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev) */ arm_smmu_enable_pasid(master); - if (dt_device_is_protected(dev_to_dt(dev))) { + if ( device_is_protected(dev) ) + { dev_err(dev, "Already added to SMMUv3\n"); return -EEXIST; } /* Let Xen know that the master device is protected by an IOMMU. */ - dt_device_set_protected(dev_to_dt(dev)); + device_set_protected(dev); dev_info(dev, "Added master device (SMMUv3 %s StreamIds %u)\n", dev_name(fwspec->iommu_dev), fwspec->num_ids); diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index c37fa9af1366..d874417958b5 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -837,7 +837,7 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu, master->of_node = dev_node; /* Xen: Let Xen know that the device is protected by an SMMU */ - dt_device_set_protected(dev_node); + device_set_protected(dev); for (i = 0; i < fwspec->num_ids; ++i) { if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index d9b63da7260a..c60e78eaf556 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -34,7 +34,7 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev) if ( !is_iommu_enabled(d) ) return -EINVAL; - if ( !dt_device_is_protected(dev) ) + if ( !device_is_protected(dt_to_dev(dev)) ) return -EINVAL; spin_lock(&dtdevs_lock); @@ -65,7 +65,7 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev) if ( !is_iommu_enabled(d) ) return -EINVAL; - if ( !dt_device_is_protected(dev) ) + if ( !device_is_protected(dt_to_dev(dev)) ) return -EINVAL; spin_lock(&dtdevs_lock); @@ -87,7 +87,7 @@ static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev) { bool_t assigned = 0; - if ( !dt_device_is_protected(dev) ) + if ( !device_is_protected(dt_to_dev(dev)) ) return 0; spin_lock(&dtdevs_lock); @@ -146,7 +146,7 @@ int iommu_add_dt_device(struct dt_device_node *np) * and iommus property, there is no need to register it again. In this case * simply return success early. */ - if ( dt_device_is_protected(np) ) + if ( device_is_protected(dev) ) return 0; if ( dev_iommu_fwspec_get(dev) ) diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index c2eada748915..c2f315140560 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -90,9 +90,6 @@ struct dt_device_node { struct dt_device_node *next; /* TODO: Remove it. Only use to know the last children */ struct dt_device_node *allnext; - /* IOMMU specific fields */ - bool is_protected; - /* HACK: Remove this if there is a need of space */ bool_t static_evtchn_created; @@ -329,16 +326,6 @@ static inline domid_t dt_device_used_by(const struct dt_device_node *device) return device->used_by; } -static inline void dt_device_set_protected(struct dt_device_node *device) -{ - device->is_protected = true; -} - -static inline bool dt_device_is_protected(const struct dt_device_node *device) -{ - return device->is_protected; -} - static inline bool_t dt_property_name_is_equal(const struct dt_property *pp, const char *name) {