linux-kernel - Re: [tpmdd-devel] [PATCH] tpm: fix cacheline alignment for DMA-able buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160809150114.GA9672@intel.com>
Date:	Tue, 9 Aug 2016 18:01:14 +0300
From:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:	Dmitry Torokhov <dtor@...omium.org>
Cc:	Christophe Ricard <christophe.ricard@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	tpmdd-devel@...ts.sourceforge.net
Subject: Re: [tpmdd-devel] [PATCH] tpm: fix cacheline alignment for DMA-able
 buffers

On Tue, Aug 09, 2016 at 12:46:10PM +0300, Jarkko Sakkinen wrote:
> On Fri, Jul 29, 2016 at 10:30:22AM -0700, Dmitry Torokhov wrote:
> >    On Fri, Jul 29, 2016 at 10:27 AM, Jason Gunthorpe
> >    <jgunthorpe@...idianresearch.com> wrote:
> > 
> >      On Thu, Jul 28, 2016 at 07:59:13PM -0700, Andrey Pronin wrote:
> >      > Annotate buffers used in spi transactions as ____cacheline_aligned
> >      > to use in DMA transfers.
> >      >
> >      > Signed-off-by: Andrey Pronin <apronin@...omium.org>
> >      >Â  drivers/char/tpm/st33zp24/spi.c | 4 ++--
> >      >Â  drivers/char/tpm/tpm_tis_spi.cÂ  | 4 ++--
> >      >Â  2 files changed, 4 insertions(+), 4 deletions(-)
> >      >
> >      > diff --git a/drivers/char/tpm/st33zp24/spi.c
> >      b/drivers/char/tpm/st33zp24/spi.c
> >      > index 9f5a011..0e9aad9 100644
> >      > +++ b/drivers/char/tpm/st33zp24/spi.c
> >      > @@ -70,8 +70,8 @@
> >      >Â  struct st33zp24_spi_phy {
> >      >Â  Â  Â  Â struct spi_device *spi_device;
> >      >
> >      > -Â  Â  Â u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE];
> >      > -Â  Â  Â u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE];
> >      > +Â  Â  Â u8 tx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
> >      > +Â  Â  Â u8 rx_buf[ST33ZP24_SPI_BUFFER_SIZE] ____cacheline_aligned;
> >      >
> >      >Â  Â  Â  Â int io_lpcpd;
> >      >Â  Â  Â  Â int latency;
> > 
> >      Hurm, this still looks wrong to me. Aligning the start of buffers is
> >      not enough, the DMA'able space must also end on a cache line as well.
> > 
> >      So, the buffers must also always be placed at the end of the struct.
> > 
> >      IMHO It would be cleaner and safer to always kmalloc the DMA buffer
> >      alone than to try and optimize like this.
> > 
> >    In this case moving them to the end of the structure and commenting why
> >    they have to be at the end might be less invasive change. More
> >    performance-efficient and resilient in low memory situations too.
> 
> kmallocs would be done in the driver initialization:
> 
> * you rarely are in low memory situation
> * performance gain/loss is insignificant
> 
> I really don't see your point.

I'm fine having them at the end of the structure mainly for simplicity
reasons but those arguments just didn't hold at all.

/Jarkko