Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.02.2020 16:19, Andrew Cooper wrote:
> On 10/02/2020 15:02, Jan Beulich wrote:
>> On 10.02.2020 15:55, Andrew Cooper wrote:
>>> On 05/02/2020 10:36, Jan Beulich wrote:
>>>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>>>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>>>>> @@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu 
>>>>> *iommu, u32 cmd[])
>>>>>  {
>>>>>      uint32_t tail, head;
>>>>>  
>>>>> -    tail = iommu->cmd_buffer.tail;
>>>>> -    if ( ++tail == iommu->cmd_buffer.entries )
>>>>> +    tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
>>>>> +    if ( tail == iommu->cmd_buffer.size )
>>>>>          tail = 0;
>>>>>  
>>>>> -    head = iommu_get_rb_pointer(readl(iommu->mmio_base +
>>>>> -                                      IOMMU_CMD_BUFFER_HEAD_OFFSET));
>>>>> +    head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>>>>      if ( head != tail )
>>>> Surely you want to mask off reserved (or more generally
>>>> unrelated) bits, before consuming the value for the purpose
>>>> here (and elsewhere below)?
>>> Reserved bits are defined in the IOMMU spec to be read-only zero.
>>>
>>> It is also undefined behaviour for this value to ever be outside of the
>>> size configured for command buffer, so using the value like this is spec
>>> compliant.
>>>
>>> As for actually masking the values, that breaks the optimisers ability
>>> to construct commands in the command ring.  This aspect can be worked
>>> around with other code changes, but I also think it is implausible that
>>> the remaining reserved bits here are going to sprout incompatible future
>>> uses.
>> Implausible - perhaps. But impossible - no. There could be a simple
>> flag bit appearing somewhere in these registers. I simply don't it
>> is a good idea to set a precedent of not honoring reserved bit as
>> being reserved. The spec saying "read-only zero" can only be viewed
>> as correct for the current version of the spec,
> 
> Its perfectly easy to do forward compatible changes with a spec written
> in this way.
> 
> It means that new behaviours have to be opted in to, and this is how the
> AMD IOMMU spec has evolved.  Notice how every new feature declares "this
> bit is reserved unless $OTHER_THING is enabled."
> 
> It is also a very sane way of doing forward compatibility, from
> software's point of view.

Yes. But does the IOMMU spec spell out that it'll follow this
in the future?

>> or else why would
>> the bits be called "reserved" rather than e.g. "read-as-zero"?
> 
> Read Table 1, but it also ought to be obvious.  "Reserved", "Resv" and
> "Res" are all shorter to write than "read-as-zero", especially in the
> diagrams of a few individual bits in a register.

There's also the common acronym "raz", which is as short. That table
in particular says nothing about future uses of currently reserved
bits. Just take the Extended Feature Register as a reference: How
would new features be advertised (in currently reserved bits) if use
of those bits was to be qualified by yet some other feature bit(s).
Past growth of the set of used bits also hasn't followed a pattern
you seem to suggest.

Don't get me wrong - I agree it's unlikely for these bits to gain
a meaning that would conflict with a more relaxed use like you do
suggest. But I don't think better code generation should be an
argument against having code written as compatibly as possible.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel