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
>>
>> This flag will be re-used for PCI devices by the subsequent
>> patches.
>>
>> Signed-off-by: Oleksandr Tyshchenko
>> Signed-off-by: Stewart Hildebrand
>> ---
>> 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
>> +
>> 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 --
Re: [PATCH v3 1/6] xen/arm: Move is_protected flag to struct device
Hi Stewart,
On 18/05/2023 23:06, Stewart Hildebrand wrote:
>
>
> From: Oleksandr Tyshchenko
>
> This flag will be re-used for PCI devices by the subsequent
> patches.
>
> Signed-off-by: Oleksandr Tyshchenko
> Signed-off-by: Stewart Hildebrand
> ---
> 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
> +
> 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
[PATCH v3 1/6] xen/arm: Move is_protected flag to struct device
From: Oleksandr Tyshchenko
This flag will be re-used for PCI devices by the subsequent
patches.
Signed-off-by: Oleksandr Tyshchenko
Signed-off-by: Stewart Hildebrand
---
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
+
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 */
};
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;
-}
-
/* 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)
