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.
> 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.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
