Skip to content

[ATAPI] Add ATA storage driver #6577

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.

Sign up for GitHub

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

Jump to bottom
Draft
disean wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

[ATAPI] Add ATA storage driver #6577

disean wants to merge 7 commits into from
+33,927 −7,683

Conversation

@disean
Copy link
Contributor

@disean disean commented Mar 3, 2024

Purpose

This patch introduces a new PnP-aware ATA storage stack for PATA and AHCI devices. The patch is WIP, posted for comments.

JIRA issue: CORE-17256

@github-actions github-actions bot added the drivers Kernel mode drivers and frameworks label Mar 3, 2024
@binarymaster binarymaster added enhancement For PRs with an enhancement/new feature. janitorial Removal of unmaintained stuff and cleaning up labels Mar 3, 2024
binarymaster

This comment was marked as resolved.

Sign in to view

@disean
Copy link
Contributor Author

disean commented Mar 4, 2024

Ok, reverted the changes on the drivers. It's now possible to remove them in a separate PR.

@binarymaster binarymaster removed the janitorial Removal of unmaintained stuff and cleaning up label Mar 4, 2024
binarymaster
binarymaster reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 10, 2024
View reviewed changes
JoachimHenze
JoachimHenze reviewed Mar 11, 2024
View reviewed changes
binarymaster
binarymaster reviewed Apr 25, 2024
View reviewed changes
drivers/storage/ide/atapi/data.c Outdated
Comment on lines 40 to 43
const CHAR* const AtapBrokenInquiryDrive[] =
{
"THOMSON-DVD",
"PHILIPS XBOX DVD DRIVE",
"PHILIPS J5 3235C",
"SAMSUNG DVD-ROM SDG-605B"
};
Copy link
Member

@binarymaster binarymaster Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const CHAR* const AtapBrokenInquiryDrive[] =
{
"THOMSON-DVD",
"PHILIPS XBOX DVD DRIVE",
"PHILIPS J5 3235C",
"SAMSUNG DVD-ROM SDG-605B"
};
const CHAR* const AtapBrokenInquiryDrive[] =
{
"THOMSON-DVD",
"PHILIPS XBOX DVD DRIVE",
"PHILIPS J5 3235C",
"SAMSUNG DVD-ROM SDG-605B",
/* TODO: Xbox 360 drives are also affected. */
};

Technically it's possible to connect Xbox DVD drives to an ordinary PC.

Regarding X360 drives, it was mentioned somewhere in XboxDev.

Copy link
Contributor Author

@disean disean Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I failed to find anything useful in the wiki. Can you list the X360 drives here please?

Copy link
Member

@binarymaster binarymaster Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be postponed then, since I have no information for these.

@github-actions github-actions bot added the kernel&hal Code changes to the ntoskrnl and HAL label Jun 2, 2024
@disean disean force-pushed the ideport branch 4 times, most recently from cc4cbbc to 231250c Compare June 7, 2024 15:21
@disean disean force-pushed the ideport branch 3 times, most recently from 3c08a4d to 0f0e260 Compare June 14, 2024 16:23
@DarkFire01
Copy link
Contributor

DarkFire01 commented Nov 10, 2025

debuglog.txt

This strange problem was caused by the AHCI interrupts not being generated. Presuming that the hardware is sane, it will a bit hard to understand what happening.

Take a look at the AMD Simnow emulator. You'll be able to reproduce this there I'm pretty sure. but i can check for you if you'd like first :)

@DarkFire01
Copy link
Contributor

DarkFire01 commented Nov 18, 2025 •
edited
Loading

debuglog.txt

This strange problem was caused by the AHCI interrupts not being generated. Presuming that the hardware is sane, it will a bit hard to understand what happening.

Take a look at the AMD Simnow emulator. You'll be able to reproduce this there I'm pretty sure. but i can check for you if you'd like first :)

a bit of an update,
it seems this happens on pretty much all AMD systems, even later ryzen chipsets.
AGC_20251118_203000530

@disean
Copy link
Contributor Author

disean commented Nov 22, 2025 •
edited
Loading

