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

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

[Andrew Cooper]

In practice, this allows the compiler to replace the loop with a pair of movs.

No 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);
 
         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

[PATCH 2/2] x86/SVM-IOMMU: Drop get_field_from_byte()

[Andrew Cooper]

It is MASK_EXTR() in disguise, but less flexible.

No 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_map.c       | 2 +-
 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 5 -----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 70b4345..ded2cc7 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -220,7 +220,7 @@ void __init iommu_dte_add_device_entry(u32 *dte, struct ivrs_mappings *ivrs_dev)
     dte[7] = dte[6] = dte[4] = dte[2] = dte[1] = dte[0] = 0;
 
     flags = ivrs_dev->device_flags;
-    sys_mgt = get_field_from_byte(flags, ACPI_IVHD_SYSTEM_MGMT);
+    sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
     dev_ex = ivrs_dev->dte_allow_exclusion;
 
     flags &= mask;
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 99bc21c..1b965e1 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -155,11 +155,6 @@ static inline u32 set_field_in_reg_u32(u32 field, u32 reg_value,
     return reg_value;
 }
 
-static inline u8 get_field_from_byte(u8 value, u8 mask)
-{
-    return (value & mask) / (mask & -mask);
-}
-
 static inline unsigned long region_to_pages(unsigned long addr, unsigned long size)
 {
     return (PAGE_ALIGN(addr + size) - (addr & PAGE_MASK)) >> PAGE_SHIFT;
-- 
2.1.4


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

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

[Paul Durrant]

> -----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

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

[Andrew Cooper]

On 24/09/18 12:59, Paul Durrant wrote:
>> -----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.

"No functional change" != "The binary is identical".

The functionality of queue_iommu_command() does not change, even if it's
code generation does.  It is just copying bytes into a shared ring,
bounded later by updating the tail pointer.

~Andrew

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

Re: [PATCH 2/2] x86/SVM-IOMMU: Drop get_field_from_byte()

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 24 September 2018 11:56
> 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 2/2] x86/SVM-IOMMU: Drop get_field_from_byte()
> 
> It is MASK_EXTR() in disguise, but less flexible.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@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_map.c       | 2 +-
>  xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 5 -----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> index 70b4345..ded2cc7 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -220,7 +220,7 @@ void __init iommu_dte_add_device_entry(u32 *dte,
> struct ivrs_mappings *ivrs_dev)
>      dte[7] = dte[6] = dte[4] = dte[2] = dte[1] = dte[0] = 0;
> 
>      flags = ivrs_dev->device_flags;
> -    sys_mgt = get_field_from_byte(flags, ACPI_IVHD_SYSTEM_MGMT);
> +    sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
>      dev_ex = ivrs_dev->dte_allow_exclusion;
> 
>      flags &= mask;
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> index 99bc21c..1b965e1 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -155,11 +155,6 @@ static inline u32 set_field_in_reg_u32(u32 field, u32
> reg_value,
>      return reg_value;
>  }
> 
> -static inline u8 get_field_from_byte(u8 value, u8 mask)
> -{
> -    return (value & mask) / (mask & -mask);
> -}
> -
>  static inline unsigned long region_to_pages(unsigned long addr, unsigned
> long size)
>  {
>      return (PAGE_ALIGN(addr + size) - (addr & PAGE_MASK)) >> PAGE_SHIFT;
> --
> 2.1.4

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

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

[Jan Beulich]

>>> On 24.09.18 at 13:59, <Paul.Durrant@citrix.com> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 24 September 2018 11:55
>> 
>> @@ -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).

I don't think the compiler is required to translate the original loop to
accesses of 4 bytes in ascending order, so I think I agree with
Andrew's "no functional change".

Jan



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

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

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 24 September 2018 13:06
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
> Subject: Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> queue_iommu_command()
> 
> On 24/09/18 12:59, Paul Durrant wrote:
> >> -----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.
> 
> "No functional change" != "The binary is identical".
> 
> The functionality of queue_iommu_command() does not change, even if it's
> code generation does.  It is just copying bytes into a shared ring,
> bounded later by updating the tail pointer.

Yes, my point is that the ring is shared and so DMA by the h/w may race. This is clearly not a good situation but the fact that the code generation may change may have side effects.

  Paul

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

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

[Jan Beulich]

>>> On 24.09.18 at 12:55, <andrew.cooper3@citrix.com> wrote:
> In practice, this allows the compiler to replace the loop with a pair of 
> movs.

I'm surprised the compiler doesn't recognize this opportunity anyway.

> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

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

[Jan Beulich]

