[U-Boot] [PATCH v2] bootm: Align cache flush begin address

[Simon Glass]

Hi Bryan,

On 18 April 2018 at 11:22, Bryan O'Donoghue <bryan.odonoghue at 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 at linaro.org> wrote:
>>>
>>>
>>>
>>>
>>> On 16/04/18 17:49, Simon Glass wrote:
>>>>
>>>>
>>>> On 15 April 2018 at 04:48, Bryan O'Donoghue <bryan.odonoghue at 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 at linaro.org>
>>>>> Reported-by: Breno Matheus Lima <brenomatheus at gmail.com>
>>>>> Suggested-by: Tom Rini <trini at konsulko.com>
>>>>> Cc: Simon Glass <sjg at 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 at 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