> -----Original Message----- > From: Andrew Cooper > Sent: 24 September 2018 13:06 > To: Paul Durrant <paul.durr...@citrix.com>; Xen-devel <xen- > de...@lists.xen.org> > Cc: Jan Beulich <jbeul...@suse.com>; Wei Liu <wei.l...@citrix.com>; Roger > Pau Monne <roger....@citrix.com>; Suravee Suthikulpanit > <suravee.suthikulpa...@amd.com>; Brian Woods <brian.wo...@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.coop...@citrix.com] > >> Sent: 24 September 2018 11:55 > >> To: Xen-devel <xen-de...@lists.xen.org> > >> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Jan Beulich > >> <jbeul...@suse.com>; Wei Liu <wei.l...@citrix.com>; Roger Pau Monne > >> <roger....@citrix.com>; Paul Durrant <paul.durr...@citrix.com>; Suravee > >> Suthikulpanit <suravee.suthikulpa...@amd.com>; Brian Woods > >> <brian.wo...@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.coop...@citrix.com> > >> --- > >> CC: Jan Beulich <jbeul...@suse.com> > >> CC: Wei Liu <wei.l...@citrix.com> > >> CC: Roger Pau Monné <roger....@citrix.com> > >> CC: Paul Durrant <paul.durr...@citrix.com> > >> CC: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com> > >> CC: Brian Woods <brian.wo...@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
