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 <[email protected]>
>>> ---
>>> CC: Jan Beulich <[email protected]>
>>> CC: Roger Pau Monné <[email protected]>
>>>
>>> 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
