Misc code cleanup and more comments in AS map

app/src/main/cpp/skyline/common/address_space.inc

@@ -27,7 +27,7 @@ namespace skyline {
 
         auto blockEndSuccessor{std::lower_bound(blocks.begin(), blocks.end(), virtEnd)};
         if (blockEndSuccessor == blocks.begin())
-            throw exception("Unexpected Memory Manager state!");
+            throw exception("Trying to map a block before the VA start!");
 
         auto blockEndPredecessor{std::prev(blockEndSuccessor)};
 
@@ -36,7 +36,7 @@ namespace skyline {
             if (blockEndSuccessor->virt != virtEnd) {
                 PaType tailPhys{[&]() -> PaType {
                     if (!PaContigSplit || blockEndPredecessor->Unmapped())
-                        return blockEndPredecessor->phys; // Always propagate unmapped regions
+                        return blockEndPredecessor->phys; // Always propagate unmapped regions rather than calculating offset
                     else
                         return blockEndPredecessor->phys + virtEnd - blockEndPredecessor->virt;
                 }()};
@@ -70,14 +70,15 @@ namespace skyline {
         while (std::prev(blockStartSuccessor)->virt >= virt)
             std::advance(blockStartSuccessor, -1);
 
-        if (blockStartSuccessor->virt > virtEnd)
+        // Check that the start successor is either the end block or something in between
-            throw exception("Unexpected Memory Manager state!");
+        if (blockStartSuccessor->virt > virtEnd) {
-
+            throw exception("Unsorted block in AS map!");
-        if (blockStartSuccessor->virt == virtEnd) {
+        } else if (blockStartSuccessor->virt == virtEnd) {
             // We need to create a new block as there are none spare that we would overwrite
             blocks.insert(blockStartSuccessor, Block(virt, phys, flag));
             return;
         } else {
+            // Reuse a block that would otherwise be overwritten as a start block
             blockStartSuccessor->virt = virt;
             blockStartSuccessor->phys = phys;
             blockStartSuccessor->flag = flag;
@@ -85,7 +86,7 @@ namespace skyline {
             // Erase overwritten blocks
             if (auto eraseStart{std::next(blockStartSuccessor)}; blockStartSuccessor != blockEndPredecessor) {
                 if (eraseStart == blockEndPredecessor)
-                    __builtin_trap();
+                    throw exception("Trying to erase the end block of a newly mapped region!");
 
                 blocks.erase(eraseStart, blockEndPredecessor);
             }
@@ -98,11 +99,11 @@ namespace skyline {
         VaType virtEnd{virt + size};
 
         if (virtEnd > vaLimit)
-            throw exception("Trying to map a block past the VA limit!");
+            throw exception("Trying to unmap a block past the VA limit!");
 
         auto blockEndSuccessor{std::lower_bound(blocks.begin(), blocks.end(), virtEnd)};
         if (blockEndSuccessor == blocks.begin())
-            throw exception("Unexpected Memory Manager state!");
+            throw exception("Trying to unmap a block before the VA start!");
 
         auto blockEndPredecessor{std::prev(blockEndSuccessor)};
 
@@ -130,7 +131,7 @@ namespace skyline {
 
             // We can't have two unmapped regions after each other
             if (eraseEnd == blockStartSuccessor || (blockStartPredecessor->Unmapped() && eraseEnd->Unmapped()))
-                throw exception("Unexpected Memory Manager state!");
+                throw exception("Multiple contiguous unmapped regions are unsupported!");
 
             blocks.erase(blockStartSuccessor, eraseEnd);
         }};
@@ -172,10 +173,9 @@ namespace skyline {
         auto blockStartPredecessor{walkBackToPredecessor(blockEndPredecessor)};
         auto blockStartSuccessor{std::next(blockStartPredecessor)};
 
-        if (blockStartSuccessor->virt > virtEnd)
+        if (blockStartSuccessor->virt > virtEnd) {
-            throw exception("Unexpected Memory Manager state!");
+            throw exception("Unsorted block in AS map!");
-
+        } else if (blockStartSuccessor->virt == virtEnd) {
-        if (blockStartSuccessor->virt == virtEnd) {
             // There are no blocks between the start and the end that would let us skip inserting a new one for head
 
             // The previous block is may be unmapped, if so we don't need to insert any unmaps after it
@@ -192,7 +192,7 @@ namespace skyline {
             // Erase overwritten blocks, skipping the first one as we have written the unmapped start block there
             if (auto eraseStart{std::next(blockStartSuccessor)}; blockStartSuccessor != blockEndPredecessor) {
                 if (eraseStart == blockEndPredecessor)
-                    __builtin_trap();
+                    throw exception("Unexpected Memory Manager state!");
 
                 blocks.erase(eraseStart, blockEndPredecessor);
             }
@@ -225,6 +225,7 @@ namespace skyline {
         u8 *blockPhys{predecessor->phys + (virt - predecessor->virt)};
         VaType blockReadSize{std::min(successor->virt - virt, size)};
 
+        // Reads may span across multiple individual blocks
         while (size) {
             if (predecessor->phys == nullptr) {
                 if (predecessor->flag) // Sparse mapping
@@ -262,6 +263,7 @@ namespace skyline {
         u8 *blockPhys{predecessor->phys + (virt - predecessor->virt)};
         VaType blockWriteSize{std::min(successor->virt - virt, size)};
 
+        // Writes may span across multiple individual blocks
         while (size) {
             if (predecessor->phys == nullptr) {
                 if (!predecessor->flag) // Sparse mappings allow unmapped writes
@@ -292,15 +294,17 @@ namespace skyline {
         VaType allocStart{UnmappedVa};
         VaType allocEnd{currentLinearAllocEnd + size};
 
+        // Avoid searching backwards in the address space if possible
         if (allocEnd >= currentLinearAllocEnd && allocEnd <= this->vaLimit) {
             auto allocEndSuccessor{std::lower_bound(this->blocks.begin(), this->blocks.end(), allocEnd)};
             if (allocEndSuccessor == this->blocks.begin())
-                throw exception("Unexpected allocator state!");
+                throw exception("First block in AS map is invalid!");
 
             auto allocEndPredecessor{std::prev(allocEndSuccessor)};
             if (allocEndPredecessor->virt <= currentLinearAllocEnd) {
                 allocStart = currentLinearAllocEnd;
             } else {
+                // Skip over fixed any mappings in front of us
                 while (allocEndSuccessor != this->blocks.end()) {
                     if (allocEndSuccessor->virt - allocEndPredecessor->virt < size || allocEndPredecessor->Mapped() ) {
                         allocStart = allocEndPredecessor->virt;
@@ -309,6 +313,7 @@ namespace skyline {
 
                     allocEndPredecessor = allocEndSuccessor++;
 
+                    // Use the VA limit to calculate if we can fit in the final block since it has no successor
                     if (allocEndSuccessor == this->blocks.end()) {
                         allocEnd = allocEndPredecessor->virt + size;
 

app/src/main/cpp/skyline/services/nvdrv/devices/nvhost/as_gpu.cpp

@@ -12,6 +12,8 @@ namespace skyline {
 }
 
 namespace skyline::service::nvdrv::device::nvhost {
+    using GMMU = soc::gm20b::GM20B::GMMU;
+
     AsGpu::AsGpu(const DeviceState &state, Core &core, const SessionContext &ctx) : NvDevice(state, core, ctx) {}
 
     PosixResult AsGpu::BindChannel(In<FileDescriptor> channelFd) {
@@ -45,7 +47,7 @@ namespace skyline::service::nvdrv::device::nvhost {
         u64 size{static_cast<u64>(pages) * static_cast<u64>(pageSize)};
 
         if (flags.sparse)
-            state.soc->gm20b.gmmu.Map(offset, soc::gm20b::GM20B::GMMU::SparsePlaceholderAddress(), size, true);
+            state.soc->gm20b.gmmu.Map(offset, GMMU::SparsePlaceholderAddress(), size, true);
 
         allocationMap[offset] = {
             .size = size,
@@ -74,8 +76,10 @@ namespace skyline::service::nvdrv::device::nvhost {
                 allocator->Free(mapping->offset >> pageSizeBits, mapping->size >> pageSizeBits);
             }
 
+            // Sparse mappings shouldn't be fully unmapped, just returned to their sparse state
+            // Only FreeSpace can unmap them fully
             if (mapping->sparseAlloc)
-                state.soc->gm20b.gmmu.Map(offset, soc::gm20b::GM20B::GMMU::SparsePlaceholderAddress(), mapping->size, true);
+                state.soc->gm20b.gmmu.Map(offset, GMMU::SparsePlaceholderAddress(), mapping->size, true);
             else
                 state.soc->gm20b.gmmu.Unmap(offset, mapping->size);
 
@@ -93,6 +97,7 @@ namespace skyline::service::nvdrv::device::nvhost {
 
         state.logger->Debug("flags: ( fixed: {}, remap: {} ), kind: {}, handle: {}, bufferOffset: 0x{:X}, mappingSize: 0x{:X}, offset: 0x{:X}", flags.fixed, flags.remap, kind, handle, bufferOffset, mappingSize, offset);
 
+        // Remaps a subregion of an existing mapping to a different PA
         if (flags.remap) {
             try {
                 auto mapping{mappingMap.at(offset)};
@@ -206,6 +211,7 @@ namespace skyline::service::nvdrv::device::nvhost {
             vm.vaRangeStart = bigPageSize << VM::VaStartShift;
         }
 
+        // If this is unspecified then default values should be used
         if (vaRangeStart) {
             vm.vaRangeStart = vaRangeStart;
             vm.vaRangeSplit = vaRangeSplit;