RFC: failsafe dealing with _PSP_FW_VERSION

[rofl0r]

i've been trying to port my gnuboy fork to psp, and while it works fine in PPSSPP, it crashes on real psp with fw 6.60, which might or might not be due to wrong struct layouts when anything using these structs has been compiled with the wrong value for _PSP_FW_VERSION. so i recompiled pspsdk, GL, SDL2 etc with -D_PSP_FW_VERSION=600, yet it still crashes (i might have missed some library which is still using the default of 150).

which made me think of how this imo annoying situation with different struct layouts can be solved and the SDK user doesn't have to deal with it at all. musl libc solves similar issues by having wrappers for syscalls that use structs that for some reason might be different in userspace than what the kernel expects.

let's say there's a syscall void sceFooBar(struct sceFooBarStruct*); and a struct sceFooBarStruct that would currently look like:

struct sceFooBarStruct {
  int x;
#if _PSP_FW_VERSION > 150
  int foo; /* fw 1.5.1 added foo member in the middle of struct */
#endif 
  int y;
};

this clearly raises a lot of issues, a user of sceFooBar() that doesn't have the entire sdk and all libraries compiled with the exact firmware macro then what this will run on may either pass a struct that's too short (which will result in the kernel overwriting or reading some unrelated memory) or values intended for the y slot will end up somewhere else.

this could be dealt with by designing the struct like this:

struct sceFooBarCompatStruct {
  int x;
  int y;
  int foo; /* put new member at the end */
};

struct sceFooBarStruct150 {
  int x;
  int y;
};

struct sceFooBarStructGt150 {
  int x;
  int foo;
  int y;
};

and then making sceFooBar() a wrapper:

void sceFooBar(struct sceFooBarCompatStruct *foo) {
  struct sceFooBarStruct150 f150;
  struct sceFooBarStructGt150 later;
  if (sceKernelDevkitVersion() <= 0x01050001) {
     f150.x = foo->x;
     f150.y = foo->y;
     realSceFooBar(&f150);
  } else {
     later.x = foo->x;
     later.y = foo->y;
     later.foo = foo->foo;
     realSceFooBar(&later);
  }
}

from what i can tell this would completely eliminate the need to set _PSP_FW_VERSION and would make the code work on all firmware versions. i'm not sure how the actual syscalls are done but from what i've seen from psp-fixup-imports tool it looks like stubs are used that could be used for wrapping syscalls like this.

please let me know what you think of this proposal or if you have better ideas.

[fjtrujy]

Are you sure that is the real issue? Have you used psplink to debug your app, and really understand where it is crashing?

Finally IMO I wouldn't take care of legacy firmware version, I don't even know if current toolchain supports that legacy version

[sharkwouter]

I think the problem here is probably not that your application is being compiled for the wrong firmware, but that SDL2main is not being loaded in your main file.

For a game or application to be able to start on the PSP it will need the following code in the main:

PSP_MODULE_INFO("SDL App", 0, 1, 0);
PSP_MAIN_THREAD_ATTR(THREAD_ATTR_VFPU | THREAD_ATTR_USER);

Try adding that. If it resolves the issue, that means SDL2main isn't working. Make sure to add #include <SDL.h> to your main and link to SDL2main. Then You should not need the above code.

I think this is a bug in PPSSPP to be honest. It doesn't care about what PSP_MAIN_THREAD_ATTR is set to, it will work anyway. I could be wrong, though. There could be a bug in the PSPSDK here, but I'd suggest trying this out first to make sure.

[sajattack]

It's not a bug, PPSSPP just ignores VFPU exceptions by default.

[rofl0r]

For a game or application to be able to start on the PSP it will need the following code in the main:

thanks, this fixes it indeed!

interestingly, the HOME button is ignored though, and the emulator appears to be not running at full speed, as indicated by stuttering audio (which is kinda surprising, as gnuboy runs with 3% cpu use on my 800 mhz laptop)... would it be faster to use oldschool library instead of SDL2? (i assumed that since SDL2 uses GL for drawing, it should be fully hw accelerated)

but back to topic...

Finally IMO I wouldn't take care of legacy firmware version, I don't even know if current toolchain supports that legacy version

