IOS HLE cleanup #4488
IOS HLE cleanup #4488
Conversation
|
Source/Core/Core/IPC_HLE/WII_IPC_HLE.cpp, line 442 at r1 (raw file):
Might just be me confused by this choice, but why Comments from Reviewable |
leoetlino
force-pushed
the
ios-hle-cleanup
branch
from
2a3e325
to
65fa6c4
Compare
|
Review status: 0 of 24 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. Source/Core/Core/IPC_HLE/WII_IPC_HLE.cpp, line 442 at r1 (raw file): Previously, lioncash (Mat M.) wrote…
Hmm, I guess I initially wanted it to be consistent with the other addresses (for example, the device string is Comments from Reviewable |
leoetlino
force-pushed
the
ios-hle-cleanup
branch
5 times, most recently
from
586fae1
to
2a26faf
Compare
|
This needs reviewers. Are there any people who can review IOS-HLE code? Tested: |
leoetlino
force-pushed
the
ios-hle-cleanup
branch
from
2a26faf
to
d46f381
Compare
|
LBTM, net/ssl-wise. Mario Kart Wii & Boom Street failed to connect with error code 61020. I can't use or create a new profile.
The Wii disconnects before the end of the communication. While testing SSL with Monster Hunter 3(~tri) I got the same issue with error code 11612. Basically, I got this error when the game processed the data I sent and close the connection. But in that case I didn't send anything. I'm suspecting a non-persistent network connection. That regression isn't present in the latest master. I'm going to investigate to see if I can find the root cause. |
|
@leoetlino You're right, that regression isn't here if I fall back just before this commit. |
| param, param2, param3, param4, param5, _BufferIn, BufferInSize, _BufferIn2, | ||
| BufferInSize2); | ||
| param, param2, param3, param4, param5, request.in_vectors[0].addr, | ||
| request.in_vectors[0].size, request.in_vectors[1].addr, request.in_vectors[1].size); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| return device->IOCtl(address); | ||
| { | ||
| IOSResourceIOCtlRequest ioctl_request{request.address}; | ||
| ioctl_request.ClearOutputBuffer(); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
leoetlino
force-pushed
the
ios-hle-cleanup
branch
from
d46f381
to
0466d8c
Compare
| void IOSResourceRequest::SetReturnValue(const s32 new_return_value) | ||
| { | ||
| return_value = new_return_value; | ||
| Memory::Write_U32(return_value, address + 4); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| LogTypes::LOG_LEVELS verbosity) const | ||
| { | ||
| GENERIC_LOG(log_type, verbosity, "Dump of request 0x%08x (fd %d)", address, fd); | ||
| for (u32 i = 0; i < num_commands; i++) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| void IOSResourceRequest::DumpCommands(size_t num_commands, LogTypes::LOG_TYPE log_type, | ||
| LogTypes::LOG_LEVELS verbosity) const | ||
| { | ||
| GENERIC_LOG(log_type, verbosity, "Dump of request 0x%08x (fd %d)", address, fd); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| void IOSResourceIOCtlRequest::Log(const std::string& device_name, LogTypes::LOG_TYPE type, | ||
| LogTypes::LOG_LEVELS verbosity) const | ||
| { | ||
| GENERIC_LOG(type, verbosity, "%s (fd %d) - IOCtl %d (in 0x%08x %i, out 0x%08x %i)", |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| LogTypes::LOG_LEVELS verbosity) const | ||
| { | ||
| GENERIC_LOG(type, verbosity, "======= Dump ======"); | ||
| GENERIC_LOG(type, verbosity, "%s (%d) - IOCtlV %d (%zu in, %zu io)", device_name.c_str(), fd, |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| default: | ||
| ERROR_LOG(WII_IPC_DVD, "IOCtlV: %i", CommandBuffer.Parameter); | ||
| _dbg_assert_msg_(WII_IPC_DVD, 0, "IOCtlV: %i", CommandBuffer.Parameter); | ||
| ERROR_LOG(WII_IPC_DVD, "IOCtlV: %i", request.request); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
leoetlino
force-pushed
the
ios-hle-cleanup
branch
from
0466d8c
to
87a7112
Compare
|
@lioncash fixed. |
leoetlino
force-pushed
the
ios-hle-cleanup
branch
2 times, most recently
from
68ac820
to
0ed95ae
Compare
|
LGTM concerning net/ssl features. |
leoetlino
force-pushed
the
ios-hle-cleanup
branch
from
0ed95ae
to
41fa79c
Compare
|
@lioncash Is this good to go now? Is there anything else to do? |
leoetlino
force-pushed
the
ios-hle-cleanup
branch
5 times, most recently
from
8d3010a
to
dbfe03f
Compare
ExecuteCommand was becoming pretty confusing with unused variables for some commands, confusing names (device ID != IOS file descriptor), duplicated error checking code, not keeping the indentation level low, and having everything into the same function. This commit gives more correct names to variables, centralises the device checking code, and splits ExecuteCommand so that it's easier to read.
It looks like at some point Dolphin device IDs coincided with IOS file descriptors, but this is not the case anymore, so keeping those WriteReturnCode(GetDeviceID()) in every single IOS HLE device, as if the device ID was used as IOS fd, is both unnecessary and confusing.
This removes Open() and Close() functions from devices whenever they did nothing more than the base class (setting m_Active, returning a default reply). Also, since IOS close commands practically always return FS_SUCCESS, writing the return code is moved to HandleCommand() in WII_IPC_HLE, which has two benefits: it's not duplicated all over the place (so people will not forget it) and it gets rid of having to check the force parameter, since HandleCommand() is always called for real IOS commands, so command_address is guaranteed to be valid.
We don't really have to keep track of device opens/closes manually, since we can already check that by calling IsOpened() on the device. This also replaces some loops with for range loops.
- Use an enum instead of defines. - Only use the FS_ prefix for return codes which are actually related to FS stuff, not for everything. - Remove duplicated error codes and clean up the names.
Removes #defines which have been unused for years and cleans up naming. This also changes IPC_REP_ASYNC to simply IPC_REPLY because it turns out it's actually not specific to async replies, but used for all command replies.
This moves the resource request parsing code into well-defined structs instead of duplicating it all over IOS HLE. Command handler functions are passed parsed resource requests instead of a command address. Note: Device_net really needs some serious cleanup… Some other notable changes: - The return code is not an obscure Memory::Write_U32 anymore, but an explicit SetReturnValue() call. - The DumpAsync and DumpCommands were moved to the relevant request structs, because it didn't really make sense to have them on the IOS device and DumpAsync only works for ioctlv requests. - Clearing the ioctl output buffer is now done in the constructor of IOSResourceIOCtlRequest; there's no reason to copy/paste the clearing code in IOS HLE now. The diff may look huge, but most of them are just renaming and removing things.
Note: the HID code and Bluetooth code are left untouched since PR 4408 is going to rewrite most of it anyway and already drops the GetPointer usages in the USB code.
leoetlino
force-pushed
the
ios-hle-cleanup
branch
from
dbfe03f
to
909e0f5
Compare
|
Closing this in favour of smaller, easier to review PRs. |
The first commit refactors ExecuteCommand to be less confusing and to not have duplicate checks; fyi there was a bug that caused the HBC homebrew launches to fail (and which was fixed only a few days ago in 288e75f), because the device check was duplicated inside of each
switchcase.The second commit is a straightforward unused defines/naming cleanup.
The third and fifth remove dead/useless code duplicated all over IOS HLE; namely, the Open()/Close() methods. A lot of IOS device classes simply changed m_Active, wrote a return code to the emulated memory and returned a default reply… which resulted in loads of duplicated code, especially since that's what the base class methods already do… (And it's been this way since the initial megacommit, but as years passed on, more devices were added, which meant more copy-and-pasted code.)
Another problem is that the devices' Open() method used GetDeviceID() and wrote its return value to the IOS return code, as if it were a IOS fd. It looks like that may have been true at one point, but that's definitely not the case anymore, so I have removed it because that is just both confusing (especially as someone who didn't have any experience with IOS HLE) and unnecessary.
The eighth commit deduplicates the resource parsing/reply code. Instead of doing memory reads/writes manually, command handlers are now passed parsed structs, which clarifies things like setting the return value.
The last commit drops GetPointer usages in the whole IPC_HLE code – except in USB code, since that's going to be replaced in #4408 anyway – to prepare for locking. These are replaced either with Memory::Read/Write/CopyFromEmu/CopyToEmu directly, and helper wrappers around them for ioctls and IO vectors.
This change is