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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2024 18:22, Andrew Cooper wrote:
> 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.

That's odd, as per my subsequent reply.

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

And it was specifically not the DTE I used as example in my reply, ...

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

... but other MMIO registers.

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

See those other examples, which are usable with writeq() by way of their
"raw" fields.

Jan