Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
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
Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
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
Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
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.
>
>> @@ -45,13 +43,11 @@ static int queue_iommu_command(struct amd_iommu *iommu,
>> u32 cmd[])
>>
>> static void commit_iommu_command_buffer(struct amd_iommu *iommu)
>> {
>> -u32 tail = 0;
>> -
>> -iommu_set_rb_pointer(, iommu->cmd_buffer.tail);
>> -writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET);
>> +writel(iommu->cmd_buffer.tail,
>> + iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
> I guess not preserving the reserved bits isn't a problem
> right now, but is doing so a good idea in general?
As above - there are by definition no bits to preserve.
>> @@ -316,22 +316,20 @@ static int iommu_read_log(struct amd_iommu *iommu,
>> IOMMU_PPR_LOG_HEAD_OFFSET;
>>
>> tail = readl(iommu->mmio_base + tail_offest);
>> -tail = iommu_get_rb_pointer(tail);
>>
>> while ( tail != log->head )
>> {
>> /* read event log entry */
>> -entry = (u32 *)(log->buffer + log->head * entry_size);
>> +entry = (u32 *)(log->buffer + log->head);
> Would you mind dropping the pointless cast here at the same time?
Can do.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
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)?
> @@ -45,13 +43,11 @@ static int queue_iommu_command(struct amd_iommu *iommu,
> u32 cmd[])
>
> static void commit_iommu_command_buffer(struct amd_iommu *iommu)
> {
> -u32 tail = 0;
> -
> -iommu_set_rb_pointer(, iommu->cmd_buffer.tail);
> -writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET);
> +writel(iommu->cmd_buffer.tail,
> + iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
I guess not preserving the reserved bits isn't a problem
right now, but is doing so a good idea in general? (I notice
the head point updating when processing the logs did so
before, so perhaps it's indeed acceptable.)
> @@ -316,22 +316,20 @@ static int iommu_read_log(struct amd_iommu *iommu,
> IOMMU_PPR_LOG_HEAD_OFFSET;
>
> tail = readl(iommu->mmio_base + tail_offest);
> -tail = iommu_get_rb_pointer(tail);
>
> while ( tail != log->head )
> {
> /* read event log entry */
> -entry = (u32 *)(log->buffer + log->head * entry_size);
> +entry = (u32 *)(log->buffer + log->head);
Would you mind dropping the pointless cast here at the same time?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
