All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Simek <michal.simek@xilinx.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7
Date: Fri, 9 Mar 2018 13:25:09 +0100	[thread overview]
Message-ID: <b986fec0-13ee-935a-bb63-5d57aa1615f9@xilinx.com> (raw)
In-Reply-To: <20180309120241.069A6247241@gemini.denx.de>

Dear Wolfgang,

On 9.3.2018 13:02, Wolfgang Denk wrote:
> Dear Michal,
> 
> In message <fc0a315b-4e51-a009-05e3-1a140bf82155@xilinx.com> you wrote:
>>
>> We are not seeing any issue from u-boot code itself and I can believe
>> that structures and accesses are aligned properly.
>>
>> The initial reason was that u-boot allows you to run for example command
>> md 1 and it ends up in panic. Which is something what should be possible
>> to run or code should check that architecture has alignment restriction.
> 
> I'm not sure this needs any "fixing".  Actually I use this on a
> regular base as a simple means to find out if an architecutre allos
> unaligned accesses...  U-Boot is just a boot loader.  It has no
> "normal users" - users are usually developers who know what they are
> doing.  I think this is an area where this old quote applies nicely:
> 
> "UNIX was not designed to stop you from doing stupid things,  because
> that would also stop you from doing clever things."       - Doug Gwyn

I understand your opinion and no problem with it.


>> Definitely panic is not proper reaction on command like this.
> 
> Why not?  Yes, we can blow up the code to handle this in a better
> way, and add some kB of stricgs for nice and user-friendy warnings
> not to do this or that.  But is it really worth the effort?
> I consider this a pretty useful feature and would be sad if it went
> missing. If someone runs into it by accident, he learns a lesson -
> but htere is no real damage done.  Or am I missing anything?

It is not that the patch is removing or changing current behavior.
Default option is to use strict alignment and have an option to change
this behavior.


>> It means we have two options:
>> 1. enable non aligned accesses
>> 2. fix code/commands which enables to pass these non aligned addresses
>> which ends up in panic
> 
> My vorte is for option 3:
> 
> 3. Leave as is.  It's a boot loader, and like other sharp tools it
> can be dangerous if improperly used.

Sure that's also an option but it seems to me weird that by default
armv7 has this requirement and a53 not.


>>> But I guess they still come under a (severe?) performance penalty?
>>
>> It is really a question where that penalty is coming from and when it
>> can happen.
> 
> It comes from the memory and cache architecture.  Just assume an
> unaligned read of 2 bytes that happen to cross a cacheline boundary.
> This access causes a double cache miss; in the worst case this
> causes two cache lines to be flushed to memory and then two
> cachelines to be written from memory.  Instead of just reading two
> bytes, we write 128 bytes and read 128 bytes.  Yes, this is a
> pathological case, but you get the idea.

If this is WB case that write shouldn't be there but you are right that
this could happen.

>> But again the code is align and there is not an issue in code that's why
>> this penalty is not happening. And non align accesses are performed when
>> user asks for this.
> 
> Why not tell the user: don't perform unaligend accesses on this
> platform then?  I can't see the situation where he really _needs_ to
> do something which is not natively supported by his hardware?

It is supported by hardware. U-Boot/SW just doing this configuration not
to allow it.

>>> Also, I wonder if ARM still maintains atomicity for unaligned reads/
>>> writes ? [IIRC, x86 doesn't, while it's guaranteed for aligned
>>> accesses.]
>>
>> Don't know.
> 
> Well, I'm willing to bet a few beer this doesn't work at least in
> pathological cases like example of crossing cache lines above.

:-)

Thanks,
Michal

  reply	other threads:[~2018-03-09 12:25 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-07 14:39 [U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7 Michal Simek
2018-03-08 22:52 ` Wolfgang Denk
2018-03-09  6:59   ` Michal Simek
2018-03-09 11:26     ` Wolfgang Denk
2018-03-09 11:41       ` Michal Simek
2018-03-09 12:02         ` Wolfgang Denk
2018-03-09 12:25           ` Michal Simek [this message]
2018-03-09 12:23       ` Mark Kettenis
2018-03-09 12:29         ` Michal Simek

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=b986fec0-13ee-935a-bb63-5d57aa1615f9@xilinx.com \
    --to=michal.simek@xilinx.com \
    --cc=u-boot@lists.denx.de \
    /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.