Merge pull request #5926 from xXPBSXx/fix/agc-vertex-fetch-overhang

fix(libSceAgcDriver): zero the overhanging record of a vertex fetch

core/libs/prx/libSceAgcDriver/Graphics/include/Draw.hpp

@@ -29,7 +29,7 @@ struct DrawInputCopy {
     std::uint64_t registryGeneration = 0;
 };
 DrawInputCopy CopyDrawInput(const Context& context, Recorder* recorder, std::uint64_t address, std::size_t bytes, std::size_t alignment, Recorder::SnapshotUse use);
-DrawInputCopy CopyZeroPaddedDrawInput(const Context& context, std::uint64_t address, std::size_t bytes, std::size_t validBytes);
+DrawInputCopy CopyZeroPaddedDrawInput(const Context& context, std::uint64_t address, std::size_t bytes, std::size_t validBytes, std::size_t stride = 0);
 void KeepDrawInput(Recorder* recorder, std::uint64_t address, const DrawInputCopy& copy, Recorder::SnapshotUse use, std::uint32_t derived);
 const HostImport* InPlaceDrawInput(const Context& context, std::uint64_t address, std::size_t bytes, std::size_t alignment, bool* behindGpuWrite = nullptr);
 struct InPlaceIndices {

core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp

@@ -947,14 +947,20 @@ DrawInputCopy CopyDrawInput(const Context& context, Recorder* recorder, std::uin
     return copy;
 }
 
-DrawInputCopy CopyZeroPaddedDrawInput(const Context& context, std::uint64_t address, std::size_t bytes, std::size_t validBytes) {
+DrawInputCopy CopyZeroPaddedDrawInput(const Context& context, std::uint64_t address, std::size_t bytes, std::size_t validBytes, std::size_t stride) {
     Require(validBytes <= bytes, "the valid bytes of a vertex fetch exceed the fetch");
     GuestMemory::FlushGpuWrites(address, bytes);
     DrawInputCopy copy;
     copy.buffer = std::make_shared<Buffer>(context, bytes, VK_BUFFER_USAGE_VERTEX_BUFFER_BIT);
     const auto span = copy.buffer->Bytes();
-    GuestMemory::Read(address, span.subspan(0, validBytes), 1);
+    auto readable = validBytes;
-    std::fill(span.begin() + static_cast<std::ptrdiff_t>(validBytes), span.end(), std::byte{0});
+    if (stride != 0 && !GuestMemory::Accessible(reinterpret_cast<const void*>(address), validBytes)) {
+        const auto ranges = GuestMemory::CommittedRanges(address, validBytes);
+        const std::size_t prefix = !ranges.empty() && ranges.front().first == address ? static_cast<std::size_t>(ranges.front().second - address) : 0;
+        if (validBytes - prefix < stride) readable = prefix;
+    }
+    GuestMemory::Read(address, span.subspan(0, readable), 1);
+    std::fill(span.begin() + static_cast<std::ptrdiff_t>(readable), span.end(), std::byte{0});
     return copy;
 }
 
@@ -1250,7 +1256,7 @@ DrawInputs prepareDrawInputs(const Context& context, const State& state, const P
     for (const auto f : soloFetches) {
         const auto begin = fetches[f].begin;
         const auto bytes = static_cast<std::size_t>(fetches[f].end - begin);
-        auto copy = CopyZeroPaddedDrawInput(context, begin, bytes, fetchValid[f]);
+        auto copy = CopyZeroPaddedDrawInput(context, begin, bytes, fetchValid[f], fetches[f].stride);
         soloHandles[f] = copy.buffer->Handle();
         inputs.vertexBuffers.push_back(std::move(copy.buffer));
     }

core/libs/prx/libSceAgcDriver/tests/Graphics.cpp

@@ -28,6 +28,11 @@
 #include "ImageTableAbi.hpp"
 #include "SpirvBackend/SpirvSpecialization.hpp"
 #include <spirv/unified1/spirv.hpp>
+#ifdef _WIN32
+#include <windows.h>
+#else
+#include <sys/mman.h>
+#endif
 #include <algorithm>
 #include <array>
 #include <atomic>
@@ -8765,6 +8770,39 @@ void vertexZeroFillTests() {
     Require(std::all_of(bytes.begin() + static_cast<std::ptrdiff_t>(valid), bytes.end(), [](std::byte value) { return value == std::byte{0}; }), "a vertex fetch past the record count did not read as zeros");
 }
 
+void vertexOverhangTests() {
+    using AgcDriver::Graphics::CopyZeroPaddedDrawInput;
+    constexpr std::size_t page = 65536;
+    constexpr std::size_t stride = 44;
+    constexpr std::size_t overhang = 8;
+#ifdef _WIN32
+    auto* block = static_cast<std::byte*>(VirtualAlloc(nullptr, page * 2, MEM_RESERVE, PAGE_NOACCESS));
+    Require(block != nullptr && VirtualAlloc(block, page, MEM_COMMIT, PAGE_READWRITE) != nullptr, "cannot reserve the guest pages of the overhang test");
+#else
+    void* mapped = mmap(nullptr, page * 2, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    Require(mapped != MAP_FAILED && mprotect(mapped, page, PROT_READ | PROT_WRITE) == 0, "cannot reserve the guest pages of the overhang test");
+    auto* block = static_cast<std::byte*>(mapped);
+#endif
+    constexpr std::size_t offset = 0x650;
+    const auto readable = page - offset;
+    std::fill(block + offset, block + page, std::byte{0x7a});
+    const auto address = reinterpret_cast<std::uint64_t>(block) + offset;
+    {
+        const auto copy = CopyZeroPaddedDrawInput(mockContext(), address, readable + overhang, readable + overhang, stride);
+        const auto bytes = copy.buffer->Bytes();
+        Require(bytes.size() == readable + overhang, "an overhanging vertex fetch changed its size");
+        Require(std::all_of(bytes.begin(), bytes.begin() + static_cast<std::ptrdiff_t>(readable), [](std::byte value) { return value == std::byte{0x7a}; }), "an overhanging vertex fetch did not copy the readable bytes");
+        Require(std::all_of(bytes.begin() + static_cast<std::ptrdiff_t>(readable), bytes.end(), [](std::byte value) { return value == std::byte{0}; }), "an overhanging vertex fetch did not zero its last record");
+    }
+    expectFailure([&] { CopyZeroPaddedDrawInput(mockContext(), address, readable + stride, readable + stride, stride); }, "is not readable");
+    expectFailure([&] { CopyZeroPaddedDrawInput(mockContext(), address, readable + overhang, readable + overhang, 0); }, "is not readable");
+#ifdef _WIN32
+    VirtualFree(block, 0, MEM_RELEASE);
+#else
+    munmap(block, page * 2);
+#endif
+}
+
 void vertexZeroFillMergeTests() {
     using AgcDriver::Graphics::PlanVertexCopies;
     using AgcDriver::Graphics::SoloZeroPaddedFetchIndices;
@@ -9460,6 +9498,7 @@ int main(int argc, char** argv) {
         storeAtLabelTests();
         nullVertexDescriptorTests();
         vertexZeroFillTests();
+        vertexOverhangTests();
         vertexZeroFillMergeTests();
         parallelCompareTests();
         pixelParameterSlotTests();

docs/dev/debt/libSceAgcDriver.md

@@ -30,6 +30,7 @@
 - [Strided buffer overhang](../../../core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp) (libSceAgcDriver) - a strided buffer descriptor that ends less than one stride past the end of the readable registered allocation holding its base is bound only up to that allocation's end, even when another readable allocation starts right there. This assumes the bytes past the end are the rounded-up `num_records` of the last record (RADV rounds it up), which no fetch inside the buffer reads; a guest that really keeps a vertex split across two adjacent allocations would read zeros for the cut tail. Taken from the RetroArch Dolphin core case in #2912, not measured on hardware.
 - [Buffer descriptor CACHE_SWIZZLE hint](../../../core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp) (libSceAgcDriver) - bit 30 of word 1 in a buffer resource descriptor (`words[1] & 0x40000000u`) is normalized to zero when binding guest buffers in `ShaderResources::addGuestBuffer`. On GFX10 (gfx1013), bit 30 of V# word 1 is `CACHE_SWIZZLE` (Mesa's `gfx10-rsrc.json`: STRIDE [29:16], CACHE_SWIZZLE [30], SWIZZLE_ENABLE [31]), a cache-bank hint with no effect on the address or data, which is dropped here.
 - [Buffer descriptors past the range limit](../../../core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp) (libSceAgcDriver) - a buffer V# whose size is over `maxStorageBufferRange` is bound only up to the end of the readable registered allocation holding its base when that allocation ends before the limit, and as the empty placeholder buffer, like a null V#, when no registered allocation holds its base; a V# over an allocation that reaches the limit still throws. Titles leave such words in descriptor slots their shaders do not reach on the path taken (PPSA10528 binds `c8010002 2c6bec20 c0053c00 04000013`, about 33 TB from an address above the user address space). What the console returns for an access past the bound range is not known; here the device's robust buffer access decides. Follows the review of #3202, not measured on hardware.
+- [Vertex fetch overhang](../../../core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp) (libSceAgcDriver) - a zero-padded vertex fetch whose valid range (`num_records * stride`) ends less than one stride past the end of the readable memory at its base copies only the readable bytes and reads the rest as zeros, instead of failing the draw; a longer overhang still fails. This extends the strided buffer overhang rule above (the last record is the rounded-up `num_records`) to the staging copy and is not measured on hardware. The observed case is a fetch of 14858 records of 44 bytes that ends 8 bytes past the end of the 0xa0000-byte mapping holding it.
 - [Zero-stride vertex fetch range](../../../core/libs/prx/libSceAgcDriver/Graphics/include/VertexInput.hpp) (libSceAgcDriver) - for a stride-0 V# with OOB_SELECT 2 the whole fetch reads memory unless NUM_RECORDS is 0; with OOB_SELECT 3 it reads zero when NUM_RECORDS is 0 and throws when the fetch only partly fits. Rules taken from the GFX10 OOB_SELECT table in Mesa [ac_descriptors.c](https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/amd/common/ac_descriptors.c), not measured on hardware. Whether OOB_SELECT 3 checks each dword of a multi-dword fetch separately is unknown. OOB_SELECT 0 and 1 keep the structured record checks.
 - [Resource registration](../../../core/libs/prx/libSceAgcDriver/Resource/src/Registration.cpp) (libSceAgcDriver) - registration always fails with `RESOURCE_REGISTRATION_UNAVAILABLE`, so the owner and resource queries, `sceAgcDriverSetResourceUserData`, `sceAgcDriverFindResourcesPublic` and `sceAgcDriverUnregisterAllResourcesForOwner` return the same error without reading their arguments. Their parameter lists follow the libSceGnmDriver counterparts and are unverified. `sceAgcDriverGetResourceRegistrationMaxNameLength` writes 0xfc and returns 0, as [prosper](https://github.com/mattias800/prosper) does from a title's registration configuration; the `uint32_t*` parameter matches the SDK wrapper `sce::Agc::ResourceRegistration::getMaxNameLength(unsigned int*)`. sharpemu writes 256 and KytyPS5 returns `RESOURCE_REGISTRATION_UNAVAILABLE`, so the value is unverified; a null output throws
 - [sceAgcDriverRegisterMultipleResources](../../../core/libs/prx/libSceAgcDriver/Resource/src/Registration.cpp) uses the existing unavailable-registration state and leaves caller memory untouched. Its four arguments (uint32 handle output, uint32 owner, descriptor pointer, uint32 count) follow the original PPSA34502 / 01.010.000 codzm call. The caller builds 40-byte descriptors and explicitly accepts 0x8A6C9018 with an invalid output handle. Enabled registration and console argument-validation precedence are unverified.