Take a look at the AMD Simnow emulator. You'll be able to reproduce this there I'm pretty sure. but i can check for you if you'd like first :)

x64 ROS (this branch) fails to boot on a SB700 machine due to bugs in the APIC HAL. SimNow.txt
EDIT: I am able to reproduce the problem with ati_mako_hd3870.bsd (1022:4380 appears to be supporting AHCI).

it seems this happens on pretty much all AMD systems, even later ryzen chipsets.

I was able to reproduce this timeout issue with VMware. Unfortunately it seems that uniata shows the same behavior in VMware.

@disean
Copy link
Contributor Author

disean commented Nov 24, 2025

debuglog.txt

@DarkFire01 Can you please try this debug patch and grab a log? 77c7c0e

@DarkFire01
Copy link
Contributor

DarkFire01 commented Nov 25, 2025 •
edited
Loading

debuglog.txt

@DarkFire01 Can you please try this debug patch and grab a log? 77c7c0e

here:
logatapi.txt
As you can imagine this loops for awhile

@disean disean force-pushed the ideport branch 2 times, most recently from 335539a to 892f599 Compare November 25, 2025 17:30
@disean
Copy link
Contributor Author

disean commented Nov 25, 2025

As you can imagine this loops for awhile

Thanks, I've fixed a bug and disabled the PMP support code for this chip. Hope that helps. 892f599

@obsolete-is-better
Copy link

obsolete-is-better commented Nov 25, 2025

I'm running into a assert when testing this pr's artifact livecd with Bochs.
bochs_latest.txt

@DarkFire01
Copy link
Contributor

DarkFire01 commented Nov 25, 2025 •
edited
Loading

As you can imagine this loops for awhile

Thanks, I've fixed a bug and disabled the PMP support code for this chip. Hope that helps. 892f599

Thanks! Here's the log, sadly still drives aren't detected
atapilog.txt

@disean
Copy link
Contributor Author

disean commented Nov 26, 2025

Thanks! Here's the log, sadly still drives aren't detected
atapilog.txt

I'm afraid I've no idea how to debug this case any further, the log doesn't give me any further clues. It detects attaches devices correctly, but the IDENTIFY DEVICE command fails after 10 seconds due to timeout. I suspect the issue is coming from somewhere besides the driver code. What kind of mobo is it? I thought it would be helpful to acquire one.

I'm running into a assert when testing this pr's artifact livecd with Bochs.
bochs_latest.txt

These are harmless asserts, so just ignore for now.

@DarkFire01
Copy link
Contributor

DarkFire01 commented Nov 26, 2025 •
edited
Loading

Thanks! Here's the log, sadly still drives aren't detected
atapilog.txt

I'm afraid I've no idea how to debug this case any further, the log doesn't give me any further clues. It detects attaches devices correctly, but the IDENTIFY DEVICE command fails after 10 seconds due to timeout. I suspect the issue is coming from somewhere besides the driver code. What kind of mobo is it? I thought it would be helpful to acquire one.

to be honest this is happening on every single one of my AMD systems, all the way from phoenom to Ryzen.

so I imagine picking up any AMD system will work, but can give you a list of motherboard I use in my testing suite if you’d like that all did this.

Appending this list as i go through them today off and on:
ROG STRIX B850-A Gaming WIFI
MSI 770-C45

laptops:
Compaq CQ62-220US

binarymaster
binarymaster reviewed Dec 1, 2025
View reviewed changes
@disean disean force-pushed the ideport branch 3 times, most recently from d937e74 to 01b667e Compare December 15, 2025 17:11
@disean
[PCIIDEX] Add DMA support
01aeb61 
CORE-17256
@disean
[ATAPI] Add ATA storage driver
fe7953a 
CORE-17256
CORE-17191
CORE-17716
CORE-17977
CORE-13976
CORE-16216
@disean
[UNIATA] Disable the driver and enable the new ATA stack
8aa6367 
CORE-17256
@DarkFire01
Copy link
Contributor

DarkFire01 commented Dec 27, 2025 •
edited
Loading

