[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/6] xen/arm: Move is_protected flag to struct device
On 5/22/23 10:28, Michal Orzel wrote: > Hi Stewart, > > On 18/05/2023 23:06, Stewart Hildebrand wrote: >> >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx> >> >> This flag will be re-used for PCI devices by the subsequent >> patches. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx> >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx> >> --- >> 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 | 13 +++++++++++++ >> xen/common/device_tree.c | 2 +- >> xen/drivers/passthrough/arm/ipmmu-vmsa.c | 8 +------- >> xen/drivers/passthrough/arm/smmu-v3.c | 7 +------ >> xen/drivers/passthrough/arm/smmu.c | 2 +- >> xen/drivers/passthrough/device_tree.c | 15 +++++++++------ >> xen/include/xen/device_tree.h | 13 ------------- >> 8 files changed, 28 insertions(+), 36 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 71f307a572e9..d228da641367 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -2503,7 +2503,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); >> @@ -3003,7 +3003,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..086dde13eb6b 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, >> @@ -20,6 +22,7 @@ struct device >> #endif >> struct dev_archdata archdata; >> struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */ >> + bool is_protected; /* Shows that device is protected by IOMMU */ > This will add 7 bytes of padding on arm64. Did you check if there is a hole > you can reuse? Good point, I'll move it to save space (on arm64). With is_protected located in the hole after the enum, there will be no change in sizeof(struct device) on arm64. BTW, how do we feel about explicit padding? struct device { enum device_type type; + bool is_protected; /* Shows that device is protected by IOMMU */ + uint8_t _pad[3]; /* unused padding - can be removed to make room for future data */ #ifdef CONFIG_HAS_DEVICE_TREE struct dt_device_node *of_node; /* Used by drivers imported from Linux */ #endif struct dev_archdata archdata; struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */ }; >> }; >> >> typedef struct device device_t; >> @@ -94,6 +97,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 6c9712ab7bda..1d5d7cb5f01b 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -1874,7 +1874,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 091f09b21752..039212a3a990 100644 >> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c >> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c >> @@ -1288,14 +1288,8 @@ 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)) ) >> - { >> - dev_err(dev, "Already added to IPMMU\n"); >> - return -EEXIST; >> - } > This removal and in smmuv3 needs to be explained in the commit msg. I think the rationale for the removal is no longer valid. In v1 of this series, we had: pci_add_device() pci.c iommu_add_device() pci.c iommu_add_dt_pci_device() device_tree.c if ( device_is_protected(dev) ) return 0; ops->add_device() device_set_protected() So in v1 of this series, the device_is_protected checks inside the add_device hooks were redundant because there was already such a check before calling the add_device hook. Now in v3 we don't have a device_is_protected check before calling ops->add_device: pci_add_device() pci.c iommu_add_device() pci.c ops->add_device() device_set_protected() So in v3 of this series, the checks in smmu-v3.c/ipmmu-vmsa.c aren't redundant anymore. I'll re-add the checks in v4. >> - >> /* 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 4ca55d400a7b..f5910e79922f 100644 >> --- a/xen/drivers/passthrough/arm/smmu-v3.c >> +++ b/xen/drivers/passthrough/arm/smmu-v3.c >> @@ -1521,13 +1521,8 @@ 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))) { >> - 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 0a514821b336..5b6024d579a8 100644 >> --- a/xen/drivers/passthrough/arm/smmu.c >> +++ b/xen/drivers/passthrough/arm/smmu.c >> @@ -838,7 +838,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 1c32d7b50cce..b5bd13393b56 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); >> @@ -141,12 +141,15 @@ int iommu_add_dt_device(struct dt_device_node *np) >> return -EINVAL; >> >> /* >> - * The device may already have been registered. As there is no harm in >> - * it just return success early. >> + * This is needed in case a device has both the iommus property and >> + * also appears in the mmu-masters list. >> */ >> - if ( dev_iommu_fwspec_get(dev) ) >> + if ( device_is_protected(dev) ) >> return 0; >> >> + if ( dev_iommu_fwspec_get(dev) ) >> + return -EEXIST; > This needs to be explained, because before this change, we were returning 0. Since this change is not related to moving the is_protected flag, I prefer to move it to a separate patch (with an appropriate explanation). Then we will keep this patch focused on actually moving the is_protected flag as the title suggests. >> + >> /* >> * According to the Documentation/devicetree/bindings/iommu/iommu.txt >> * from Linux. >> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h >> index 19a74909cece..c1e4751a581f 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; > Not your fault (and I don't know if you should do anything about it) but this > single field now causes > the whole structure to be 8 bytes larger than it could be on arm64. > > ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |