New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Services/AM: Implement GetPatchTitleInfos, Misc Cleanup #3048

Merged
merged 1 commit into from Oct 28, 2017

Conversation

Projects
None yet
2 participants
@shinyquagsire23
Copy link
Contributor

shinyquagsire23 commented Oct 25, 2017

Fixes #2980 (thanks to @mailwl for finding the actual issue there)


This change is Reviewable

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-patch branch 2 times, most recently from 5c24707 to 2bb20a4 Oct 25, 2017

@@ -341,6 +346,12 @@ void GetDataTitleInfos(Service::Interface* self) {
LOG_WARNING(Service_AM, "(STUBBED) called");
}

void GetPatchTitleInfos(Service::Interface* self) {
GetProgramInfos(self);

This comment has been minimized.

@Subv

Subv Oct 25, 2017

Member

You can't just call GetProgramInfos, the command headers are different. If needed, factor out the body of GetProgramInfos and call that from both functions after extracting the input parameters

This comment has been minimized.

@Subv

Subv Oct 25, 2017

Member

On that note, GetProgramInfos has IPC::RequestParser rp(Kernel::GetCommandBuffer(), 3, 2, 2); // 0x00030084 which is wrong, it should be IPC::RequestParser rp(Kernel::GetCommandBuffer(), 3, 2, 4); // 0x00030084

This comment has been minimized.

@Subv

Subv Oct 25, 2017

Member

According to RE, GetPatchTitleInfos has 4 translate parameters that you're not sending in the response.

This comment has been minimized.

@Subv

Subv Oct 25, 2017

Member

The only difference between the GetPatchTitleInfos and GetProgramInfos implementation is that GetPatchTitleInfos will first iterate over the input title ids and return 0xE0E0803C if any of them satisfies high >> 14 != 16 || high & 0xFFFF == 14

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 25, 2017

Contributor

Ah, yeah you're right, on hardware I'm seeing a 0x100D0044 in the [0] spot for the response. Looks like it's just spitting out the same buffers passed in, at least judging by the sizes. Weird.

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 25, 2017

Contributor

Actually, shouldn't it be high & 0xFFFF != 0xE? Since the TID high should always be 0x0004000E.

This comment has been minimized.

@Subv

Subv Oct 26, 2017

Member

Ah, yeah you're right, on hardware I'm seeing a 0x100D0044 in the [0] spot for the response. Looks like it's just spitting out the same buffers passed in, at least judging by the sizes. Weird.

Yeah that's precisely what it's doing, it is indeed weird but 🤷‍♂️

Actually, shouldn't it be high & 0xFFFF != 0xE? Since the TID high should always be 0x0004000E.

Yeah you're right, sorry, i misread the condition.

@shinyquagsire23 shinyquagsire23 changed the title Services/AM: Implement GetPatchTitleInfos Services/AM: Implement GetPatchTitleInfos, Misc Cleanup Oct 25, 2017

for (u32 i = 0; i < title_count; i++) {
u32 tid_high = static_cast<u32>(title_id_list[i] >> 32);
if (tid_high >> 14 != 0x10 || tid_high & 0xFFFF != 0xE) {
result = ResultCode(ErrCodes::InvalidTIDInList, ErrorModule::AM,

This comment has been minimized.

@Subv

Subv Oct 26, 2017

Member

break

LOG_WARNING(Service_AM, "(STUBBED) called");
IPC::RequestBuilder rb = rp.MakeBuilder(1, 4);
rb.Push(result);
rb.PushMappedBuffer(title_id_list_pointer, title_id_list_size, title_id_list_perms);

This comment has been minimized.

@Subv

Subv Oct 26, 2017

Member

GetProgramInfos also pushes these buffers, i am unaware of whether GetDataTitleInfos does as well, but it should be checked

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 26, 2017

Contributor

Just checked, it does seem to be the case so I might be able to merge things a bit nicer then.

Memory::ReadBlock(title_id_list_pointer, title_id_list.data(), title_count * sizeof(u64));
for (u32 i = 0; i < title_count; i++) {
u32 tid_high = static_cast<u32>(title_id_list[i] >> 32);
if (tid_high >> 14 != 0x10 || tid_high & 0xFFFF != 0xE) {

This comment has been minimized.

@Subv

Subv Oct 26, 2017

Member

add some parentheses to this expression, operator precedence will screw you over otherwise.

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-patch branch from 447a59c to 3c4b01d Oct 26, 2017

if (must_be_patch) {
for (u32 i = 0; i < title_count; i++) {
u32 tid_high = static_cast<u32>(title_id_list[i] >> 32);
if ((tid_high >> 14 != 0x10) || (tid_high & 0xFFFF != 0xE)) {

This comment has been minimized.

@Subv

Subv Oct 26, 2017

Member

that's not what i meant, this still has incorrect behavior due to operator precedence, use
if ((tid_high >> 14) != 0x10 || (tid_high & 0xFFFF) != 0xE)

This comment has been minimized.

@shinyquagsire23

shinyquagsire23 Oct 26, 2017

Contributor

Derp, fixed

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 26, 2017

I know this is pretty much the definition of scope creep, but GetDataTitleInfos also checks that every tid meets (high >> 14) == 16 && (high & 0xFFFF) == 0x8C, if any of them doesn't meet that check, then error 0xE0E0803C is returned.

The different conditions in the different functions make me like your previous approach more, reading the IPC parameters in each service handler function, and then just passing the parameters to a more general function. The TID checking of GetPatchTitleInfos and GetDataTitleInfos would happen in their respective functions before calling the general one, just like the actual AM module does.

}

for (u32 i = 0; i < title_count; i++) {
ResultCode GetTitleInfoFromList(std::vector<u64>& title_id_list, Service::FS::MediaType media_type,

This comment has been minimized.

@Subv

Subv Oct 26, 2017

Member

make the title id list const

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Oct 26, 2017

Make the std::vector parameter a const reference and squash, LGTM

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-patch branch from 2268d27 to 34a2641 Oct 26, 2017

// Validate that DLC TIDs were passed in
for (u32 i = 0; i < title_count; i++) {
u32 tid_high = static_cast<u32>(title_id_list[i] >> 32);
if ((tid_high >> 14) != 0x10 || (tid_high & 0xFFFF) != 0x8C) {

This comment has been minimized.

@Subv

Subv Oct 27, 2017

Member

If this is checking for the high tid to be some specific value, define that value as an inline static constexpr variable and compare it to that instead of doing this bit fiddling

// Validate that update TIDs were passed in
for (u32 i = 0; i < title_count; i++) {
u32 tid_high = static_cast<u32>(title_id_list[i] >> 32);
if ((tid_high >> 14) != 0x10 || (tid_high & 0xFFFF) != 0xE) {

This comment has been minimized.

@Subv

Subv Oct 27, 2017

Member

same here

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-patch branch from 34a2641 to 6682929 Oct 27, 2017

@shinyquagsire23 shinyquagsire23 force-pushed the shinyquagsire23:am-patch branch from 6682929 to 2e38ea7 Oct 27, 2017

@Subv

Subv approved these changes Oct 28, 2017

@Subv Subv merged commit 79852d3 into citra-emu:master Oct 28, 2017

2 of 3 checks passed

code-review/reviewable 4 files, 8 discussions left (shinyquagsire23, Subv)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment