Hi Stewart,
On 18/05/2023 23:06, Stewart Hildebrand wrote:
>
>
> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
>
> This flag will be re-used for PCI devices by the subsequent
> patches.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com>
> ---
> 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?
> };
>
> 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.
> -
> /* 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.
> +
> /*
> * 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
