Merge pull request #5662 from blubberdiblub/improve_mmu_mask_checks

Improve MMU mask checks

3 files changed, 59 insertions, 14 deletions

diff --git a/Source/Core/Common/BitUtils.h b/Source/Core/Common/BitUtils.h
index eacf6bf099..8d5398fb39 100644
--- a/Source/Core/Common/BitUtils.h
+++ b/Source/Core/Common/BitUtils.h
@@ -99,4 +99,26 @@ constexpr Result ExtractBits(const T src) noexcept
 
   return ExtractBits<T, Result>(src, begin, end);
 }
+
+///
+/// Verifies whether the supplied value is a valid bit mask of the form 0b00...0011...11.
+/// Both edge cases of all zeros and all ones are considered valid masks, too.
+///
+/// @param  mask The mask value to test for validity.
+///
+/// @tparam T    The type of the value.
+///
+/// @return A bool indicating whether the mask is valid.
+///
+template <typename T>
+constexpr bool IsValidLowMask(const T mask) noexcept
+{
+  static_assert(std::is_integral<T>::value, "Mask must be an integral type.");
+  static_assert(std::is_unsigned<T>::value, "Signed masks can introduce hard to find bugs.");
+
+  // Can be efficiently determined without looping or bit counting. It's the counterpart
+  // to https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
+  // and doesn't require special casing either edge case.
+  return (mask & (mask + 1)) == 0;
+}
 }  // namespace Common
diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp
index b5e2448617..ad218b4244 100644
--- a/Source/Core/Core/PowerPC/MMU.cpp
+++ b/Source/Core/Core/PowerPC/MMU.cpp
@@ -7,7 +7,7 @@
 #include <string>
 
 #include "Common/Atomic.h"
-#include "Common/BitSet.h"
+#include "Common/BitUtils.h"
 #include "Common/CommonTypes.h"
 
 #include "Core/ConfigManager.h"
@@ -947,26 +947,17 @@ static void GenerateISIException(u32 _EffectiveAddress)
 void SDRUpdated()
 {
   u32 htabmask = SDR1_HTABMASK(PowerPC::ppcState.spr[SPR_SDR]);
-  u32 x = 1;
-  u32 xx = 0;
-  int n = 0;
-  while ((htabmask & x) && (n < 9))
-  {
-    n++;
-    xx |= x;
-    x <<= 1;
-  }
-  if (htabmask & ~xx)
+  if (!Common::IsValidLowMask(htabmask))
   {
     return;
   }
   u32 htaborg = SDR1_HTABORG(PowerPC::ppcState.spr[SPR_SDR]);
-  if (htaborg & xx)
+  if (htaborg & htabmask)
   {
     return;
   }
   PowerPC::ppcState.pagetable_base = htaborg << 16;
-  PowerPC::ppcState.pagetable_hashmask = ((xx << 10) | 0x3ff);
+  PowerPC::ppcState.pagetable_hashmask = ((htabmask << 10) | 0x3ff);
 }
 
 enum TLBLookupResult
@@ -1175,7 +1166,7 @@ static void UpdateBATs(BatTable& bat_table, u32 base_spr)
       // implemented this way for invalid BATs as well.
       WARN_LOG(POWERPC, "Bad BAT setup: BPRN overlaps BL");
     }
-    if (CountSetBits((u32)(batu.BL + 1)) != 1)
+    if (!Common::IsValidLowMask((u32)batu.BL))
     {
       // With a valid BAT, the simplest way of masking is
       // (input & ~BL_mask) for matching and (input & BL_mask) for
diff --git a/Source/UnitTests/Common/BitUtilsTest.cpp b/Source/UnitTests/Common/BitUtilsTest.cpp
index 987868e6e4..75f7adf50e 100644
--- a/Source/UnitTests/Common/BitUtilsTest.cpp
+++ b/Source/UnitTests/Common/BitUtilsTest.cpp
@@ -57,3 +57,35 @@ TEST(BitUtils, ExtractBits)
   // Ensure bit extraction with type overriding works as expected
   EXPECT_EQ((Common::ExtractBits<0, 31, s32, s32>(negative_one)), -1);
 }
+
+TEST(BitUtils, IsValidLowMask)
+{
+  EXPECT_TRUE(Common::IsValidLowMask(0b0u));
+  EXPECT_TRUE(Common::IsValidLowMask(0b1u));
+  EXPECT_FALSE(Common::IsValidLowMask(0b10u));
+  EXPECT_TRUE(Common::IsValidLowMask(0b11u));
+  EXPECT_FALSE(Common::IsValidLowMask(0b1110u));
+  EXPECT_TRUE(Common::IsValidLowMask(0b1111u));
+  EXPECT_FALSE(Common::IsValidLowMask(0b10000u));
+  EXPECT_FALSE(Common::IsValidLowMask(0b101111u));
+
+  EXPECT_TRUE(Common::IsValidLowMask((u8)~0b0));
+  EXPECT_FALSE(Common::IsValidLowMask((u8)(~0b0 - 1)));
+  EXPECT_FALSE(Common::IsValidLowMask((u8)~(0b10000)));
+  EXPECT_FALSE(Common::IsValidLowMask((u8)(~((u8)(~0b0) >> 1) | 0b1111)));
+
+  EXPECT_TRUE(Common::IsValidLowMask((u16)~0b0));
+  EXPECT_FALSE(Common::IsValidLowMask((u16)(~0b0 - 1)));
+  EXPECT_FALSE(Common::IsValidLowMask((u16)~(0b10000)));
+  EXPECT_FALSE(Common::IsValidLowMask((u16)(~((u16)(~0b0) >> 1) | 0b1111)));
+
+  EXPECT_TRUE(Common::IsValidLowMask((u32)~0b0));
+  EXPECT_FALSE(Common::IsValidLowMask((u32)(~0b0 - 1)));
+  EXPECT_FALSE(Common::IsValidLowMask((u32)~(0b10000)));
+  EXPECT_FALSE(Common::IsValidLowMask((u32)(~((u32)(~0b0) >> 1) | 0b1111)));
+
+  EXPECT_TRUE(Common::IsValidLowMask((u64)~0b0));
+  EXPECT_FALSE(Common::IsValidLowMask((u64)(~0b0 - 1)));
+  EXPECT_FALSE(Common::IsValidLowMask((u64)~(0b10000)));
+  EXPECT_FALSE(Common::IsValidLowMask((u64)(~((u64)(~0b0) >> 1) | 0b1111)));
+}