PSP memory, port, and EEPROM functions assume direct-mapped access

[skliper]

The PSP currently provides a number of access functions such as CFE_PSP_MemWrite8/16/32, CFE_PSP_PortRead8/16/32, etc.

These functions all assume that the memory is directly accessible to the current process by simply casting the address as a pointer and directly reading/writing from it. Unfortunately this is often NOT the case.

• I/O port access in the x86/intel world is never memory mapped and requires different instructions (inline asm or kernel syscalls) in order to get to it.
• When using virtual memory, physical memory addresses are not directly accessible to the running process until the memory is mapped into the current virtual memory space.
• Many EEPROM devices are actually connected via a serial bus such as SPI or TWI and therefore would not be memory mapped.

With the way it is structured right now, cfe_psp_ram.c, cfe_psp_port.c, and (to a lesser degree) cfe_psp_eeprom.c only provide slow performance-robbing function calls to simply cast a pointer.

Furthermore, it is arguable whether direct I/O port or physical memory access even belongs in the PSP at all; the API this provides to the application layer remains far too hardware-specific to provide any useful abstraction. Any CFS application performing direct I/O is already unlikely to be portable to any other platform, since (by definition) this would be accessing a specific hardware device at a specific location.

Proposed changes:

• Stop compiling cfe_psp_ram.c, cfe_psp_port.c, and cfe_psp_eeprom.c from the "shared" code by default; what these currently implement is more of an exception than the general rule. These can be renamed or moved to indicate they are not always used.
• Deprecate/discourage future use of the RAM/port access functions. Instead, a "driver" architecture should be used so the hardware device can be better abstracted (see PSP modular build enhancements #6 This also allows easier simulation of the hardware, and the resulting CFS application will be much more portable/reusable.
• On the PSP's for which direct memory access //is// valid, they can continue to compile-in the current implementations to maintain backward compatibility.
• On the PSP's for which direct memory access //is not// valid, either customized functions can be provided or simply return the NOT_IMPLEMENTED error.

[skliper]

Imported from trac issue 7. Created by jphickey on 2014-12-30T16:03:54, last modified: 2019-08-26T10:19:37

[skliper]

Trac comment by jphickey on 2015-04-06 12:34:44:

This needs some discussion by the CCB to determine what the course of action should be.

[skliper]

Trac comment by jphickey on 2018-05-14 16:48:24:

Should discuss this for the next PSP release.

IMO these API calls should be deprecated. They could be moved to a compatibility module that could be compiled-in at the mission discretion (CMake build system already has this static module feature).

[skliper]

Trac comment by jhageman on 2019-06-10 14:46:24:

Marked for CCB review - proposal is to deprecate (preference is to get them marked for end-of-summer release so they can be removed in 7.0).

[skliper]

Trac comment by jhageman on 2019-06-13 10:15:12:

CCB 6/12/2019 - Agreed to deprecate, mark functions in cfe_psp_ram, cfe_psp_port, and cfe_psp_eeprom as deprecated

[skliper]

Trac comment by jhageman on 2019-07-16 13:49:53:

Is this actively being worked? Estimated work complete date?

[skliper]

Trac comment by jphickey on 2019-07-16 15:02:28:

This is a quick fix and it can be implemented as soon as we have settled on a general deprecation procedure. Once we have the agreed upon procedure, we can start the process on these items and the code change is minimal as we are not removing them for the first release.

[skliper]

Trac comment by jhageman on 2019-07-16 17:13:09:

What's the plan for cfe_psp_eeprom_mmap_file.c? Just confirmed internal use of these functions is limited to CFE_PSP_MemRead8 in cfe/fsw/cfe-core/src/es/cfe_es_api.c so I agree, not extensive changes to deprecate.

[skliper]

Trac comment by jhageman on 2019-08-14 12:59:54:

CCB 8/14/2019 - Expected complete by 8/21/2019

[skliper]

Trac comment by jhageman on 2019-08-16 21:40:54:

Per CCB email, just stick with the general deprecation define.

[skliper]

Trac comment by jphickey on 2019-08-20 12:26:08:

Propose commit [changeset:34b2016] on branch trac-7-deprecate-direct-ram-port-functions for review.

Upon further inspection of the use cases for the memory access routines, I came to the conclusion that we really can't just simply deprecate them. They are used frequently in CFS apps, in particular MM, MD, etc. And there may be justified use cases of accessing memory through a wrapper, particularly when the memory resides off-chip or somewhere that it needs to be "paged-in" or otherwise indirectly accessed.

Instead of trying to remove this function, this moves the code into separate modules. This allows the abstraction to become useful. For systems that do need the memory access routines, it is as simple as including the module in the TGT<x>_PSP_MODULELIST in the targets.cmake file.

This does, however, deprecate the CFE_PSP_MemCpy and CFE_PSP_MemSet routines using the general method.

[skliper]

Trac comment by jhageman on 2019-08-21 14:44:13:

Needs careful review and further discussion. I'm not in favor adding this additional set of configuration options to the cFS Framework without sufficient justification. My preference is we either include the wrappers or don't. I'm not in favor of the cFS Framework owning the additional configuration complexity of trying to account for every unique project's memory configurations.

[skliper]

Trac comment by jphickey on 2019-08-22 14:20:02:

I would argue that this is not quite the same thing as "additional configuration options" ... this approach is different than something like an on/off macro switch inside cfe_platform_cfg.h or whatever, and I would say its more sustainable, which is why I'm suggesting it.

That being said, I don't consider this a final solution either, its more of a baby step. We have discussed in many circumstances that CFE needs some sort of "driver" implementation for platform devices, but nothing has been agreed to yet.

The build system does, however, already support a pluggable PSP module option - configured just like CFS apps and libraries are- so this a baby step toward using that existing feature to implement a sort-of "driver-like" module for memory device access.

It's not quite there yet because its still primarily a compile-time thing, whereas a true driver infrastructure would provide more selectability at runtime. There is a solution for that too (see #33 but this is a technical steering committee topic.

So in short, the real issue with the existing code as it was that it was a sort of "useless abstraction" -- that is, it was implemented in the PSP, yet the same exact code was linked into all the PSP implementations without any sort of provision to do anything else. So while the abstract call was defined, it wasn't possible to actually provide any abstraction with it, because all the platforms use the same one.

My position here is that these calls do offer some value, so we should keep them, it's just they are not useful in their existing form. I'd rather make them useful than take them out.

[skliper]

Trac comment by jphickey on 2019-08-22 14:24:58:

Also important to note that this does not add any new build system logic/features/options. It is entirely using build features that already existed. In fact it is the exact same mechanisms that are used for static CFS apps, just applying it in a new place.

[skliper]

Trac comment by jhageman on 2019-08-26 10:19:37:

Implementation does not match latest CCB agreed direction, moved to future release to allow for further discussion.

[skliper]

@jphickey is this a candidate for config/source selection related updates? I didn't refresh on all the comments, but separate the code and select source files as needed.

[jphickey]

Yes - to clarify this would be a prime example for the fully-modular PSP concept. With the pending merge that moves the startup code to OSAL BSP, this becomes more possible where a "PSP name" is just a collection of modular code abstraction blobs for that platform. I'd like to see all remaining PSP functions, including these, go in that direction.