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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
Date: Mon, 10 Feb 2020 14:55:56 +0000	[thread overview]
Message-ID: <cf596ea9-f9dd-b938-9666-34165edb8a0c@citrix.com> (raw)
In-Reply-To: <76961409-b553-0ccc-6cd1-33ccf5cc75a0@suse.com>

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(&tail, 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