diff --git a/src/graphics/host_gpu/memoryTracker.cpp b/src/graphics/host_gpu/memoryTracker.cpp index c5d6b6b2b..d853a86b6 100644 --- a/src/graphics/host_gpu/memoryTracker.cpp +++ b/src/graphics/host_gpu/memoryTracker.cpp @@ -18,8 +18,8 @@ MemoryTracker::~MemoryTracker() = default; #if KYTY_BUILD == KYTY_BUILD_DEBUG void MemoryTracker::ValidateGpuDirtyPages(const RangeSet& dirty, uint64_t vaddr, uint64_t size, const char* operation) const noexcept { - if (vaddr == 0 || size == 0 || size > UINT64_MAX - vaddr || - (vaddr & (TRACKER_PAGE_SIZE - 1)) != 0 || (size & (TRACKER_PAGE_SIZE - 1)) != 0) { + if (!GuestRange {vaddr, size}.Valid() || (vaddr & (TRACKER_PAGE_SIZE - 1)) != 0 || + (size & (TRACKER_PAGE_SIZE - 1)) != 0) { EXIT("MemoryTracker: invalid dirty-page validation range\n"); } for (auto page = vaddr; page < vaddr + size; page += TRACKER_PAGE_SIZE) { @@ -37,9 +37,6 @@ void MemoryTracker::ValidateGpuDirtyPages(const RangeSet& dirty, uint64_t vaddr, void MemoryTracker::ValidateGpuDirtyOwnership(const RangeSet& dirty, uint64_t vaddr, uint64_t size, const char* operation) { ValidateRange(vaddr, size); - if (vaddr + size > UINT64_MAX - (TRACKER_PAGE_SIZE - 1)) { - EXIT("MemoryTracker: dirty ownership range alignment overflow\n"); - } const auto begin = vaddr & ~(TRACKER_PAGE_SIZE - 1); const auto end = (vaddr + size + TRACKER_PAGE_SIZE - 1) & ~(TRACKER_PAGE_SIZE - 1); for (auto page = begin; page < end; page += TRACKER_PAGE_SIZE) { @@ -56,8 +53,7 @@ void MemoryTracker::ValidateGpuDirtyOwnership(const RangeSet& dirty, uint64_t va #endif void MemoryTracker::ValidateRange(uint64_t vaddr, uint64_t size) { - if (vaddr == 0 || size == 0 || vaddr >= TRACKER_ADDRESS_SIZE || - size > TRACKER_ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { EXIT("invalid memory tracker range\n"); } } diff --git a/src/graphics/host_gpu/pageManager.cpp b/src/graphics/host_gpu/pageManager.cpp index be911dbea..9abf1de82 100644 --- a/src/graphics/host_gpu/pageManager.cpp +++ b/src/graphics/host_gpu/pageManager.cpp @@ -110,7 +110,7 @@ private: }; void ValidateRange(uint64_t vaddr, uint64_t size) { - if (vaddr == 0 || size == 0 || vaddr >= ADDRESS_SIZE || size > ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { Fatal("invalid range vaddr=0x%016" PRIx64 ", size=0x%016" PRIx64, vaddr, size); } } diff --git a/src/graphics/host_gpu/regionDefinitions.h b/src/graphics/host_gpu/regionDefinitions.h index 1c5ee11c2..aa6c4e92d 100644 --- a/src/graphics/host_gpu/regionDefinitions.h +++ b/src/graphics/host_gpu/regionDefinitions.h @@ -4,6 +4,8 @@ #include "common/bitArray.h" #include "common/common.h" +#include + namespace Libs::Graphics { constexpr uint64_t TRACKER_PAGE_SIZE = 4ull * 1024ull; @@ -11,6 +13,20 @@ constexpr uint64_t TRACKER_REGION_SIZE = 4ull * 1024ull * 1024ull; constexpr uint64_t TRACKER_ADDRESS_SIZE = 1ull << 40u; constexpr size_t TRACKER_REGION_PAGES = TRACKER_REGION_SIZE / TRACKER_PAGE_SIZE; +struct GuestRange { + uint64_t address = 0; + uint64_t size = 0; + + [[nodiscard]] constexpr bool Empty() const noexcept { return address == 0 && size == 0; } + [[nodiscard]] constexpr bool Valid() const noexcept { + return address != 0 && size != 0 && address < TRACKER_ADDRESS_SIZE && + size <= TRACKER_ADDRESS_SIZE - address; + } + [[nodiscard]] constexpr bool ValidOrEmpty() const noexcept { return Empty() || Valid(); } + [[nodiscard]] constexpr uint64_t End() const noexcept { return address + size; } + auto operator<=>(const GuestRange&) const = default; +}; + enum class DirtySource { Cpu, Gpu }; using RegionBits = Common::BitArray; static_assert(sizeof(RegionBits) == TRACKER_REGION_PAGES / 8); diff --git a/src/graphics/host_gpu/renderer/cache/bufferCache.cpp b/src/graphics/host_gpu/renderer/cache/bufferCache.cpp index 4e689c71a..1a28c97b7 100644 --- a/src/graphics/host_gpu/renderer/cache/bufferCache.cpp +++ b/src/graphics/host_gpu/renderer/cache/bufferCache.cpp @@ -241,8 +241,7 @@ BufferCache::~BufferCache() { } void BufferCache::InvalidateMemory(uint64_t vaddr, uint64_t size) { - if (vaddr == 0 || size == 0 || vaddr >= TRACKER_ADDRESS_SIZE || - size > TRACKER_ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { EXIT("BufferCache: invalid memory-invalidation range\n"); } m_memory_tracker.InvalidateRegion(vaddr, size, @@ -314,7 +313,7 @@ BufferId BufferCache::FindBuffer(uint64_t vaddr, uint64_t size) { if (vaddr == 0) { return NULL_BUFFER_ID; } - if (size == 0 || vaddr >= TRACKER_ADDRESS_SIZE || size > TRACKER_ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { EXIT("BufferCache: invalid buffer discovery request\n"); } const auto* owner = m_page_table.Find(vaddr >> PageTable::kPageBits); @@ -443,8 +442,7 @@ std::pair BufferCache::ObtainBuffer(uint64_t vaddr, uint64_t bool is_written, bool is_texel_buffer, BufferId id) { auto& command = m_scheduler.Current(); - if (command.IsInvalid() || vaddr == 0 || size == 0 || - vaddr >= TRACKER_ADDRESS_SIZE || size > TRACKER_ADDRESS_SIZE - vaddr) { + if (command.IsInvalid() || !GuestRange {vaddr, size}.Valid()) { EXIT("BufferCache: buffer request requires a recording command buffer\n"); } @@ -474,8 +472,7 @@ std::pair BufferCache::ObtainBuffer(uint64_t vaddr, uint64_t } std::pair BufferCache::ObtainBufferForImage(uint64_t vaddr, uint64_t size) { - if (vaddr == 0 || size == 0 || vaddr >= TRACKER_ADDRESS_SIZE || - size > TRACKER_ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { EXIT("BufferCache: invalid image source\n"); } auto find_owner = [&]() -> Buffer* { @@ -661,8 +658,7 @@ void BufferCache::CopyBuffer(uint64_t dst_vaddr, uint64_t src_vaddr, uint64_t si } bool BufferCache::IsRegionRegistered(uint64_t vaddr, uint64_t size) { - if (vaddr == 0 || size == 0 || vaddr >= TRACKER_ADDRESS_SIZE || - size > TRACKER_ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { EXIT("BufferCache: invalid registered-region query\n"); } // Cached buffers are ordered and non-overlapping. The last buffer beginning before the query diff --git a/src/graphics/host_gpu/renderer/cache/gpuResourceManager.cpp b/src/graphics/host_gpu/renderer/cache/gpuResourceManager.cpp index 0af87b190..f49c63536 100644 --- a/src/graphics/host_gpu/renderer/cache/gpuResourceManager.cpp +++ b/src/graphics/host_gpu/renderer/cache/gpuResourceManager.cpp @@ -35,8 +35,7 @@ bool GpuResourceManager::InvalidateMemory(uint64_t vaddr, uint64_t size) { } bool GpuResourceManager::IsMapped(uint64_t vaddr, uint64_t size) const noexcept { - if (vaddr == 0 || size == 0 || vaddr >= TRACKER_ADDRESS_SIZE || - size > TRACKER_ADDRESS_SIZE - vaddr) { + if (!GuestRange {vaddr, size}.Valid()) { return false; } std::shared_lock lock(m_mapped_ranges_mutex); diff --git a/src/graphics/host_gpu/renderer/colorRenderTarget.cpp b/src/graphics/host_gpu/renderer/colorRenderTarget.cpp index b7fdacd3d..8aa19abea 100644 --- a/src/graphics/host_gpu/renderer/colorRenderTarget.cpp +++ b/src/graphics/host_gpu/renderer/colorRenderTarget.cpp @@ -309,7 +309,7 @@ void RenderExecutor::ResolveRenderColorTarget(uint64_t submit_id, CommandBuffer& if (backing_size == 0) { EXIT("render-target backing is empty\n"); } - if (backing_size > TRACKER_ADDRESS_SIZE - rt.base.addr) { + if (!GuestRange {rt.base.addr, backing_size}.Valid()) { EXIT("render-target backing range is invalid\n"); } diff --git a/src/graphics/host_gpu/renderer/depthRenderTarget.cpp b/src/graphics/host_gpu/renderer/depthRenderTarget.cpp index d6d2dca6a..e541b9f58 100644 --- a/src/graphics/host_gpu/renderer/depthRenderTarget.cpp +++ b/src/graphics/host_gpu/renderer/depthRenderTarget.cpp @@ -260,9 +260,9 @@ void RenderExecutor::ResolveRenderDepthTarget(uint64_t submit_id, CommandBuffer& const auto depth_backing_size = depth_size.size * view.image_layers; const auto stencil_backing_size = stencil_size.size * view.image_layers; const auto htile_backing_size = htile_size.size * view.image_layers; - if (depth_backing_size > TRACKER_ADDRESS_SIZE - z.z_read_base_addr || - (has_stencil && stencil_backing_size > TRACKER_ADDRESS_SIZE - z.stencil_read_base_addr) || - (has_htile && htile_backing_size > TRACKER_ADDRESS_SIZE - z.htile_data_base_addr)) { + if (!GuestRange {z.z_read_base_addr, depth_backing_size}.Valid() || + (has_stencil && !GuestRange {z.stencil_read_base_addr, stencil_backing_size}.Valid()) || + (has_htile && !GuestRange {z.htile_data_base_addr, htile_backing_size}.Valid())) { DepthFatal("layered depth backing range is invalid"); } r.htile = has_htile; diff --git a/src/graphics/host_gpu/renderer/image/image.cpp b/src/graphics/host_gpu/renderer/image/image.cpp index 735d1eaac..a5fae54ac 100644 --- a/src/graphics/host_gpu/renderer/image/image.cpp +++ b/src/graphics/host_gpu/renderer/image/image.cpp @@ -73,10 +73,8 @@ namespace { return usage; } -void ValidateRange(GuestRange range, const char* name) { - if ((range.address == 0) != (range.size == 0) || - (range.address != 0 && (range.address >= TRACKER_ADDRESS_SIZE || - range.size > TRACKER_ADDRESS_SIZE - range.address))) { +void ValidateOptionalRange(GuestRange range, const char* name) { + if (!range.ValidOrEmpty()) { EXIT("invalid %s image range: address=0x%016llx size=0x%016llx\n", name, static_cast(range.address), static_cast(range.size)); @@ -567,13 +565,13 @@ void Image::CopyMip(Image& source, uint32_t mip, uint32_t layer) { namespace ImageOps { void Validate(const ImageInfo& info) { - ValidateRange(info.data, "data"); - ValidateRange(info.stencil, "stencil"); + ValidateOptionalRange(info.data, "data"); + ValidateOptionalRange(info.stencil, "stencil"); if (info.pixel_format == vk::Format::eUndefined) { const bool metadata_empty = - info.metadata.range.address == 0 && info.metadata.range.size == 0 && - info.metadata.kind == ImageMetadataKind::None && info.metadata.control == 0 && + info.metadata.range.Empty() && info.metadata.kind == ImageMetadataKind::None && + info.metadata.control == 0 && info.metadata.compression == VideoOutCompression::Uncompressed && !info.metadata.stencil_compressed; if (info.data.Empty() || info.HasStencil() || !metadata_empty || info.extent.width == 0 || @@ -620,16 +618,14 @@ void Validate(const ImageInfo& info) { } switch (info.metadata.kind) { case ImageMetadataKind::None: - if (info.metadata.range.address != 0 || info.metadata.range.size != 0 || - info.metadata.control != 0 || + if (!info.metadata.range.Empty() || info.metadata.control != 0 || info.metadata.compression != VideoOutCompression::Uncompressed || info.metadata.stencil_compressed) { EXIT("metadata-free image has metadata state\n"); } break; case ImageMetadataKind::Htile: - ValidateRange(info.metadata.range, "HTILE"); - if (info.metadata.range.Empty() || + if (!info.metadata.range.Valid() || info.metadata.compression != VideoOutCompression::Uncompressed) { EXIT("invalid HTILE metadata\n"); } diff --git a/src/graphics/host_gpu/renderer/image/imageInfo.h b/src/graphics/host_gpu/renderer/image/imageInfo.h index 7b7fb100c..77e71f4f1 100644 --- a/src/graphics/host_gpu/renderer/image/imageInfo.h +++ b/src/graphics/host_gpu/renderer/image/imageInfo.h @@ -15,18 +15,6 @@ namespace Libs::Graphics { -struct GuestRange { - uint64_t address = 0; - uint64_t size = 0; - - [[nodiscard]] constexpr bool Empty() const noexcept { return address == 0 || size == 0; } - [[nodiscard]] constexpr bool Valid() const noexcept { - return !Empty() && address < TRACKER_ADDRESS_SIZE && size <= TRACKER_ADDRESS_SIZE - address; - } - [[nodiscard]] constexpr uint64_t End() const noexcept { return address + size; } - auto operator<=>(const GuestRange&) const = default; -}; - enum class VideoOutCompression : uint8_t { Uncompressed, Dcc256_256_0, Dcc256_64_64, Unsupported }; enum class ImageMetadataKind : uint8_t { None, Htile, Dcc }; diff --git a/tests/MemoryTrackerTests.cpp b/tests/MemoryTrackerTests.cpp index 994cac14d..96345a431 100644 --- a/tests/MemoryTrackerTests.cpp +++ b/tests/MemoryTrackerTests.cpp @@ -29,9 +29,11 @@ namespace { +using Libs::Graphics::GuestRange; using Libs::Graphics::MemoryTracker; using Libs::Graphics::PageManager; using Libs::Graphics::RangeSet; +using Libs::Graphics::TRACKER_ADDRESS_SIZE; void Check(bool value, const char *text) { if (!value) { @@ -208,6 +210,27 @@ void TestRangeSet() { "range set subtraction did not preserve both exact tails"); } +void TestGuestRange() { + constexpr GuestRange empty{}; + constexpr GuestRange first_byte{1, 1}; + constexpr GuestRange last_byte{TRACKER_ADDRESS_SIZE - 1, 1}; + + static_assert(empty.Empty() && !empty.Valid() && empty.ValidOrEmpty()); + static_assert(!first_byte.Empty() && first_byte.Valid() && + first_byte.ValidOrEmpty() && first_byte.End() == 2); + static_assert(last_byte.Valid() && last_byte.End() == TRACKER_ADDRESS_SIZE); + + Check(!GuestRange{0, 1}.Empty() && !GuestRange{0, 1}.ValidOrEmpty(), + "zero-address nonempty guest range is rejected"); + Check(!GuestRange{1, 0}.Empty() && !GuestRange{1, 0}.ValidOrEmpty(), + "nonzero-address empty guest range is rejected"); + Check(!GuestRange{TRACKER_ADDRESS_SIZE, 1}.Valid(), + "first address beyond the guest range is rejected"); + Check(!GuestRange{TRACKER_ADDRESS_SIZE - 1, 2}.Valid(), + "guest range crossing the address-space end is rejected"); + Check(!GuestRange{UINT64_MAX, 2}.Valid(), "wrapping guest range is rejected"); +} + void TestQueriesDoNotRequireMappedOwnership() { constexpr uint64_t address = 0x0000000203000000ull; TrackerHarness harness; @@ -829,6 +852,7 @@ int main(int argc, char **argv) { if (argc == 3 && std::strcmp(argv[1], "--death") == 0) { RunDeathCase(argv[2]); } + TestGuestRange(); TestRangeSet(); TestQueriesDoNotRequireMappedOwnership(); TestConcurrentRegionPublication();