Skip to content
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

DEV9: fix incorrect max hdd size #10427

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

AKuHAK
Copy link
Contributor

@AKuHAK AKuHAK commented Dec 17, 2023

PS2 support 28-bit addressing not the the 24-bit addressing. 24-bit addressing is limited by 8Gb storage, while 28-bit is limited by 128Gb

Description of Changes

Change hdd max size from 8Gb to 128Gb

Rationale behind Changes

Currently, games with official HDD support very fast reach the limit of 8Gb and cannot be installed pass that mark.

Suggested Testing Steps

Run any app that shows hdd size or free space (launchelf for example). Now it will shows correct size/free space.

@github-actions github-actions bot added the DEV9 label Dec 17, 2023
PS2 support 28-bit addressing not the the 24-bit addressing. 24-bit addressing is limited by 8Gb storage, while 28-bit is limited by 128Gb
@refractionpcsx2
Copy link
Member

refractionpcsx2 commented Dec 18, 2023

Okay I just sat down to do this math and it's very confusing.

to start with 1<<28 -1 is 268,435,455, however if you go down a little bit, the size is going to be limited to 16,514,064 for cylinderslong. So I don't think changing the max size is as easy as you thought, not that the original code was correct, either.

@TheLastRar
Copy link
Contributor

TheLastRar commented Dec 20, 2023

The PS2 uses LBA addressing, the CHS values are calculated just for completeness sake (I'm not aware of anything on the PS2 that uses it)

I've checked and the 16,514,064 limitation on line 63 is as per the ATAPI spec, however, the limitation is incorrectly applied to 67 as said limit only applies to the default CHS values.

The LBA size is reported in line 167 (word 60-61), which is limited to 268,435,456 as per spec

@refractionpcsx2
Copy link
Member

Sorry, didn't realise it got used again further down, would be nice for the CHS code to not be incorrect though, I dunno if you wanna tackle it or not @AKuHAK

@AKuHAK
Copy link
Contributor Author

AKuHAK commented Dec 20, 2023

you wanna tackle it or not

According to recent researches PS2 supports UDMA 5/UDMA 6. That means that ATA interface was built according to ATA-7 specifications. But already in ATA-6 CHS support was dropped, so it is safe to not calculate that values at all.
Also CHS is limited by 16,514,063 sectors that is less than 7Gb. If HDD is larger (and PCSX2 officially doesnt support HDDs less than 40Gb), then CHS are constant values: 16382, 15, 63, so also no need for calculations.
If you wish I can rewrite the rest of the code that fills IDENTIFY response according to ATA-7 specs, but it is up to team.

This PR is fixing breaking bug that is critical. Before HDD OSD reports:
PS2 BIOS (USA)_20030623-142356_20231221001756
after change HDD OSD reports correct free space:
PS2 BIOS (USA)_20030623-142356_20231221001703

@refractionpcsx2
Copy link
Member

Aright, no worries then, thanks :)

@refractionpcsx2 refractionpcsx2 merged commit 669cadf into PCSX2:master Dec 20, 2023
12 checks passed
@TheLastRar
Copy link
Contributor

IIRC I checked the older standards to give the obsoleted values something that made sense, although I don't recall what the highest version I targeted was.

@AKuHAK
Copy link
Contributor Author

AKuHAK commented Dec 21, 2023

IIRC I checked the older standards to give the obsoleted values something that made sense, although I don't recall what the highest version I targeted was.

Probably it was ATA-5, as last that has CHS addressing (in obsolete values though). PS2 uses ATA-7 so it is safe to drop them.

@AKuHAK AKuHAK deleted the patch-7 branch December 21, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants