From cbfbd9f4b81d9f4b17db4163cbff4a2b0b1ddd22 Mon Sep 17 00:00:00 2001 From: Nav Date: Sat, 7 Dec 2024 16:43:16 +0000 Subject: [PATCH] Applied debug-interface-specific access restrictions for memory and registers --- .../Protocols/EDBG/AVR/EdbgAvr8Interface.cpp | 10 +++++----- .../Protocols/EDBG/AVR/EdbgAvr8Interface.hpp | 4 ++-- .../Microchip/AVR8/Avr8DebugInterface.hpp | 7 +++---- .../RiscV/RiscVDebugInterface.hpp | 10 ++++++++++ .../WCH/WchLinkDebugInterface.cpp | 14 ++++++++++++++ .../WCH/WchLinkDebugInterface.hpp | 3 +++ src/Targets/Microchip/AVR8/Avr8.cpp | 19 ++++++------------- src/Targets/Microchip/AVR8/Avr8.hpp | 2 +- src/Targets/RiscV/RiscV.cpp | 18 ++++++++++++++++++ src/Targets/RiscV/RiscV.hpp | 6 ++++++ src/Targets/RiscV/Wch/WchRiscV.cpp | 10 ++++++++++ 11 files changed, 78 insertions(+), 25 deletions(-) diff --git a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp index 96dbf05a..fa119fe8 100644 --- a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp +++ b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp @@ -270,8 +270,8 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr } } - Targets::TargetRegisterAccess EdbgAvr8Interface::getRegisterAccess( - const TargetRegisterDescriptor& registerDescriptor, + void EdbgAvr8Interface::applyAccessRestrictions( + TargetRegisterDescriptor& registerDescriptor, const TargetAddressSpaceDescriptor& addressSpaceDescriptor ) { /* @@ -281,10 +281,10 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr * There are some other memory types we can use to access some other registers during a debug session, but * these are yet to be implemented. */ - const auto access = addressSpaceDescriptor.key == this->session.dataAddressSpace.key + const auto accessible = addressSpaceDescriptor.key == this->session.dataAddressSpace.key || addressSpaceDescriptor.key == this->session.registerFileAddressSpace.key; - - return {access, access}; + registerDescriptor.access.readable = accessible && registerDescriptor.access.readable; + registerDescriptor.access.writable = accessible && registerDescriptor.access.writable; } TargetMemoryAddress EdbgAvr8Interface::getProgramCounter() { diff --git a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp index f74a5dad..6db9c155 100644 --- a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp +++ b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp @@ -138,8 +138,8 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr */ void deactivate() override; - Targets::TargetRegisterAccess getRegisterAccess( - const Targets::TargetRegisterDescriptor& registerDescriptor, + void applyAccessRestrictions( + Targets::TargetRegisterDescriptor& registerDescriptor, const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor ) override; diff --git a/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR8/Avr8DebugInterface.hpp b/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR8/Avr8DebugInterface.hpp index 3b7825a3..308879c2 100644 --- a/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR8/Avr8DebugInterface.hpp +++ b/src/DebugToolDrivers/TargetInterfaces/Microchip/AVR8/Avr8DebugInterface.hpp @@ -70,8 +70,7 @@ namespace DebugToolDrivers::TargetInterfaces::Microchip::Avr8 * registers can only be accessed during a programming session. This restriction is specific to the EDBG debug * interface. * - * This function should communicate any access restrictions for the given register, which apply during a debug - * session. It does not need to account for any access restrictions that only apply outside of a debug session. + * This function should apply any additional access restrictions for the given register. * * @param registerDescriptor * The descriptor of the register. @@ -81,8 +80,8 @@ namespace DebugToolDrivers::TargetInterfaces::Microchip::Avr8 * * @return */ - virtual Targets::TargetRegisterAccess getRegisterAccess( - const Targets::TargetRegisterDescriptor& registerDescriptor, + virtual void applyAccessRestrictions( + Targets::TargetRegisterDescriptor& registerDescriptor, const Targets::TargetAddressSpaceDescriptor& addressSpaceDescriptor ) = 0; diff --git a/src/DebugToolDrivers/TargetInterfaces/RiscV/RiscVDebugInterface.hpp b/src/DebugToolDrivers/TargetInterfaces/RiscV/RiscVDebugInterface.hpp index bde8a89f..e5ce5289 100644 --- a/src/DebugToolDrivers/TargetInterfaces/RiscV/RiscVDebugInterface.hpp +++ b/src/DebugToolDrivers/TargetInterfaces/RiscV/RiscVDebugInterface.hpp @@ -59,5 +59,15 @@ namespace DebugToolDrivers::TargetInterfaces::RiscV virtual void enableProgrammingMode() = 0; virtual void disableProgrammingMode() = 0; + + /** + * The debug interface may have its own access restrictions for memory segments and registers. These member + * functions should adjust the access members of the given segment/register descriptor, to apply any additional + * access restrictions. + * + * The member functions are called as part of the construction of the RISC-V target descriptor. + */ + virtual void applyAccessRestrictions(Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor) = 0; + virtual void applyAccessRestrictions(Targets::TargetRegisterDescriptor& registerDescriptor) = 0; }; } diff --git a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp index 4bd6c4a9..6c5208e9 100644 --- a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp +++ b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp @@ -357,6 +357,20 @@ namespace DebugToolDrivers::Wch this->softwareBreakpointRegistry.clear(); } + void WchLinkDebugInterface::applyAccessRestrictions(TargetMemorySegmentDescriptor& memorySegmentDescriptor) { + if (memorySegmentDescriptor.type == TargetMemorySegmentType::FLASH) { + /* + * Actually, we *can* write to flash memory whilst in debug mode (via a partial page write), but we don't + * need to, so I'm just going to block it, for now. + */ + memorySegmentDescriptor.debugModeAccess.writeable = false; + } + } + + void WchLinkDebugInterface::applyAccessRestrictions(Targets::TargetRegisterDescriptor& registerDescriptor) { + // I don't believe any further access restrictions are required for registers. TODO: Review after v2.0.0. + } + void WchLinkDebugInterface::setSoftwareBreakpoint(const TargetProgramBreakpoint& breakpoint) { if (breakpoint.size != 2 && breakpoint.size != 4) { throw Exception{"Invalid software breakpoint size (" + std::to_string(breakpoint.size) + ")"}; diff --git a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp index d8aa23f4..b7d0f385 100644 --- a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp +++ b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp @@ -78,6 +78,9 @@ namespace DebugToolDrivers::Wch void enableProgrammingMode() override; void disableProgrammingMode() override; + void applyAccessRestrictions(Targets::TargetMemorySegmentDescriptor& memorySegmentDescriptor) override; + void applyAccessRestrictions(Targets::TargetRegisterDescriptor& registerDescriptor) override; + private: const WchLinkToolConfig& toolConfig; const Targets::RiscV::RiscVTargetConfig& targetConfig; diff --git a/src/Targets/Microchip/AVR8/Avr8.cpp b/src/Targets/Microchip/AVR8/Avr8.cpp index 1f5238b0..8972ff5d 100644 --- a/src/Targets/Microchip/AVR8/Avr8.cpp +++ b/src/Targets/Microchip/AVR8/Avr8.cpp @@ -328,7 +328,7 @@ namespace Targets::Microchip::Avr8 // The debug interface may have its own access restrictions for registers. for (auto& [peripheralKey, peripheral] : descriptor.peripheralDescriptorsByKey) { for (auto& [groupKey, registerGroup] : peripheral.registerGroupDescriptorsByKey) { - this->applyDebugInterfaceRegisterAccessRestrictions( + this->applyDebugInterfaceAccessRestrictions( registerGroup, descriptor.getAddressSpaceDescriptor(registerGroup.addressSpaceKey) ); @@ -781,23 +781,16 @@ namespace Targets::Microchip::Avr8 this->writeRegisters({{descriptor, value}}); } - void Avr8::applyDebugInterfaceRegisterAccessRestrictions( + void Avr8::applyDebugInterfaceAccessRestrictions( TargetRegisterGroupDescriptor& groupDescriptor, const TargetAddressSpaceDescriptor& addressSpaceDescriptor ) { - for (auto& [subgroupKey, subgroupDescriptor] : groupDescriptor.subgroupDescriptorsByKey) { - this->applyDebugInterfaceRegisterAccessRestrictions(subgroupDescriptor, addressSpaceDescriptor); + for (auto& [registerKey, registerDescriptor] : groupDescriptor.registerDescriptorsByKey) { + this->avr8DebugInterface->applyAccessRestrictions(registerDescriptor, addressSpaceDescriptor); } - for (auto& [registerKey, registerDescriptor] : groupDescriptor.registerDescriptorsByKey) { - if (!registerDescriptor.access.readable && !registerDescriptor.access.writable) { - // This register is already inaccessible - no need to check for further restrictions - continue; - } - - const auto access = this->avr8DebugInterface->getRegisterAccess(registerDescriptor, addressSpaceDescriptor); - registerDescriptor.access.readable = registerDescriptor.access.readable && access.readable; - registerDescriptor.access.writable = registerDescriptor.access.writable && access.writable; + for (auto& [subgroupKey, subgroupDescriptor] : groupDescriptor.subgroupDescriptorsByKey) { + this->applyDebugInterfaceAccessRestrictions(subgroupDescriptor, addressSpaceDescriptor); } } diff --git a/src/Targets/Microchip/AVR8/Avr8.hpp b/src/Targets/Microchip/AVR8/Avr8.hpp index 175ccdd1..a190e6c0 100644 --- a/src/Targets/Microchip/AVR8/Avr8.hpp +++ b/src/Targets/Microchip/AVR8/Avr8.hpp @@ -169,7 +169,7 @@ namespace Targets::Microchip::Avr8 TargetMemoryBuffer readRegister(const TargetRegisterDescriptor& descriptor); void writeRegister(const TargetRegisterDescriptor& descriptor, const TargetMemoryBuffer& value) ; - void applyDebugInterfaceRegisterAccessRestrictions( + void applyDebugInterfaceAccessRestrictions( TargetRegisterGroupDescriptor& groupDescriptor, const TargetAddressSpaceDescriptor& addressSpaceDescriptor ); diff --git a/src/Targets/RiscV/RiscV.cpp b/src/Targets/RiscV/RiscV.cpp index a7608018..2b356026 100644 --- a/src/Targets/RiscV/RiscV.cpp +++ b/src/Targets/RiscV/RiscV.cpp @@ -307,6 +307,22 @@ namespace Targets::RiscV return this->programmingMode; } + void RiscV::applyDebugInterfaceAccessRestrictions(TargetAddressSpaceDescriptor& addressSpaceDescriptor) { + for (auto& [segmentKey, segmentDescriptor] : addressSpaceDescriptor.segmentDescriptorsByKey) { + this->riscVDebugInterface->applyAccessRestrictions(segmentDescriptor); + } + } + + void RiscV::applyDebugInterfaceAccessRestrictions(TargetRegisterGroupDescriptor& registerGroupDescriptor) { + for (auto& [registerKey, registerDescriptor] : registerGroupDescriptor.registerDescriptorsByKey) { + this->riscVDebugInterface->applyAccessRestrictions(registerDescriptor); + } + + for (auto& [subgroupKey, subgroupDescriptor] : registerGroupDescriptor.subgroupDescriptorsByKey) { + this->applyDebugInterfaceAccessRestrictions(subgroupDescriptor); + } + } + const TargetMemorySegmentDescriptor& RiscV::resolveRegisterMemorySegmentDescriptor( const TargetRegisterDescriptor& regDescriptor, const TargetAddressSpaceDescriptor& addressSpaceDescriptor @@ -355,6 +371,7 @@ namespace Targets::RiscV false, {true, true}, {false, false}, + false, std::nullopt } ); @@ -371,6 +388,7 @@ namespace Targets::RiscV false, {true, true}, {false, false}, + false, std::nullopt } ); diff --git a/src/Targets/RiscV/RiscV.hpp b/src/Targets/RiscV/RiscV.hpp index 1ae1fd51..c2487279 100644 --- a/src/Targets/RiscV/RiscV.hpp +++ b/src/Targets/RiscV/RiscV.hpp @@ -11,6 +11,9 @@ #include "TargetDescriptionFile.hpp" #include "IsaDescriptor.hpp" +#include "src/Targets/TargetAddressSpaceDescriptor.hpp" +#include "src/Targets/TargetRegisterGroupDescriptor.hpp" + #include "src/DebugToolDrivers/TargetInterfaces/RiscV/RiscVDebugInterface.hpp" namespace Targets::RiscV @@ -119,6 +122,9 @@ namespace Targets::RiscV bool programmingMode = false; + void applyDebugInterfaceAccessRestrictions(TargetAddressSpaceDescriptor& addressSpaceDescriptor); + void applyDebugInterfaceAccessRestrictions(TargetRegisterGroupDescriptor& registerGroupDescriptor); + const TargetMemorySegmentDescriptor& resolveRegisterMemorySegmentDescriptor( const TargetRegisterDescriptor& regDescriptor, const TargetAddressSpaceDescriptor& addressSpaceDescriptor diff --git a/src/Targets/RiscV/Wch/WchRiscV.cpp b/src/Targets/RiscV/Wch/WchRiscV.cpp index c0be949f..b3940b19 100644 --- a/src/Targets/RiscV/Wch/WchRiscV.cpp +++ b/src/Targets/RiscV/Wch/WchRiscV.cpp @@ -78,6 +78,16 @@ namespace Targets::RiscV::Wch this->cpuPeripheralDescriptor.clone() ); + for (auto& [addressSpaceKey, addressSpaceDescriptor] : descriptor.addressSpaceDescriptorsByKey) { + this->applyDebugInterfaceAccessRestrictions(addressSpaceDescriptor); + } + + for (auto& [peripheralKey, peripheralDescriptor] : descriptor.peripheralDescriptorsByKey) { + for (auto& [groupKey, groupDescriptor] : peripheralDescriptor.registerGroupDescriptorsByKey) { + this->applyDebugInterfaceAccessRestrictions(groupDescriptor); + } + } + auto& sysAddressSpaceDescriptor = descriptor.getAddressSpaceDescriptor("system"); sysAddressSpaceDescriptor.getMemorySegmentDescriptor("internal_program_memory").inspectionEnabled = true; sysAddressSpaceDescriptor.getMemorySegmentDescriptor("internal_ram").inspectionEnabled = true;