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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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