Thanks! Here's the log, sadly still drives aren't detected
atapilog.txt

I'm afraid I've no idea how to debug this case any further, the log doesn't give me any further clues. It detects attaches devices correctly, but the IDENTIFY DEVICE command fails after 10 seconds due to timeout. I suspect the issue is coming from somewhere besides the driver code. What kind of mobo is it? I thought it would be helpful to acquire one.

to be honest this is happening on every single one of my AMD systems, all the way from phoenom to Ryzen.

so I imagine picking up any AMD system will work, but can give you a list of motherboard I use in my testing suite if you’d like that all did this.

Appending this list as i go through them today off and on: ROG STRIX B850-A Gaming WIFI MSI 770-C45

laptops: Compaq CQ62-220US

Reattempting this PR, the machines all listed above work and install now.
Thanks for looking into it!!!

@ahmed605 ahmed605 mentioned this pull request Jan 9, 2026
Merged
@cbialorucki
Copy link
Contributor

cbialorucki commented Jan 9, 2026

Wanted to add another data point: this new ata driver fixes booting ReactOS x64 on my main developer machine in VMware with an AMD Ryzen CPU. With uniata I get a double fault well before getting to desktop. Before this driver, I had to resort to using an older Intel laptop to debug ReactOS x64 on VMware.

image

cbialorucki
cbialorucki reviewed Jan 19, 2026
View reviewed changes
drivers/storage/ide/atapi/data.c
@@ -0,0 +1,68 @@
/*
* PROJECT: ReactOS ATA Port Driver
* LICENSE: GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
Copy link
Contributor

@cbialorucki cbialorucki Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be willing to license this driver under MIT instead?

HBelusca
HBelusca reviewed Jan 20, 2026
View reviewed changes
sdk/include/ndk/section_attribs.h
#endif

/*
* This attribute should be applied to a function that
Copy link
Contributor

@HBelusca HBelusca Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to a function that is called from pageable code" ?

sdk/include/ndk/section_attribs.h
#define DECLSPEC_NOINLINE_FROM_PAGED DECLSPEC_NOINLINE

/*
* This attribute should be applied to a function that
Copy link
Contributor

@HBelusca HBelusca Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same question, but for non-pageable)

hal/halx86/generic/dma.c
AdapterControlContext->MapRegisterBase = MapRegisterBase;

TempElements = ExAllocatePoolZero(NonPagedPool, sizeof(*TempElements) * MAX_SG_ELEMENTS, TAG_DMA);
ASSERT(TempElements);
Copy link
Contributor

@HBelusca HBelusca Jan 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you safely fail there instead?

drivers/storage/ide/atapi/CMakeLists.txt
Comment on lines +2 to +3
include_directories(
${REACTOS_SOURCE_DIR}/sdk/lib/drivers/sptilib)
Copy link
Contributor

@HBelusca HBelusca Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be adding
target_include_directories(sptilib INTERFACE ${REACTOS_SOURCE_DIR}/sdk/lib/drivers/sptilib)
in the sdk/lib/drivers/sptilib/CMakeLists.txt file (after the add_library command).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tkreuzer tkreuzer tkreuzer left review comments

@binarymaster binarymaster binarymaster left review comments

@HBelusca HBelusca HBelusca left review comments

@cbialorucki cbialorucki cbialorucki left review comments

@DarkFire01 DarkFire01 DarkFire01 left review comments

@JoachimHenze JoachimHenze JoachimHenze left review comments

@ThFabba ThFabba Awaiting requested review from ThFabba ThFabba is a code owner

@hpoussin hpoussin Awaiting requested review from hpoussin

Assignees

No one assigned

Labels

drivers Kernel mode drivers and frameworks enhancement For PRs with an enhancement/new feature. kernel&hal Code changes to the ntoskrnl and HAL

Projects

ReactOS PRs
Status: New PRs

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@disean @oleg-dubinskiy @tkreuzer @DarkFire01 @obsolete-is-better @binarymaster @cbialorucki @HBelusca @JoachimHenze