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

[Jan Beulich <jbeulich@xxxxxxxx>]

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(&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? (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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel