Re: [PATCH] iommu/arm-smmu-v3: Make CD programming use arm_smmu_write_entry_step()

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, Jan 06, 2024 at 04:36:15PM +0800, Michael Shavit wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> CD table entries and STE's have the same essential programming sequence,
> just with different types and sizes.
> 
> Have arm_smmu_write_ctx_desc() generate a target CD and call
> arm_smmu_write_entry_step() to do the programming. Due to the way the
> target CD is generated by modifying the existing CD this alone is not
> enough for the CD callers to be freed of the ordering requirements.
> 
> The following patches will make the rest of the CD flow mirror the STE
> flow with precise CD contents generated in all cases.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 90 +++++++++++++++------
>  1 file changed, 67 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index c9559c4075b4b..5a598500b5c6d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -23,6 +23,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
> +#include <linux/minmax.h>
>  #include <linux/pci.h>
>  #include <linux/pci-ats.h>
>  #include <linux/platform_device.h>
> @@ -994,7 +995,9 @@ static bool entry_set(const struct arm_smmu_entry_writer_ops *ops,
>  	return changed;
>  }
>  
> -#define NUM_ENTRY_QWORDS (sizeof_field(struct arm_smmu_ste, data) / sizeof(u64))
> +#define NUM_ENTRY_QWORDS (max(sizeof_field(struct arm_smmu_ste, data), \
> +			     sizeof_field(struct arm_smmu_cd, data)) \
> +			     / sizeof(u64))

So, the reason I wrote it the other way, with the enum, is because
this isn't a constexpr in Linux. max() has some complex implementation
hidden inside.

An obvious consequence of this is you can't do something like:

static unsigned int foo[NUM_ENTRY_QWORDS]; // error: statement expression not allowed at file scope

Now, the question is what does the compiler do with an automatic stack
variable when it is not a constexpr but with optimization can be made
constant. Particularly will someone's checker (sparse perhaps?)
complain that this is a forbidden "variable length array" alloca?

At least latest gcc and clang are able to avoid the variable length
array, but I wonder if this is asking for trouble...

Jason