Unable to read data in ATAPIO after writing data

[suncloudsmoon]

Recently, I embarked on a journey to create a simple hobby OS to learn more about how computers work. While creating the OS, I felt the need to create a ATAPIO driver to read/write to hard drive. However, when I try to read some data from the ATAPIO driver after I first write some data to the hard drive, I only get zeros back. I tried several ways to fix this issue, such as flushing the write cache, but none of them worked. Any solutions welcome.

Github Repo: https://github.com/suncloudsmoon/Simple ... 543ec1903d

ata.hpp

Code: Select all

#ifndef DRIVERS_ATA_HPP
#define DRIVERS_ATA_HPP

#include <stddef.h>
#include <stdint.h>

#include <lib/zutil.hpp>
#include <lib/zstring.hpp>

namespace os {
	namespace driv {
		namespace ata {
			using LBA28 = uint32_t;
			namespace commands {
				constexpr uint16_t identity = 0xEC;
				constexpr uint8_t read_sec = 0x20;
				constexpr uint8_t write_sec = 0x30;
				constexpr uint16_t cache_flush = 0xE7;
			}
			namespace ports {
				constexpr uint16_t io_base = (uint16_t) 0x1F0;
				constexpr uint16_t data = io_base + (uint16_t) 0;
				constexpr uint16_t err = io_base + (uint16_t) 1;
				
				constexpr uint16_t sec_count = io_base + (uint16_t) 2;
				constexpr uint16_t lba_low = io_base + (uint16_t) 3;
				constexpr uint16_t lba_mid = io_base + (uint16_t) 4;
				constexpr uint16_t lba_high = io_base + (uint16_t) 5;

				constexpr uint16_t drive_head_select = io_base + (uint16_t) 6;
				constexpr uint16_t command = io_base + (uint16_t) 7;
				constexpr uint16_t status = command;
			}
			// Useful once you get the status value from ports::status
			namespace status_info {
				constexpr uint8_t err = (uint8_t) 1;
				constexpr uint8_t idx = (uint8_t) 2;
				constexpr uint8_t corr = (uint8_t) 4;
				constexpr uint8_t drq = (uint8_t) 8;
				constexpr uint8_t srv = (uint8_t) 16;
				constexpr uint8_t df = (uint8_t) 32;
				constexpr uint8_t rdy = (uint8_t) 64;
				constexpr uint8_t bsy = (uint8_t) 128;
			}

			namespace drive_type {
				enum {
					master_drive = 0xE0,
					slave_drive = 0xF0
				};
			}
			// Got info from https://wiki.osdev.org/PCI_IDE_Controller#Read.2FWrite_From_ATA_Drive
			namespace drive_bit {
				enum {
					master_bit = 0,
					slave_bit = 1
				};
			}

			struct data_packet {
				data_packet(uint16_t *dat, size_t siz) : data(dat), bytes(siz) {}
				operator bool() { return data && bytes; }
				uint16_t *data;
				size_t bytes;
			};
			struct atapio_identify_info {
				LBA28 max_sector_count = 0;
			};
			struct err_message {
				zl::string str;
				int code = 0;
			};
			class atapio {
				public:
					atapio(unsigned polling_limit = 10000);
					operator bool() { return ata_init_success; }
					
					bool write_cache_flush();
					zl::expected<data_packet> read(int drive_bit, LBA28 addr, uint16_t num_sectors);
					bool write(int drive_bit, LBA28 addr, data_packet dat);
					
					const atapio_identify_info& get_dev_info() const {
						return hdd_info;
					}
					const err_message& get_err() const {
						return err;
					}
				private:
					void identity_cmd();
					unsigned poll_lim;
					bool ata_init_success = true;
					
					atapio_identify_info hdd_info;
					err_message err;
			};
		}
	}
}

ata.cpp

Code: Select all

#include <drivers/ata.hpp>
#include <drivers/instr.hpp>

#include <lib/zmem.hpp>
#include <lib/zio.hpp>
#include <lib/zassert.hpp>

namespace os {
	namespace driv {
		namespace ata {
			atapio::atapio(unsigned polling_limit) : poll_lim(polling_limit) {
				identity_cmd();
			}

			void atapio::identity_cmd() {
				x86::outb(ports::drive_head_select, drive_type::master_drive);
				// Zero out the IO ports
				for (uint16_t port = 0x1F2; port <= 0x1F5; port++)
					x86::outb(port, NULL);
				x86::outb(ports::command, commands::identity);
				
				// Polling for the status
				unsigned polling_index = 0;
				uint16_t status = 0;
				while (polling_index++ < poll_lim) {
					status = x86::inb(ports::status);
					if ((status & 128) == 0) break;
				}
				if (status == 0) {
					ata_init_success = false;
					err.str = "[os::driv::ata::atapio::identity_cmd() error] -> status poll failure!";
					err.code = -1;
					return;
				}

				// Read 256 16 bit values from the data (0x1F0) port
				uint16_t identity_data[256]{};
				for (uint16_t i = 0; i < 256; i++)
					identity_data[i] = x86::inw(ports::data);
				
				uint32_t first_lba_max = identity_data[60];
				uint32_t second_lba_max = identity_data[61] << 16;
				hdd_info.max_sector_count |= first_lba_max;
				hdd_info.max_sector_count |= second_lba_max;
				if (hdd_info.max_sector_count == 0) {
					ata_init_success = false;
					err.str = "[os::driv::ata::atapio::identity_cmd() error] -> LBA28 addressing mode is not supported on current hardware!";
					err.code = -2;
				}
			}

			bool atapio::write_cache_flush() {
				x86::outb(ports::command, commands::cache_flush);
				unsigned index = 0;
				while (index++ < poll_lim)
					if ((x86::inb(ports::status) & status_info::bsy) == 0) break;
				if (index >= poll_lim)
					return false;
				return true;
			}

			// TODO: fix memory leak resulting from data_packet... maybe add a destructor?
			// num_sectors -> number of 512 byte sectors to read
			zl::expected<data_packet> atapio::read(int drive_bit, LBA28 addr, uint16_t num_sectors) {
				zl::assert(ata_init_success, "[os::driv::ata::atapio::read(int, LBA28, uint16_t) error] -> ATA failed to initialize!");
				
				uint16_t num_read = num_sectors * 256;
				data_packet packet = { new uint16_t[num_read]{}, num_read * sizeof(uint16_t) };

				x86::outb(ports::sec_count, num_sectors / 256);
				x86::outb(ports::lba_low, addr);
				x86::outb(ports::lba_mid, addr >> 8);
				x86::outb(ports::lba_high, addr >> 16);
				x86::outb(ports::drive_head_select, drive_type::master_drive | (drive_bit << 4) | ((addr >> 24) & 0xF));
				x86::outb(ports::command, commands::read_sec);

				// Wait until the device is ready to tranfer data
				unsigned poll_index = 0;
				while (poll_index++ < poll_lim) {
					uint8_t status = x86::inb(ports::status);
					// (Bit 3 is not 0 and Bit 7 is 0) or (Bit 0/5 is 1)
					if ((status & status_info::bsy) == 0 && (status & status_info::drq) != 0) break;
				}
				if (poll_index >= poll_lim) {
					delete[] packet.data;
					return {{nullptr, 0}, "[os::driv::ata::atapio::read() error] -> unable to read from ATA bus within polling limit!", -1};
				}
				for (size_t i = 0; i < packet.bytes / sizeof(uint16_t); i++)
					packet.data[i] = x86::inw(ports::data);

				return packet;
			}
			bool atapio::write(int drive_bit, LBA28 addr, data_packet dat) {
				zl::assert(ata_init_success, "[os::driv::ata::atapio::write(int, LBA28, data_packet) error] -> ATA failed to initialize!");
				
				if (!dat) return false;

				x86::outb(ports::sec_count, (dat.bytes / 512) / 256);
				x86::outb(ports::lba_low, addr);
				x86::outb(ports::lba_mid, addr >> 8);
				x86::outb(ports::lba_high, addr >> 16);
				x86::outb(ports::drive_head_select, drive_type::master_drive | (drive_bit << 4) | ((addr >> 24) & 0xF));
				x86::outb(ports::command, commands::write_sec);

				// Wait until the device is ready to tranfer data
				unsigned poll_index = 0;
				while (poll_index++ < poll_lim) {
					uint8_t status = x86::inb(ports::status);
					// (Bit 3 is not 0 and Bit 7 is 0) or (Bit 0/5 is 1)
					if ((status & status_info::bsy) == 0 && (status & status_info::drq) != 0) break;
				}
				if (poll_index >= poll_lim)
					return false;
				for (size_t i = 0; i < dat.bytes / sizeof(uint16_t); i++)
					x86::outw(ports::data, dat.data[i]);

				return write_cache_flush();		
			}
		}
	}
}

[Octocontrabass]

Each drive has its own set of registers, and writing to the drive/head register switches between them. You need to write to the drive/head register first when switching between drives, otherwise some of your writes will update one drive's registers and the rest will update the other drive's registers.

However, when I try to read some data from the ATAPIO driver after I first write some data to the hard drive, I only get zeros back.

Open your disk image in a hex editor and see if your attempted writes are actually writing anything.

Code: Select all

err.str = "[os::driv::ata::atapio::identity_cmd() error] -> LBA28 addressing mode is not supported on current hardware!";

...Do you have any drives old enough for this message to appear?

[Ethin]

Might be just me nitpicking, but since your using constexpr, you can collapse your namespace declarations into one (e.g. namespace os::drive::atapio) instead of using lots of nested namespace declarations.

[suncloudsmoon]

Might be just me nitpicking, but since your using constexpr, you can collapse your namespace declarations into one (e.g. namespace os::drive::atapio) instead of using lots of nested namespace declarations.

Oh, I didn't know that such syntax was valid in C++. It appears to be a recent C++ feature. Thanks for the suggestion.

[suncloudsmoon]

Each drive has its own set of registers, and writing to the drive/head register switches between them. You need to write to the drive/head register first when switching between drives, otherwise some of your writes will update one drive's registers and the rest will update the other drive's registers.

However, when I try to read some data from the ATAPIO driver after I first write some data to the hard drive, I only get zeros back.

Open your disk image in a hex editor and see if your attempted writes are actually writing anything.

Code: Select all

err.str = "[os::driv::ata::atapio::identity_cmd() error] -> LBA28 addressing mode is not supported on current hardware!";

...Do you have any drives old enough for this message to appear?

Thanks for the help. After digging in the code for a while, it seems like the bug found is rather silly.
This is the line of code (in the atapio::write() function) that has been causing a lot of issues.

Code: Select all

x86::outb(ports::sec_count, (dat.bytes / 512) / 256);

This statement on the OSdev website confused me:

Assume you have a sectorcount byte and a 28 bit LBA value. A sectorcount of 0 means 256 sectors = 128K.

I thought that the above rule applies to both the read() and write() ATAPIO commands. However, it turns out that the sectorcount in atapio::write() command is the number of 512 byte sectors to write and is not following the above rule. The above rule only applies to the atapio::read() command.

[Octocontrabass]

This statement on the OSdev website confused me:

Assume you have a sectorcount byte and a 28 bit LBA value. A sectorcount of 0 means 256 sectors = 128K.

I thought that the above rule applies to both the read() and write() ATAPIO commands. However, it turns out that the sectorcount in atapio::write() command is the number of 512 byte sectors to write and is not following the above rule. The above rule only applies to the atapio::read() command.

It's always the number of 512-byte sectors. I think you've misunderstood that rule: it only applies when the sector count is 0.