Kernel/riscv64: Don't hard-code the page fault reason on RISC-V

Instead, rewrite the region page fault handling code to not use
PageFault::type() on RISC-V.

I split Region::handle_fault into having a RISC-V-specific
implementation, as I am not sure if I cover all page fault handling edge
cases by solely relying on MM's own region metadata.
We should probably also take the processor-provided page fault reason
into account, if we decide to merge these two implementations in the
future.
This commit is contained in:
Sönke Holz 2024-03-03 14:54:10 +01:00 committed by Andrew Kaster
parent 496a7541a2
commit 6654021655
Notes: sideshowbarker 2024-07-16 22:51:10 +09:00
5 changed files with 60 additions and 7 deletions

View file

@ -120,7 +120,8 @@ void PageFault::handle(RegisterState& regs)
auto fault_address_string = KString::formatted("{:p}", fault_address);
auto fault_address_view = fault_address_string.is_error() ? ""sv : fault_address_string.value()->view();
(void)current_process.try_set_coredump_property("fault_address"sv, fault_address_view);
(void)current_process.try_set_coredump_property("fault_type"sv, type() == PageFault::Type::PageNotPresent ? "NotPresent"sv : "ProtectionViolation"sv);
if (type() != PageFault::Type::Unknown)
(void)current_process.try_set_coredump_property("fault_type"sv, type() == PageFault::Type::PageNotPresent ? "NotPresent"sv : "ProtectionViolation"sv);
StringView fault_access;
if (is_instruction_fetch())
fault_access = "Execute"sv;

View file

@ -50,6 +50,7 @@ public:
enum class Type {
PageNotPresent = PageFaultFlags::NotPresent,
ProtectionViolation = PageFaultFlags::ProtectionViolation,
Unknown,
};
enum class Access {
@ -90,7 +91,7 @@ public:
bool is_instruction_fetch() const { return m_is_instruction_fetch; }
private:
Type m_type;
Type m_type = Type::Unknown;
Access m_access;
ExecutionMode m_execution_mode;
bool m_is_reserved_bit_violation { false };

View file

@ -96,9 +96,8 @@ extern "C" void trap_handler(TrapFrame& trap_frame)
else if (scause == StoreOrAMOPageFault)
fault.set_access(PageFault::Access::Write);
// FIXME: RISC-V doesn't tell you *why* a memory access failed, only the original access type (r/w/x).
// So either detect the correct type by walking the page table or rewrite MM to not depend on the processor-provided reason.
fault.set_type(PageFault::Type::ProtectionViolation);
// RISC-V doesn't tell you the reason why a page fault occurred, so we don't use PageFault::set_type() here.
// The RISC-V implementation of Region::handle_fault() works without a correct PageFault::type().
fault.handle(*trap_frame.regs);
break;

View file

@ -358,6 +358,7 @@ void Region::clear_to_zero()
PageFaultResponse Region::handle_fault(PageFault const& fault)
{
#if !ARCH(RISCV64)
auto page_index_in_region = page_index_from_address(fault.vaddr());
if (fault.type() == PageFault::Type::PageNotPresent) {
if (fault.is_read() && !is_readable()) {
@ -404,6 +405,57 @@ PageFaultResponse Region::handle_fault(PageFault const& fault)
}
dbgln("PV(error) fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr());
return PageFaultResponse::ShouldCrash;
#else
// FIXME: Consider to merge the RISC-V page fault handling code with the x86_64/aarch64 implementation.
// RISC-V doesn't tell you *why* a memory access failed, only the original access type (r/w/x).
// We probably should take the page fault reason into account, if the processor provides it.
auto page_index_in_region = page_index_from_address(fault.vaddr());
if (fault.is_read() && !is_readable()) {
dbgln("Read page fault in non-readable Region({})[{}]", this, page_index_in_region);
return PageFaultResponse::ShouldCrash;
}
if (fault.is_write() && !is_writable()) {
dbgln("Write page fault in non-writable Region({})[{}] at {}", this, page_index_in_region, fault.vaddr());
return PageFaultResponse::ShouldCrash;
}
if (fault.is_instruction_fetch() && !is_executable()) {
dbgln("Instruction fetch page fault in non-executable Region({})[{}] at {}", this, page_index_in_region, fault.vaddr());
return PageFaultResponse::ShouldCrash;
}
if (fault.is_write() && is_writable() && should_cow(page_index_in_region)) {
dbgln_if(PAGE_FAULT_DEBUG, "CoW page fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr());
auto phys_page = physical_page(page_index_in_region);
if (phys_page->is_shared_zero_page() || phys_page->is_lazy_committed_page()) {
dbgln_if(PAGE_FAULT_DEBUG, "Zero page fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr());
return handle_zero_fault(page_index_in_region, *phys_page);
}
return handle_cow_fault(page_index_in_region);
}
if (vmobject().is_inode()) {
dbgln_if(PAGE_FAULT_DEBUG, "Inode page fault in Region({})[{}]", this, page_index_in_region);
return handle_inode_fault(page_index_in_region);
}
SpinlockLocker vmobject_locker(vmobject().m_lock);
auto& page_slot = physical_page_slot(page_index_in_region);
if (page_slot->is_lazy_committed_page()) {
auto page_index_in_vmobject = translate_to_vmobject_page(page_index_in_region);
VERIFY(m_vmobject->is_anonymous());
page_slot = static_cast<AnonymousVMObject&>(*m_vmobject).allocate_committed_page({});
if (!remap_vmobject_page(page_index_in_vmobject, *page_slot))
return PageFaultResponse::OutOfMemory;
return PageFaultResponse::Continue;
}
dbgln("Unexpected page fault in Region({})[{}] at {}", this, page_index_in_region, fault.vaddr());
return PageFaultResponse::ShouldCrash;
#endif
}
PageFaultResponse Region::handle_zero_fault(size_t page_index_in_region, PhysicalPage& page_in_slot_at_time_of_fault)

View file

@ -81,8 +81,8 @@ static TitleAndText build_backtrace(Coredump::Reader const& coredump, ELF::Core:
auto fault_address = metadata.get("fault_address");
auto fault_type = metadata.get("fault_type");
auto fault_access = metadata.get("fault_access");
if (fault_address.has_value() && fault_type.has_value() && fault_access.has_value()) {
builder.appendff("{} fault on {} at address {}", fault_type.value(), fault_access.value(), fault_address.value());
if (fault_address.has_value() && fault_access.has_value()) {
builder.appendff("{} fault on {} at address {}", fault_type.value_or("Page"), fault_access.value(), fault_address.value());
constexpr FlatPtr malloc_scrub_pattern = explode_byte(MALLOC_SCRUB_BYTE);
constexpr FlatPtr free_scrub_pattern = explode_byte(FREE_SCRUB_BYTE);
auto raw_fault_address = AK::StringUtils::convert_to_uint_from_hex(fault_address.value().substring_view(2));