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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 24 September 2018 13:25
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich'
> <JBeulich@xxxxxxxx>
> Cc: Brian Woods <brian.woods@xxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> Wei Liu <wei.liu2@xxxxxxxxxx>; Xen-devel <xen-devel@xxxxxxxxxxxxx>
> 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@xxxxxxxx]
> >> Sent: 24 September 2018 13:16
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> Cc: Brian Woods <brian.woods@xxxxxxx>; Suravee Suthikulpanit
> >> <suravee.suthikulpanit@xxxxxxx>; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> Wei
> >> Liu <wei.liu2@xxxxxxxxxx>; Xen-devel <xen-devel@xxxxxxxxxxxxx>
> >> 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@xxxxxxxxxx> wrote:
> >>>>  -----Original Message-----
> >>>> From: Andrew Cooper
> >>>> Sent: 24 September 2018 13:06
> >>>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Xen-devel <xen-
> >>>> devel@xxxxxxxxxxxxx>
> >>>> Cc: Jan Beulich <JBeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> >> Roger
> >>>> Pau Monne <roger.pau@xxxxxxxxxx>; Suravee Suthikulpanit
> >>>> <suravee.suthikulpanit@xxxxxxx>; Brian Woods <brian.woods@xxxxxxx>
> >>>> 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@xxxxxxxxxx]
> >>>>>> Sent: 24 September 2018 11:55
> >>>>>> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> >>>>>> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> >>>>>> <JBeulich@xxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Roger Pau Monne
> >>>>>> <roger.pau@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>;
> >> Suravee
> >>>>>> Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Brian Woods
> >>>>>> <brian.woods@xxxxxxx>
> >>>>>> 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@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >>>>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> >>>>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>>>>> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >>>>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> >>>>>> CC: Brian Woods <brian.woods@xxxxxxx>
> >>>>>> ---
> >>>>>>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel