From f31a9e9374334a44b41097aecabbc5845a4edbca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Julian=20Offenh=C3=A4user?= <offenhaeuser@protonmail.com>
Date: Tue, 14 Mar 2023 15:23:49 +0100
Subject: [PATCH] Kernel: Refactor AHCIController to propagate more errors

Before, the mapping of our HBA region would be done in the constructor.
Since this can fail, I moved it into initialize().

Additionally, we now use the TypedMapping helper for mapping the HBA
instead of doing it manually. This actually uncovered a bug where we
would ignore any possible offset into the page we were mapping, which
caused us to miss the mapped registers entirely.
---
 Kernel/Storage/ATA/AHCI/Controller.cpp | 13 +++++++------
 Kernel/Storage/ATA/AHCI/Controller.h   |  5 +++--
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/Kernel/Storage/ATA/AHCI/Controller.cpp b/Kernel/Storage/ATA/AHCI/Controller.cpp
index ca55dd3340..ca39c04cb4 100644
--- a/Kernel/Storage/ATA/AHCI/Controller.cpp
+++ b/Kernel/Storage/ATA/AHCI/Controller.cpp
@@ -106,18 +106,16 @@ volatile AHCI::PortRegisters& AHCIController::port(size_t port_number) const
 
 volatile AHCI::HBA& AHCIController::hba() const
 {
-    return static_cast<volatile AHCI::HBA&>(*(volatile AHCI::HBA*)(m_hba_region->vaddr().as_ptr()));
+    return const_cast<AHCI::HBA&>(*m_hba_mapping);
 }
 
 UNMAP_AFTER_INIT AHCIController::AHCIController(PCI::DeviceIdentifier const& pci_device_identifier)
     : ATAController()
     , PCI::Device(const_cast<PCI::DeviceIdentifier&>(pci_device_identifier))
-    , m_hba_region(default_hba_region())
-    , m_hba_capabilities(capabilities())
 {
 }
 
-AHCI::HBADefinedCapabilities AHCIController::capabilities() const
+UNMAP_AFTER_INIT AHCI::HBADefinedCapabilities AHCIController::capabilities() const
 {
     u32 capabilities = hba().control_regs.cap;
     u32 extended_capabilities = hba().control_regs.cap2;
@@ -154,15 +152,18 @@ AHCI::HBADefinedCapabilities AHCIController::capabilities() const
     };
 }
 
-UNMAP_AFTER_INIT NonnullOwnPtr<Memory::Region> AHCIController::default_hba_region() const
+UNMAP_AFTER_INIT ErrorOr<Memory::TypedMapping<AHCI::HBA volatile>> AHCIController::map_default_hba_region(PCI::DeviceIdentifier const& pci_device_identifier)
 {
-    return MM.allocate_kernel_region(PhysicalAddress(PCI::get_BAR5(device_identifier())).page_base(), Memory::page_round_up(sizeof(AHCI::HBA)).release_value_but_fixme_should_propagate_errors(), "AHCI HBA"sv, Memory::Region::Access::ReadWrite).release_value();
+    return Memory::map_typed_writable<AHCI::HBA volatile>(PhysicalAddress(PCI::get_BAR5(pci_device_identifier)));
 }
 
 AHCIController::~AHCIController() = default;
 
 UNMAP_AFTER_INIT ErrorOr<void> AHCIController::initialize_hba(PCI::DeviceIdentifier const& pci_device_identifier)
 {
+    m_hba_mapping = TRY(map_default_hba_region(pci_device_identifier));
+    m_hba_capabilities = capabilities();
+
     u32 version = hba().control_regs.version;
 
     hba().control_regs.ghc = 0x80000000; // Ensure that HBA knows we are AHCI aware.
diff --git a/Kernel/Storage/ATA/AHCI/Controller.h b/Kernel/Storage/ATA/AHCI/Controller.h
index 0dd5caba3f..b2405c703c 100644
--- a/Kernel/Storage/ATA/AHCI/Controller.h
+++ b/Kernel/Storage/ATA/AHCI/Controller.h
@@ -9,6 +9,7 @@
 #include <AK/OwnPtr.h>
 #include <AK/Types.h>
 #include <Kernel/Library/LockRefPtr.h>
+#include <Kernel/Memory/TypedMapping.h>
 #include <Kernel/Sections.h>
 #include <Kernel/Storage/ATA/AHCI/Definitions.h>
 #include <Kernel/Storage/ATA/ATAController.h>
@@ -49,11 +50,11 @@ private:
     LockRefPtr<StorageDevice> device_by_port(u32 index) const;
 
     volatile AHCI::PortRegisters& port(size_t port_number) const;
-    NonnullOwnPtr<Memory::Region> default_hba_region() const;
+    ErrorOr<Memory::TypedMapping<AHCI::HBA volatile>> map_default_hba_region(PCI::DeviceIdentifier const&);
     volatile AHCI::HBA& hba() const;
 
     Array<LockRefPtr<AHCIPort>, 32> m_ports;
-    NonnullOwnPtr<Memory::Region> m_hba_region;
+    Memory::TypedMapping<AHCI::HBA volatile> m_hba_mapping;
     AHCI::HBADefinedCapabilities m_hba_capabilities;
 
     // FIXME: There could be multiple IRQ (MSI) handlers for AHCI. Find a way to use all of them.