Re: [PATCH v4 2/7] xen/arm: Move is_protected flag to struct device

[Julien Grall <julien@xxxxxxx>]

Hi Stewart,

On 07/06/2023 04:02, Stewart Hildebrand wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> This flag will be re-used for PCI devices by the subsequent
> patches.
I was expecting that we would only do PCI passthrough on boards where 
all the PCI devices are behind an IOMMU. So it would be a all or nothing 
and therefore is_protected would not be used.
Do you have an example where this wouldn't be the case?

Regardless that, it would be good to spell out that you also rename 
helpers. But see below.
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> ---
> v3->v4:
> * move is_protected flag within struct device to reduce padding
> * re-add device_is_protected checks in add_device hooks in 
> smmu-v3.c/ipmmu-vmsa.c
> * split mmu-masters check into separate patch
>
> 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        | 14 ++++++++++++++
>   xen/common/device_tree.c                 |  2 +-
>   xen/drivers/passthrough/arm/ipmmu-vmsa.c |  4 ++--
>   xen/drivers/passthrough/arm/smmu-v3.c    |  5 +++--
>   xen/drivers/passthrough/arm/smmu.c       |  2 +-
>   xen/drivers/passthrough/device_tree.c    |  8 ++++----
>   xen/include/xen/device_tree.h            | 13 -------------
>   8 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3f4558ade67f..b229bfaae712 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2524,7 +2524,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)) )
I would actually prefer if we keep dt_device_is_protected() and call 
device_is_protected(dt_to_dev(...)) within it.
>           {
>               dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
>               res = iommu_assign_dt_device(d, dev);
> @@ -3024,7 +3024,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..8ac807482737 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,
> @@ -15,6 +17,8 @@ struct dev_archdata {
>   struct device
>   {
>       enum device_type type;
> +    bool is_protected; /* Shows that device is protected by IOMMU */
> +    uint8_t _pad[3];
AFAIK, a compiler would be allowed to use 8-byte for the enum. So the 
padding would increase the size of the structure.
Therefore, I don't think you want to add an explicit padding here.

Cheers,

--
Julien Grall