>>> On 24.09.18 at 14:09, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Andrew Cooper
>> Sent: 24 September 2018 13:06
>> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
>> devel@lists.xen.org>
>> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
>> Pau Monne <roger.pau@citrix.com>; Suravee Suthikulpanit
>> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
>> Subject: Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
>> queue_iommu_command()
>> 
>> On 24/09/18 12:59, Paul Durrant wrote:
>> >> -----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.
>> 
>> "No functional change" != "The binary is identical".
>> 
>> The functionality of queue_iommu_command() does not change, even if it's
>> code generation does.  It is just copying bytes into a shared ring,
>> bounded later by updating the tail pointer.
> 
> Yes, my point is that the ring is shared and so DMA by the h/w may race. 
> This is clearly not a good situation but the fact that the code generation 
> may change may have side effects.

All writes to the ring occur strictly before the update of the tail pointer
(in MMIO), no matter how the copying gets done.

Jan


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

Re: [PATCH 2/2] x86/SVM-IOMMU: Drop get_field_from_byte()

[Jan Beulich]

>>> On 24.09.18 at 12:55, <andrew.cooper3@citrix.com> wrote:
> It is MASK_EXTR() in disguise, but less flexible.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 24 September 2018 13:16
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei
> Liu <wei.liu2@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Subject: RE: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> queue_iommu_command()
> 
> >>> On 24.09.18 at 14:09, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 24 September 2018 13:06
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> >> devel@lists.xen.org>
> >> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>;
> Roger
> >> Pau Monne <roger.pau@citrix.com>; Suravee Suthikulpanit
> >> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
> >> Subject: Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> >> queue_iommu_command()
> >>
> >> On 24/09/18 12:59, Paul Durrant wrote:
> >> >> -----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.
> >>
> >> "No functional change" != "The binary is identical".
> >>
> >> The functionality of queue_iommu_command() does not change, even if
> it's
> >> code generation does.  It is just copying bytes into a shared ring,
> >> bounded later by updating the tail pointer.
> >
> > Yes, my point is that the ring is shared and so DMA by the h/w may race.
> > This is clearly not a good situation but the fact that the code
> generation
> > may change may have side effects.
> 
> All writes to the ring occur strictly before the update of the tail
> pointer

...unless the compiler decides to re-order. There's no barrier.

  Paul

> (in MMIO), no matter how the copying gets done.
> 
> Jan

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

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

[Andrew Cooper]

On 24/09/18 13:09, Paul Durrant wrote:
>>
>>>> @@ -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.
>>
>> "No functional change" != "The binary is identical".
>>
>> The functionality of queue_iommu_command() does not change, even if it's
>> code generation does.  It is just copying bytes into a shared ring,
>> bounded later by updating the tail pointer.
> Yes, my point is that the ring is shared and so DMA by the h/w may race. This is clearly not a good situation but the fact that the code generation may change may have side effects.

This is writing past the tail pointer, and then updating the tail
pointer to match.

Hardware conforming to the spec won't read any of the data until it is
all ready.

~Andrew

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

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

[Andrew Cooper]

On 24/09/18 13:18, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 24 September 2018 13:16
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
>> <suravee.suthikulpanit@amd.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei
>> Liu <wei.liu2@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
>> Subject: RE: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
>> queue_iommu_command()
>>
>>>>> On 24.09.18 at 14:09, <Paul.Durrant@citrix.com> wrote:
>>>>  -----Original Message-----
>>>> From: Andrew Cooper
>>>> Sent: 24 September 2018 13:06
>>>> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
>>>> devel@lists.xen.org>
>>>> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>;
>> Roger
>>>> Pau Monne <roger.pau@citrix.com>; Suravee Suthikulpanit
>>>> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
>>>> Subject: Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
>>>> queue_iommu_command()
>>>>
>>>> On 24/09/18 12:59, Paul Durrant wrote:
>>>>>> -----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.
>>>>
>>>> "No functional change" != "The binary is identical".
>>>>
>>>> The functionality of queue_iommu_command() does not change, even if
>> it's
>>>> code generation does.  It is just copying bytes into a shared ring,
>>>> bounded later by updating the tail pointer.
>>> Yes, my point is that the ring is shared and so DMA by the h/w may race.
>>> This is clearly not a good situation but the fact that the code
>> generation
>>> may change may have side effects.
>> All writes to the ring occur strictly before the update of the tail
>> pointer
> ...unless the compiler decides to re-order. There's no barrier.

/sigh - You're right, but the code was equally buggy beforehand.

readl()/writel() aren't suitable for how they are used by this code.

