#3007 closed defect (fixed)

ARM caching issues

Reported by: munster Owned by: joel.sherrill@…
Priority: normal Milestone: 5.1
Component: arch/arm Version: 4.11
Severity: normal Keywords: ARM, cache, L2C 310
Cc: Blocked By:
Blocking:

Description

There are two problems with the caching on ARM:

  • In cases where the buffer is not aligned to line boundary at the beginning or the end, the invalidate operation would lose modifications done on the adjacent data. This applies to both L1 and L2 caches.
  • The L2C-310 cache management operations use excessive locking. According to manual, the used operations (Clean Line by PA, Clean and Invalidate Line by PA, Cache Sync) are atomic and do not require locking.

I have attached the proposed patch.

Change History (25)

Changed on Apr 27, 2017 at 2:24:20 PM by munster

Attachment: cache.diff added

comment:1 Changed on Apr 27, 2017 at 11:44:36 PM by Chris Johns

Why have you changed the signature of some of the functions to be uint32_t, uint32_t from const void* , const size_t ?

comment:2 Changed on Apr 28, 2017 at 3:13:35 PM by munster

Well, I was just following the Linux driver example. If you don't like it, I can use the previous function signature.

comment:3 in reply to:  2 Changed on Apr 30, 2017 at 10:09:35 PM by Chris Johns

Replying to munster:

Well, I was just following the Linux driver example. If you don't like it, I can use the previous function signature.

I think it is best a change like this is as small as can be and keeping the existing function signatures helps do this so please revert that part of the change and post a new patch. Thanks.

comment:4 Changed on May 2, 2017 at 5:30:52 AM by Sebastian Huber

Please don't change the function signatures. Please generate one patch for each issue.

Its up to the user to ensure that a cache line invalidation is safe. The cache manager should not take care of this.

Changed on May 8, 2017 at 9:58:29 AM by munster

Attachment: cache_invalidate.diff added

Changed on May 8, 2017 at 9:58:44 AM by munster

Attachment: cache_locking.diff added

comment:5 in reply to:  4 ; Changed on May 8, 2017 at 10:02:40 AM by munster

Replying to Sebastian Huber:

Please don't change the function signatures. Please generate one patch for each issue.

I have attached separate patches for these two issues.

Its up to the user to ensure that a cache line invalidation is safe. The cache manager should not take care of this.

Generally you are right. However, the user may not be aware of this particular problem.

comment:6 in reply to:  5 ; Changed on May 9, 2017 at 7:58:16 AM by Sebastian Huber

Replying to munster:

Replying to Sebastian Huber:

Please don't change the function signatures. Please generate one patch for each issue.

I have attached separate patches for these two issues.

Could you please format the patches via "git format-patch":

https://devel.rtems.org/wiki/Developer/Contributing

Please help to improve the wiki and website if something is unclear or hard to find.


Its up to the user to ensure that a cache line invalidation is safe. The cache manager should not take care of this.

Generally you are right. However, the user may not be aware of this particular problem.

The we should fix this issue with better documentation. In

https://git.rtems.org/rtems/tree/cpukit/rtems/include/rtems/rtems/cache.h#n111

we have

/**
 * @brief Invalidates multiple data cache lines.
 *
 * The cache lines covering the area are marked as invalid.  A later read
 * access in the area will load the data from memory.
 *
 * In case the area is not aligned on cache line boundaries, then this
 * operation may destroy unrelated data.
 *
 * @param[in] addr The start address of the area to invalidate.
 * @param[in] size The size in bytes of the area to invalidate.
 */
void rtems_cache_invalidate_multiple_data_lines(
  const void *addr,
  size_t size
);

The cache manager documentation probably needs to move into the user manual. Maybe some examples help to show what can go wrong.

comment:7 Changed on May 11, 2017 at 7:31:02 AM by Sebastian Huber

Milestone: 4.124.12.0

comment:8 in reply to:  6 Changed on May 11, 2017 at 2:27:34 PM by munster

Replying to Sebastian Huber:

Could you please format the patches via "git format-patch":

I have attached the patches.

comment:9 Changed on May 12, 2017 at 5:23:15 AM by Sebastian Huber

See

https://devel.rtems.org/wiki/Developer/Git/Users#CreatingaPatch

Please use your real name for the patch with a valid e-mail address.

comment:10 Changed on Jun 8, 2017 at 7:46:40 AM by Sebastian Huber

Milestone: 4.12.04.12.1

comment:11 Changed on Jun 12, 2017 at 7:34:17 AM by Sebastian Huber

Without a real name I cannot apply this patch.

comment:12 in reply to:  11 Changed on Jun 14, 2017 at 11:45:18 AM by munster

Replying to Sebastian Huber:

Without a real name I cannot apply this patch.

Re-attached with real name.

comment:13 Changed on Jun 14, 2017 at 1:41:30 PM by Alexei Pososin <m09123874@…>

In fd10817/rtems:

Remove excessive locking from cache operations.

According to manual, the used operations (Clean Line by PA, Clean and
Invalidate Line by PA, Cache Sync) are atomic and do not require
locking.

Update #3007.

comment:14 Changed on Jun 14, 2017 at 1:46:21 PM by Sebastian Huber

Resolution: fixed
Status: newclosed

It is up to the user to ensure that its safe to invalidate a certain data area.

comment:15 Changed on Jun 14, 2017 at 1:46:35 PM by Sebastian Huber

Version: 4.124.11

comment:16 Changed on Jul 12, 2017 at 10:32:43 PM by Chris Johns

Milestone: 4.12.14.12.0

comment:17 Changed on Oct 10, 2017 at 6:54:12 AM by Sebastian Huber

Component: bspsarch/arm

comment:18 Changed on Nov 9, 2017 at 6:27:14 AM by Sebastian Huber

Milestone: 4.12.05.1

Milestone renamed

Note: See TracTickets for help on using tickets.