that sounds like a good advice and the easiest solution for the issue. would you accept a PR that removes all usage of _PSP_FW_VERSION macro and uses the struct layout of fw version 6.60 everywhere (we could keep a comment saying something like "this struct member doesn't exist in fw version 1.50, or similar) ? iirc, there's only 5 spots where things differ in current headers. from what i know, all psp's that can run 1.50 or 2.00 can also be updated to 6.60 while maintaining homebrew ability.

[rofl0r]

for reference, these are the occurences of _PSP_FW_VERSION:

• ./src/samples/savedata/utility/main.c:#if _PSP_FW_VERSION >= 200
• ./src/samples/savedata/utility/main.c:#if _PSP_FW_VERSION >= 200
• ./src/samples/utility/netdialog/main.c:#if _PSP_FW_VERSION >= 200
• ./src/samples/utility/netdialog/main.c: #if _PSP_FW_VERSION >= 200

these can be ignored as far as this RFC is concerned.

• ./src/kernel/pspthreadman_kernel.h:#if _PSP_FW_VERSION >= 200
• ./src/kernel/psploadexec_kernel.h:#if _PSP_FW_VERSION < 200
• ./src/kernel/psploadexec_kernel.h:#if _PSP_FW_VERSION >= 200
• ./src/kernel/psploadexec_kernel.h:#if _PSP_FW_VERSION >= 300
• ./src/user/pspsysmem.h:#if _PSP_FW_VERSION >= 150
• ./src/utility/psputility_savedata.h:#if _PSP_FW_VERSION >= 200

looking at each of those in detail:

./src/kernel/pspthreadman_kernel.h:#if _PSP_FW_VERSION >= 200

/** Structure to hold the status information for a thread (kernel form)
 * 1.5 form
  */
typedef struct SceKernelThreadKInfo {
...
        /** Number of clock cycles run */
        SceKernelSysClock   runClocks;
#if _PSP_FW_VERSION >= 200
        SceUInt unk3; /* Unknown extra field on later firmwares */
#endif  
        /** Interrupt preemption count */
        SceUInt     intrPreemptCount;

./src/kernel/psploadexec_kernel.h:#if _PSP_FW_VERSION < 200

#if _PSP_FW_VERSION < 200
... (comments)
int sceKernelLoadExecBufferPlain(SceSize bufsize, void *buf, struct SceKernelLoadExecParam *param);
#endif

./src/kernel/psploadexec_kernel.h:#if _PSP_FW_VERSION >= 200

#if _PSP_FW_VERSION >= 200
... (comments)
int sceKernelExitVSHKernel(struct SceKernelLoadExecVSHParam *param);
#endif

./src/kernel/psploadexec_kernel.h:#if _PSP_FW_VERSION >= 300

#if _PSP_FW_VERSION >= 300
... (comments)
int sceKernelLoadExecVSHMs4(const char *file, struct SceKernelLoadExecVSHParam *param);
#endif

./src/user/pspsysmem.h:#if _PSP_FW_VERSION >= 150

#if _PSP_FW_VERSION >= 150
... (comments)
void sceKernelPrintf(const char *format, ...) __attribute__((format(printf, 1, 2));
#endif

./src/utility/psputility_savedata.h:#if _PSP_FW_VERSION >= 200

/** Structure to hold the parameters for the ::sceUtilitySavedataInitStart function */
typedef struct SceUtilitySavedataParam {
...
        /** unknown2: ? */
        int unknown2[4];

#if _PSP_FW_VERSION >= 200

        /** key: encrypt/decrypt key for save with firmware >= 2.00 */
        char key[16];

        /** unknown3: ? */
        char unknown3[20];

#endif
} SceUtilitySavedataParam;

so it appears the only 2 cases where memory corruption or unexpected behaviour can occur is the 2 structs here; in the latter one the struct will be simply too short for new firmware versions, so just adding the members unconditionally won't have any negative impact on older firmwares anyway. the ugly one is the SceKernelThreadKInfo struct where a member was added in the middle.

[artart78]

To be honest, I don't really see the purpose of adding a failsafe like you described. The only cases which can lead to bugs are cases where kernel versions are involved, which are supposed to be used by "experts" only, and adding a systematic call to sceKernelDevkitVersion() sounds burdensome and possibly even inappropriate in some cases (because of context, latency or whatever).

[rofl0r]

you're right. the offending struct SceKernelThreadKInfo is only used by int ThreadManForKernel_2D69D086(SceUID uid, SceKernelThreadKInfo *info); which doesn't even have it's NID cracked yet, and isn't used in the entire pspsdk nor in the newlib patch. so we can probably ignore it safely. otoh SceUtilitySavedataParam is used in an sdk sample, which means some 3rd party libs might use it too, so there's a slight change something is using it behind our back. but this one is easy to fix by just always including the new struct members a the end. with that changed, the info could be added somewhere prominently that users can safely ignore _PSP_FW_VERSION unless they hack on kernel stuff, and the _PSP_FW_VERSION-related cargo-culting can be eliminated once and for all.