Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in queue_iommu_command()

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 24 September 2018 11:55
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Suravee
> Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods
> <brian.woods@amd.com>
> Subject: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> queue_iommu_command()
> 
> In practice, this allows the compiler to replace the loop with a pair of
> movs.
> 
> No functional change.

Well there is a potential functional change...

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> ---
>  xen/drivers/passthrough/amd/iommu_cmd.c      | 12 ++++--------
>  xen/include/asm-x86/hvm/svm/amd-iommu-defs.h |  1 -
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c
> b/xen/drivers/passthrough/amd/iommu_cmd.c
> index 08247fa..c6c0b4f 100644
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -24,8 +24,7 @@
> 
>  static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>  {
> -    u32 tail, head, *cmd_buffer;
> -    int i;
> +    uint32_t tail, head;
> 
>      tail = iommu->cmd_buffer.tail;
>      if ( ++tail == iommu->cmd_buffer.entries )
> @@ -35,12 +34,9 @@ static int queue_iommu_command(struct amd_iommu *iommu,
> u32 cmd[])
>                                        IOMMU_CMD_BUFFER_HEAD_OFFSET));
>      if ( head != tail )
>      {
> -        cmd_buffer = (u32 *)(iommu->cmd_buffer.buffer +
> -                             (iommu->cmd_buffer.tail *
> -                             IOMMU_CMD_BUFFER_ENTRY_SIZE));
> -
> -        for ( i = 0; i < IOMMU_CMD_BUFFER_U32_PER_ENTRY; i++ )
> -            cmd_buffer[i] = cmd[i];
> +        memcpy(iommu->cmd_buffer.buffer +
> +               (iommu->cmd_buffer.tail * IOMMU_CMD_BUFFER_ENTRY_SIZE),
> +               cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE);

...since the built-in memcpy may not guarantee to the copy in 4 byte chunks in ascending order. As long as that change poses no danger (and seeing as the code contains no barrer-ing I'd assume that to be the case) then this change looks fine to me (with the 'no functional change' comment removed).

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
>          iommu->cmd_buffer.tail = tail;
>          return 1;
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> index c479f0b..1f19cd3 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -187,7 +187,6 @@
> 
>  #define IOMMU_CMD_BUFFER_ENTRY_SIZE			16
>  #define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE	8
> -#define IOMMU_CMD_BUFFER_U32_PER_ENTRY 	(IOMMU_CMD_BUFFER_ENTRY_SIZE /
> 4)
> 
>  #define IOMMU_CMD_OPCODE_MASK			0xF0000000
>  #define IOMMU_CMD_OPCODE_SHIFT			28
> --
> 2.1.4

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel