[v2] bootm: Align cache flush begin address

Message ID 1523789297-8011-1-git-send-email-bryan.odonoghue@linaro.org
State Accepted
Commit bbac9222dcf2a4c233269e67ea38ddfb3536d85b
Headers show
Series
  • [v2] bootm: Align cache flush begin address
Related show

Commit Message

Bryan O'Donoghue April 15, 2018, 10:48 a.m.
commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
aligns the end address of the cache flush operation to a cache-line size to
ensure lower-layers in the code accept the range provided and flush.

A similar action should be taken for the begin address of a cache flush
operation. The load address may not be aligned to a cache-line boundary, so
ensure the passed address is aligned.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
Suggested-by: Tom Rini <trini@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
---
 common/bootm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Breno Matheus Lima April 16, 2018, 5:44 p.m. | #1
Hi Bryan,

2018-04-15 7:48 GMT-03:00 Bryan O'Donoghue <bryan.odonoghue@linaro.org>:
> commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
> aligns the end address of the cache flush operation to a cache-line size to
> ensure lower-layers in the code accept the range provided and flush.
>
> A similar action should be taken for the begin address of a cache flush
> operation. The load address may not be aligned to a cache-line boundary, so
> ensure the passed address is aligned.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
> Suggested-by: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>

Thanks for the fix, I'm not seeing the issue anymore.

Tested-by: Breno Lima <breno.lima@nxp.com>
Simon Glass April 16, 2018, 5:49 p.m. | #2
On 15 April 2018 at 04:48, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
> commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
> aligns the end address of the cache flush operation to a cache-line size to
> ensure lower-layers in the code accept the range provided and flush.
>
> A similar action should be taken for the begin address of a cache flush
> operation. The load address may not be aligned to a cache-line boundary, so
> ensure the passed address is aligned.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
> Suggested-by: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  common/bootm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

I feel that using an unaligned start address should probably be an
error. Why would that be useful?

Apart from that:

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon
Bryan O'Donoghue April 17, 2018, 7:27 a.m. | #3
On 16/04/18 17:49, Simon Glass wrote:
> On 15 April 2018 at 04:48, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>> commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
>> aligns the end address of the cache flush operation to a cache-line size to
>> ensure lower-layers in the code accept the range provided and flush.
>>
>> A similar action should be taken for the begin address of a cache flush
>> operation. The load address may not be aligned to a cache-line boundary, so
>> ensure the passed address is aligned.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
>> Suggested-by: Tom Rini <trini@konsulko.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>   common/bootm.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> I feel that using an unaligned start address should probably be an
> error. Why would that be useful?
> 
> Apart from that:
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Regards,
> Simon
> 

We are booting a kernel that has an entry point aligned a cacheline 
boundary with a header prefix/load-address that is a negative offset 
from that.

We could go about trying to move the load/ep address of that kernel but, 
my feeling is that's probably the wrong thing to do, we can just as 
easily align-down and add to the flush length.

---
bod
Simon Glass April 17, 2018, 11:21 p.m. | #4
Hi Bryan,

On 17 April 2018 at 03:27, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>
>
>
> On 16/04/18 17:49, Simon Glass wrote:
>>
>> On 15 April 2018 at 04:48, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>>>
>>> commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
>>> aligns the end address of the cache flush operation to a cache-line size to
>>> ensure lower-layers in the code accept the range provided and flush.
>>>
>>> A similar action should be taken for the begin address of a cache flush
>>> operation. The load address may not be aligned to a cache-line boundary, so
>>> ensure the passed address is aligned.
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
>>> Suggested-by: Tom Rini <trini@konsulko.com>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> ---
>>>   common/bootm.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>>
>> I feel that using an unaligned start address should probably be an
>> error. Why would that be useful?
>>
>> Apart from that:
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> Regards,
>> Simon
>>
>
> We are booting a kernel that has an entry point aligned a cacheline boundary with a header prefix/load-address that is a negative offset from that.
>
> We could go about trying to move the load/ep address of that kernel but, my feeling is that's probably the wrong thing to do, we can just as easily align-down and add to the flush length.

What header is this? Perhaps it should be updated to be a cache-line
multiple in size?

I suspect the impact of this patch is minimal, since people hopefully
don't put data just before the image is loaded. But if they did, and
the image is loaded using DMA behind the cache, we might have tricky
bugs. That's why in general I'm not keen on silently messing with the
cache outside the expected range.

Regards,
Simon
Bryan O'Donoghue April 18, 2018, 5:22 p.m. | #5
On 17/04/18 23:21, Simon Glass wrote:
> Hi Bryan,
> 
> On 17 April 2018 at 03:27, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>>
>>
>>
>> On 16/04/18 17:49, Simon Glass wrote:
>>>
>>> On 15 April 2018 at 04:48, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>>>>
>>>> commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
>>>> aligns the end address of the cache flush operation to a cache-line size to
>>>> ensure lower-layers in the code accept the range provided and flush.
>>>>
>>>> A similar action should be taken for the begin address of a cache flush
>>>> operation. The load address may not be aligned to a cache-line boundary, so
>>>> ensure the passed address is aligned.
>>>>
>>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
>>>> Suggested-by: Tom Rini <trini@konsulko.com>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> ---
>>>>    common/bootm.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>>
>>> I feel that using an unaligned start address should probably be an
>>> error. Why would that be useful?
>>>
>>> Apart from that:
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> Regards,
>>> Simon
>>>
>>
>> We are booting a kernel that has an entry point aligned a cacheline boundary with a header prefix/load-address that is a negative offset from that.
>>
>> We could go about trying to move the load/ep address of that kernel but, my feeling is that's probably the wrong thing to do, we can just as easily align-down and add to the flush length.
> 
> What header is this? Perhaps it should be updated to be a cache-line
> multiple in size?

I don't think so it's a TEE header, that's pretty fixed.

> I suspect the impact of this patch is minimal, since people hopefully
> don't put data just before the image is loaded. But if they did, and
> the image is loaded using DMA behind the cache, we might have tricky
> bugs. That's why in general I'm not keen on silently messing with the
> cache outside the expected range.

Yes, I agree with both points.

How printing a warning ?

if (flush_start < load)
     flush_len += load - flush_start;

if (flush_start < load) {
     printf("WARNING: unaligned load address 0x%08lx flushing 0x%08lx\n",
            load, flush_start);
     flush_len += load - flush_start;
}
Simon Glass April 23, 2018, 8:21 p.m. | #6
Hi Bryan,

On 18 April 2018 at 11:22, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>
>
>
> On 17/04/18 23:21, Simon Glass wrote:
>>
>> Hi Bryan,
>>
>> On 17 April 2018 at 03:27, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>>>
>>>
>>>
>>>
>>> On 16/04/18 17:49, Simon Glass wrote:
>>>>
>>>>
>>>> On 15 April 2018 at 04:48, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
>>>>>
>>>>>
>>>>> commit b4d956f6bc0f ("bootm: Align cache flush end address correctly")
>>>>> aligns the end address of the cache flush operation to a cache-line size to
>>>>> ensure lower-layers in the code accept the range provided and flush.
>>>>>
>>>>> A similar action should be taken for the begin address of a cache flush
>>>>> operation. The load address may not be aligned to a cache-line boundary, so
>>>>> ensure the passed address is aligned.
>>>>>
>>>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>> Reported-by: Breno Matheus Lima <brenomatheus@gmail.com>
>>>>> Suggested-by: Tom Rini <trini@konsulko.com>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>>    common/bootm.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>>
>>>>
>>>> I feel that using an unaligned start address should probably be an
>>>> error. Why would that be useful?
>>>>
>>>> Apart from that:
>>>>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>
>>> We are booting a kernel that has an entry point aligned a cacheline boundary with a header prefix/load-address that is a negative offset from that.
>>>
>>> We could go about trying to move the load/ep address of that kernel but, my feeling is that's probably the wrong thing to do, we can just as easily align-down and add to the flush length.
>>
>>
>> What header is this? Perhaps it should be updated to be a cache-line
>> multiple in size?
>
>
> I don't think so it's a TEE header, that's pretty fixed.
>
>> I suspect the impact of this patch is minimal, since people hopefully
>> don't put data just before the image is loaded. But if they did, and
>> the image is loaded using DMA behind the cache, we might have tricky
>> bugs. That's why in general I'm not keen on silently messing with the
>> cache outside the expected range.
>
>
> Yes, I agree with both points.
>
> How printing a warning ?
>
> if (flush_start < load)
>     flush_len += load - flush_start;
>
> if (flush_start < load) {
>     printf("WARNING: unaligned load address 0x%08lx flushing 0x%08lx\n",
>            load, flush_start);
>
>     flush_len += load - flush_start;
> }

That seems like a good idea.

Regards,
Simon

Patch

diff --git a/common/bootm.c b/common/bootm.c
index adb1213..3616291 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -434,6 +434,8 @@  static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
 	ulong blob_end = os.end;
 	ulong image_start = os.image_start;
 	ulong image_len = os.image_len;
+	ulong flush_start = ALIGN_DOWN(load, ARCH_DMA_MINALIGN);
+	ulong flush_len = *load_end - load;
 	bool no_overlap;
 	void *load_buf, *image_buf;
 	int err;
@@ -447,7 +449,11 @@  static int bootm_load_os(bootm_headers_t *images, unsigned long *load_end,
 		bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE);
 		return err;
 	}
-	flush_cache(load, ALIGN(*load_end - load, ARCH_DMA_MINALIGN));
+
+	if (flush_start < load)
+		flush_len += load - flush_start;
+
+	flush_cache(flush_start, ALIGN(flush_len, ARCH_DMA_MINALIGN));
 
 	debug("   kernel loaded at 0x%08lx, end = 0x%08lx\n", load, *load_end);
 	bootstage_mark(BOOTSTAGE_ID_KERNEL_LOADED);