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, or else why would
the bits be called "reserved" rather than e.g. "read-as-zero"?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
