[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()
> -----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. Paul > (in MMIO), no matter how the copying gets done. > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |