Re: [PATCH] AMD/IOMMU: Improve register_iommu_exclusion_range()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2024 20:31, Andrew Cooper wrote:
>  * Use 64bit accesses instead of 32bit accesses
>  * Simplify the constant names
>  * Pull base into a local variable to avoid it being reloaded because of the
>    memory clobber in writeq().
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> RFC.  This is my proposed way of cleaning up the whole IOMMU file.  The
> diffstat speaks for itself.

Absolutely.

> I've finally found the bit in the AMD IOMMU spec which says 64bit accesses are
> permitted:
> 
>   3.4 IOMMU MMIO Registers:
> 
>   Software access to IOMMU registers may not be larger than 64 bits. Accesses
>   must be aligned to the size of the access and the size in bytes must be a
>   power of two. Software may use accesses as small as one byte.

I take it that the use of 32-bit writes was because of the past need
also work in a 32-bit hypervisor, not because of perceived restrictions
by the spec.

> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> @@ -338,22 +338,10 @@ union amd_iommu_control {
>  };
>  
>  /* Exclusion Register */
> -#define IOMMU_EXCLUSION_BASE_LOW_OFFSET              0x20
> -#define IOMMU_EXCLUSION_BASE_HIGH_OFFSET     0x24
> -#define IOMMU_EXCLUSION_LIMIT_LOW_OFFSET     0x28
> -#define IOMMU_EXCLUSION_LIMIT_HIGH_OFFSET    0x2C
> -#define IOMMU_EXCLUSION_BASE_LOW_MASK                0xFFFFF000U
> -#define IOMMU_EXCLUSION_BASE_LOW_SHIFT               12
> -#define IOMMU_EXCLUSION_BASE_HIGH_MASK               0xFFFFFFFFU
> -#define IOMMU_EXCLUSION_BASE_HIGH_SHIFT              0
> -#define IOMMU_EXCLUSION_RANGE_ENABLE_MASK    0x00000001U
> -#define IOMMU_EXCLUSION_RANGE_ENABLE_SHIFT   0
> -#define IOMMU_EXCLUSION_ALLOW_ALL_MASK               0x00000002U
> -#define IOMMU_EXCLUSION_ALLOW_ALL_SHIFT              1
> -#define IOMMU_EXCLUSION_LIMIT_LOW_MASK               0xFFFFF000U
> -#define IOMMU_EXCLUSION_LIMIT_LOW_SHIFT              12
> -#define IOMMU_EXCLUSION_LIMIT_HIGH_MASK              0xFFFFFFFFU
> -#define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT     0
> +#define IOMMU_MMIO_EXCLUSION_BASE           0x20
> +#define   EXCLUSION_RANGE_ENABLE            (1 << 0)
> +#define   EXCLUSION_ALLOW_ALL               (1 << 1)
> +#define IOMMU_MMIO_EXCLUSION_LIMIT          0x28

Just one question here: Previously you suggested we switch to bitfields
for anything like this, and we've already done so with e.g.
union amd_iommu_control and union amd_iommu_ext_features. IOW I wonder
if we wouldn't better strive to be consistent in this regard. Or if not,
what the (written or unwritten) guidelines are when to use which
approach.

Jan