Re: [PATCH v5 12/14] xen/arm: p2m: Clean cache PT when the IOMMU doesn't support coherent walk

["Jan Beulich" <JBeulich@suse.com>]

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -344,6 +344,17 @@ void iommu_crash_shutdown(void)
>      iommu_enabled = iommu_intremap = 0;
>  }
>  
> +bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
> +{
> +    const struct iommu_ops *ops = domain_hvm_iommu(d)->platform_ops;
> +    uint32_t features = 0;

Please here and further down - don't use fixed width type unless
you really need to.

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -67,6 +67,14 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
>                     unsigned int flags);
>  int iommu_unmap_page(struct domain *d, unsigned long gfn);
>  
> +enum iommu_feature
> +{
> +    IOMMU_FEAT_COHERENT_WALK = 1,

Why 1? Enumerations are defined to start at zero, and starting at
zero is what you really want here. Don't specify a value at all.

> @@ -139,6 +147,7 @@ struct iommu_ops {
>      void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
>      void (*iotlb_flush_all)(struct domain *d);
>      void (*dump_p2m_table)(struct domain *d);
> +    uint32_t (*features)(struct domain *d);

I think I said this on an earlier round already - for future extensibility
this should return "const unsigned long *", and get accessed by the
wrapper function using test_bit(). Or even better without an accessor
function at all, just directly having a "const unsigned long *" field here.
Unless of course the backend implementation - which isn't part of this
patch - would have difficulty setting up a suitable bitfield during (early)
initialization.

Jan