Re: [PATCH v3 1/6] xen/arm: Move is_protected flag to struct device

[Michal Orzel <michal.orzel@xxxxxxx>]

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?

>  };
> 
>  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