[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>, 'Jan Beulich' <JBeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 24 Sep 2018 13:24:45 +0100
  • Autocrypt: addr=andrew.cooper3@xxxxxxxxxx; prefer-encrypt=mutual; keydata= xsFNBFLhNn8BEADVhE+Hb8i0GV6mihnnr/uiQQdPF8kUoFzCOPXkf7jQ5sLYeJa0cQi6Penp VtiFYznTairnVsN5J+ujSTIb+OlMSJUWV4opS7WVNnxHbFTPYZVQ3erv7NKc2iVizCRZ2Kxn srM1oPXWRic8BIAdYOKOloF2300SL/bIpeD+x7h3w9B/qez7nOin5NzkxgFoaUeIal12pXSR Q354FKFoy6Vh96gc4VRqte3jw8mPuJQpfws+Pb+swvSf/i1q1+1I4jsRQQh2m6OTADHIqg2E ofTYAEh7R5HfPx0EXoEDMdRjOeKn8+vvkAwhviWXTHlG3R1QkbE5M/oywnZ83udJmi+lxjJ5 YhQ5IzomvJ16H0Bq+TLyVLO/VRksp1VR9HxCzItLNCS8PdpYYz5TC204ViycobYU65WMpzWe LFAGn8jSS25XIpqv0Y9k87dLbctKKA14Ifw2kq5OIVu2FuX+3i446JOa2vpCI9GcjCzi3oHV e00bzYiHMIl0FICrNJU0Kjho8pdo0m2uxkn6SYEpogAy9pnatUlO+erL4LqFUO7GXSdBRbw5 gNt25XTLdSFuZtMxkY3tq8MFss5QnjhehCVPEpE6y9ZjI4XB8ad1G4oBHVGK5LMsvg22PfMJ ISWFSHoF/B5+lHkCKWkFxZ0gZn33ju5n6/FOdEx4B8cMJt+cWwARAQABzSlBbmRyZXcgQ29v cGVyIDxhbmRyZXcuY29vcGVyM0BjaXRyaXguY29tPsLBegQTAQgAJAIbAwULCQgHAwUVCgkI CwUWAgMBAAIeAQIXgAUCWKD95wIZAQAKCRBlw/kGpdefoHbdD/9AIoR3k6fKl+RFiFpyAhvO 59ttDFI7nIAnlYngev2XUR3acFElJATHSDO0ju+hqWqAb8kVijXLops0gOfqt3VPZq9cuHlh IMDquatGLzAadfFx2eQYIYT+FYuMoPZy/aTUazmJIDVxP7L383grjIkn+7tAv+qeDfE+txL4 SAm1UHNvmdfgL2/lcmL3xRh7sub3nJilM93RWX1Pe5LBSDXO45uzCGEdst6uSlzYR/MEr+5Z JQQ32JV64zwvf/aKaagSQSQMYNX9JFgfZ3TKWC1KJQbX5ssoX/5hNLqxMcZV3TN7kU8I3kjK mPec9+1nECOjjJSO/h4P0sBZyIUGfguwzhEeGf4sMCuSEM4xjCnwiBwftR17sr0spYcOpqET ZGcAmyYcNjy6CYadNCnfR40vhhWuCfNCBzWnUW0lFoo12wb0YnzoOLjvfD6OL3JjIUJNOmJy RCsJ5IA/Iz33RhSVRmROu+TztwuThClw63g7+hoyewv7BemKyuU6FTVhjjW+XUWmS/FzknSi dAG+insr0746cTPpSkGl3KAXeWDGJzve7/SBBfyznWCMGaf8E2P1oOdIZRxHgWj0zNr1+ooF /PzgLPiCI4OMUttTlEKChgbUTQ+5o0P080JojqfXwbPAyumbaYcQNiH1/xYbJdOFSiBv9rpt TQTBLzDKXok86M7BTQRS4TZ/ARAAkgqudHsp+hd82UVkvgnlqZjzz2vyrYfz7bkPtXaGb9H4 Rfo7mQsEQavEBdWWjbga6eMnDqtu+FC+qeTGYebToxEyp2lKDSoAsvt8w82tIlP/EbmRbDVn 7bhjBlfRcFjVYw8uVDPptT0TV47vpoCVkTwcyb6OltJrvg/QzV9f07DJswuda1JH3/qvYu0p vjPnYvCq4NsqY2XSdAJ02HrdYPFtNyPEntu1n1KK+gJrstjtw7KsZ4ygXYrsm/oCBiVW/OgU g/XIlGErkrxe4vQvJyVwg6YH653YTX5hLLUEL1NS4TCo47RP+wi6y+TnuAL36UtK/uFyEuPy wwrDVcC4cIFhYSfsO0BumEI65yu7a8aHbGfq2lW251UcoU48Z27ZUUZd2Dr6O/n8poQHbaTd 6bJJSjzGGHZVbRP9UQ3lkmkmc0+XCHmj5WhwNNYjgbbmML7y0fsJT5RgvefAIFfHBg7fTY/i kBEimoUsTEQz+N4hbKwo1hULfVxDJStE4sbPhjbsPCrlXf6W9CxSyQ0qmZ2bXsLQYRj2xqd1 bpA+1o1j2N4/au1R/uSiUFjewJdT/LX1EklKDcQwpk06Af/N7VZtSfEJeRV04unbsKVXWZAk uAJyDDKN99ziC0Wz5kcPyVD1HNf8bgaqGDzrv3TfYjwqayRFcMf7xJaL9xXedMcAEQEAAcLB XwQYAQgACQUCUuE2fwIbDAAKCRBlw/kGpdefoG4XEACD1Qf/er8EA7g23HMxYWd3FXHThrVQ HgiGdk5Yh632vjOm9L4sd/GCEACVQKjsu98e8o3ysitFlznEns5EAAXEbITrgKWXDDUWGYxd pnjj2u+GkVdsOAGk0kxczX6s+VRBhpbBI2PWnOsRJgU2n10PZ3mZD4Xu9kU2IXYmuW+e5KCA vTArRUdCrAtIa1k01sPipPPw6dfxx2e5asy21YOytzxuWFfJTGnVxZZSCyLUO83sh6OZhJkk b9rxL9wPmpN/t2IPaEKoAc0FTQZS36wAMOXkBh24PQ9gaLJvfPKpNzGD8XWR5HHF0NLIJhgg 4ZlEXQ2fVp3XrtocHqhu4UZR4koCijgB8sB7Tb0GCpwK+C4UePdFLfhKyRdSXuvY3AHJd4CP 4JzW0Bzq/WXY3XMOzUTYApGQpnUpdOmuQSfpV9MQO+/jo7r6yPbxT7CwRS5dcQPzUiuHLK9i nvjREdh84qycnx0/6dDroYhp0DFv4udxuAvt1h4wGwTPRQZerSm4xaYegEFusyhbZrI0U9tJ B8WrhBLXDiYlyJT6zOV2yZFuW47VrLsjYnHwn27hmxTC/7tvG3euCklmkn9Sl9IAKFu29RSo d5bD8kMSCYsTqtTfT6W4A3qHGvIDta3ptLYpIAOD2sY3GYq2nf3Bbzx81wZK14JdDDHUX2Rs 6+ahAA==
  • Cc: Wei Liu <wei.liu2@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxx>, Brian Woods <brian.woods@xxxxxxx>, Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • Delivery-date: Mon, 24 Sep 2018 12:31:37 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

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.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.