All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
To: "Winkler, Tomas" <tomas.winkler@intel.com>
Cc: "tpmdd-devel@lists.sourceforge.net" 
	<tpmdd-devel@lists.sourceforge.net>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] tpm: use get_unaligned_be32 unaligned buffer access.
Date: Wed, 23 Nov 2016 14:01:39 -0700	[thread overview]
Message-ID: <20161123210138.GB15803@obsidianresearch.com> (raw)
In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B543308CB@hasmsx108.ger.corp.intel.com>

On Wed, Nov 23, 2016 at 08:56:25PM +0000, Winkler, Tomas wrote:
> > On Wed, Nov 23, 2016 at 01:04:54PM +0200, Tomas Winkler wrote:
> > > Use get_unaligned_be32 as b32_to_cpu doesn't work correctly on all
> > > platforms for unaligned access.
> > >
> > > The fix doesn't cover all the cases as also some cast structures have
> > > members on unaligned addresses.
> > 
> > I think this is a good idea..
> > 
> > > @@ -353,8 +353,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8
> > *buf, size_t bufsiz,
> > >  	if (bufsiz > TPM_BUFSIZE)
> > >  		bufsiz = TPM_BUFSIZE;
> > >
> > > -	count = be32_to_cpu(*((__be32 *) (buf + 2)));
> > > -	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
> > > +	count = get_unaligned_be32(buf + 2);
> > > +	ordinal = get_unaligned_be32(buf + 6);
> >
> > But lets fix this better and get rid of the constants too...
> 
> >  const tpm_input_header *hdr = buf;
> >  count = be32_to_cpu(hdr->length);
> >  ordinal = be32_to_cpu(hdr->ordinal);
> > 
> > Compiler will take care of unaligned for __packed.
> 
> Yes, compiler takes care at performance penalty but probably we
> don't care about that much,

Hmm? get_unaligned_be32 boils down to the same __packed construct.

As is today we must be hitting the in-kernel unaligned access trap (eg
on ARM) which is *very* expensive so this is a very worthwhile fix ...

Jason

WARNING: multiple messages have this Message-ID (diff)
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
To: "Winkler, Tomas" <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: "tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org"
	<tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH] tpm: use get_unaligned_be32 unaligned buffer access.
Date: Wed, 23 Nov 2016 14:01:39 -0700	[thread overview]
Message-ID: <20161123210138.GB15803@obsidianresearch.com> (raw)
In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B543308CB-Jy8z56yoSI8MvF1YICWikbfspsVTdybXVpNB7YpNyf8@public.gmane.org>

On Wed, Nov 23, 2016 at 08:56:25PM +0000, Winkler, Tomas wrote:
> > On Wed, Nov 23, 2016 at 01:04:54PM +0200, Tomas Winkler wrote:
> > > Use get_unaligned_be32 as b32_to_cpu doesn't work correctly on all
> > > platforms for unaligned access.
> > >
> > > The fix doesn't cover all the cases as also some cast structures have
> > > members on unaligned addresses.
> > 
> > I think this is a good idea..
> > 
> > > @@ -353,8 +353,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8
> > *buf, size_t bufsiz,
> > >  	if (bufsiz > TPM_BUFSIZE)
> > >  		bufsiz = TPM_BUFSIZE;
> > >
> > > -	count = be32_to_cpu(*((__be32 *) (buf + 2)));
> > > -	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
> > > +	count = get_unaligned_be32(buf + 2);
> > > +	ordinal = get_unaligned_be32(buf + 6);
> >
> > But lets fix this better and get rid of the constants too...
> 
> >  const tpm_input_header *hdr = buf;
> >  count = be32_to_cpu(hdr->length);
> >  ordinal = be32_to_cpu(hdr->ordinal);
> > 
> > Compiler will take care of unaligned for __packed.
> 
> Yes, compiler takes care at performance penalty but probably we
> don't care about that much,

Hmm? get_unaligned_be32 boils down to the same __packed construct.

As is today we must be hitting the in-kernel unaligned access trap (eg
on ARM) which is *very* expensive so this is a very worthwhile fix ...

Jason

------------------------------------------------------------------------------

  reply	other threads:[~2016-11-23 21:02 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-23 11:04 [PATCH] tpm: use get_unaligned_be32 unaligned buffer access Tomas Winkler
2016-11-23 11:04 ` Tomas Winkler
2016-11-23 16:57 ` Jason Gunthorpe
2016-11-23 16:57   ` Jason Gunthorpe
2016-11-23 20:56   ` Winkler, Tomas
2016-11-23 20:56     ` Winkler, Tomas
2016-11-23 21:01     ` Jason Gunthorpe [this message]
2016-11-23 21:01       ` Jason Gunthorpe
2016-11-24 13:34 ` Jarkko Sakkinen
2016-12-03 15:30   ` Jarkko Sakkinen
2016-12-03 15:30     ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161123210138.GB15803@obsidianresearch.com \
    --to=jgunthorpe@obsidianresearch.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tomas.winkler@intel.com \
    --cc=tpmdd-devel@lists.sourceforge.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.