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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/06/2024 8:45 am, Jan Beulich wrote:
> 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.

I recall having problems getting writeq() acked in the past, even after
we'd dropped 32bit.

But this is the first time that I've positively found anything in the
spec saying that 64bit accesses are ok.

>
>> --- 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.

We've got two very different kinds of things here.

The device table/etc are in-memory WB datastructure which we're
interpreting and editing routinely.  It's completely full of bits and
small fields, and letting the compiler do the hard work there is
preferable; certainly in terms of legibility.

This example is an MMIO register (in a bar on the IOMMU PCI device, even
though we find the address in the IVRS).  We set it up once at boot and
don't touch it afterwards.

So while we could make a struct for it, we'd still need to get it into a
form that we can writeq(), and that's more code than the single case
were we need to put two metadata bits into an address.

~Andrew