~Andrew

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

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

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 24 September 2018 13:25
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich'
> <JBeulich@suse.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Subject: Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> queue_iommu_command()
> 
> On 24/09/18 13:18, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 24 September 2018 13:16
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> >> <suravee.suthikulpanit@amd.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Wei
> >> Liu <wei.liu2@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> >> Subject: RE: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> >> queue_iommu_command()
> >>
> >>>>> On 24.09.18 at 14:09, <Paul.Durrant@citrix.com> wrote:
> >>>>  -----Original Message-----
> >>>> From: Andrew Cooper
> >>>> Sent: 24 September 2018 13:06
> >>>> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> >>>> devel@lists.xen.org>
> >>>> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>;
> >> Roger
> >>>> Pau Monne <roger.pau@citrix.com>; Suravee Suthikulpanit
> >>>> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>
> >>>> Subject: Re: [PATCH 1/2] x86/SVM-IOMMU: Don't opencode memcpy() in
> >>>> queue_iommu_command()
> >>>>
> >>>> On 24/09/18 12:59, Paul Durrant wrote:
> >>>>>> -----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.
> >>>>
> >>>> "No functional change" != "The binary is identical".
> >>>>
> >>>> The functionality of queue_iommu_command() does not change, even if
> >> it's
> >>>> code generation does.  It is just copying bytes into a shared ring,
> >>>> bounded later by updating the tail pointer.
> >>> Yes, my point is that the ring is shared and so DMA by the h/w may
> race.
> >>> This is clearly not a good situation but the fact that the code
> >> generation
> >>> may change may have side effects.
> >> All writes to the ring occur strictly before the update of the tail
> >> pointer
> > ...unless the compiler decides to re-order. There's no barrier.
> 
> /sigh - You're right, but the code was equally buggy beforehand.
> 

Indeed, so there is the potential for functional change. I don't care that much though... someone mining for commits in future probably won't either.

  Paul

> readl()/writel() aren't suitable for how they are used by this code.
> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 24.09.18 at 14:18, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 24 September 2018 13:16
>> 
>> All writes to the ring occur strictly before the update of the tail
>> pointer
> 
> ...unless the compiler decides to re-order. There's no barrier.

But there is (an implied) one, inside writel() (due to the "volatile").
Everything ahead of the writel() invocation may be stored in any
order.

Jan



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

Re: [PATCH 2/2] x86/SVM-IOMMU: Drop get_field_from_byte()

[Woods, Brian]

On Mon, Sep 24, 2018 at 11:55:30AM +0100, Andy Cooper wrote:
> It is MASK_EXTR() in disguise, but less flexible.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Brian Woods <brian.woods@amd.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_map.c       | 2 +-
>  xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 5 -----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index 70b4345..ded2cc7 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -220,7 +220,7 @@ void __init iommu_dte_add_device_entry(u32 *dte, struct ivrs_mappings *ivrs_dev)
>      dte[7] = dte[6] = dte[4] = dte[2] = dte[1] = dte[0] = 0;
>  
>      flags = ivrs_dev->device_flags;
> -    sys_mgt = get_field_from_byte(flags, ACPI_IVHD_SYSTEM_MGMT);
> +    sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
>      dev_ex = ivrs_dev->dte_allow_exclusion;
>  
>      flags &= mask;
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> index 99bc21c..1b965e1 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -155,11 +155,6 @@ static inline u32 set_field_in_reg_u32(u32 field, u32 reg_value,
>      return reg_value;
>  }
>  
> -static inline u8 get_field_from_byte(u8 value, u8 mask)
> -{
> -    return (value & mask) / (mask & -mask);
> -}
> -
>  static inline unsigned long region_to_pages(unsigned long addr, unsigned long size)
>  {
>      return (PAGE_ALIGN(addr + size) - (addr & PAGE_MASK)) >> PAGE_SHIFT;
> -- 
> 2.1.4
> 

-- 
Brian Woods

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

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

[Woods, Brian]

On Mon, Sep 24, 2018 at 11:55:29AM +0100, Andy Cooper wrote:
> In practice, this allows the compiler to replace the loop with a pair of movs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Brian Woods <brian.woods@amd.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);
>  
>          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
> 

-- 
Brian Woods

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

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

[Roger Pau Monné]

On Mon, Sep 24, 2018 at 11:55:29AM +0100, Andrew Cooper wrote:
> In practice, this allows the compiler to replace the loop with a pair of movs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 2/2] x86/SVM-IOMMU: Drop get_field_from_byte()

[Roger Pau Monné]

On Mon, Sep 24, 2018 at 11:55:30AM +0100, Andrew Cooper wrote:
> It is MASK_EXTR() in disguise, but less flexible.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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