Revert of [telemetry] Read MSRs from a separate privileged process. (patchset...

Revert of [telemetry] Read MSRs from a separate privileged process. (patchset #3 id:40001 of https://codereview.chromium.org/578123002/)

Reason for revert:
Fails telemetry_unittests and telemetry_perf_unittests on XP Tests (2) http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds/35314

Traceback (most recent call last):
  run at E:\b\depot_tools\python276_bin\lib\unittest\case.py:331
    testMethod()
  testPowerMonitoringSync at tools\telemetry\telemetry\core\platform\platform_backend_unittest.py:16
    if not platform.CanMonitorPower():
  CanMonitorPower at tools\telemetry\telemetry\core\platform\__init__.py:248
    return self._platform_backend.CanMonitorPower()
  CanMonitorPower at tools\telemetry\telemetry\core\platform\win_platform_backend.py:313
    return self._power_monitor.CanMonitorPower()
  CanMonitorPower at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor.py:51
    if self._PackageEnergyJoules() <= 0:
  _PackageEnergyJoules at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor.py:97
    return (self._backend.ReadMsr(MSR_PKG_ENERGY_STATUS) *
  ReadMsr at tools\telemetry\telemetry\core\platform\win_platform_backend.py:347
    self._StartMsrServerIfNeeded()
  _StartMsrServerIfNeeded at tools\telemetry\telemetry\core\platform\win_platform_backend.py:338
    sys.executable, parameters, elevate_privilege=True)
  LaunchApplication at tools\telemetry\telemetry\core\platform\win_platform_backend.py:293
    if elevate_privilege and not IsCurrentProcessElevated():
  IsCurrentProcessElevated at tools\telemetry\telemetry\core\platform\win_platform_backend.py:80
    win32security.GetTokenInformation(token, win32security.TokenElevation))
error: (87, 'GetTokenInformation', 'The parameter is incorrect.')

Locals:
  handle : -1
  token  : <PyHANDLE:0>

[  FAILED  ] platform.platform_backend_unittest.PlatformBackendTest.testPowerMonitoringSync (3422 ms)

Traceback (most recent call last):
  run at E:\b\depot_tools\python276_bin\lib\unittest\case.py:331
    testMethod()
  wrapper at tools\telemetry\telemetry\decorators.py:84
    func(*args, **kwargs)
  testMsrRuns at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor_unittest.py:19
    if not power_monitor.CanMonitorPower():
  CanMonitorPower at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor.py:51
    if self._PackageEnergyJoules() <= 0:
  _PackageEnergyJoules at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor.py:97
    return (self._backend.ReadMsr(MSR_PKG_ENERGY_STATUS) *
  ReadMsr at tools\telemetry\telemetry\core\platform\win_platform_backend.py:347
    self._StartMsrServerIfNeeded()
  _StartMsrServerIfNeeded at tools\telemetry\telemetry\core\platform\win_platform_backend.py:338
    sys.executable, parameters, elevate_privilege=True)
  LaunchApplication at tools\telemetry\telemetry\core\platform\win_platform_backend.py:293
    if elevate_privilege and not IsCurrentProcessElevated():
  IsCurrentProcessElevated at tools\telemetry\telemetry\core\platform\win_platform_backend.py:80
    win32security.GetTokenInformation(token, win32security.TokenElevation))
error: (87, 'GetTokenInformation', 'The parameter is incorrect.')

Locals:
  handle : -1
  token  : <PyHANDLE:0>

[  FAILED  ] platform.power_monitor.msr_power_monitor_unittest.MsrPowerMonitorTest.testMsrRuns (32 ms)
[----------] 1 test (32 ms total)

Traceback (most recent call last):
  run at E:\b\depot_tools\python276_bin\lib\unittest\case.py:331
    testMethod()
  wrapper at tools\telemetry\telemetry\decorators.py:55
    func(*args, **kwargs)
  BenchmarkSmokeTest at tools\perf\benchmarks\benchmark_unittest.py:59
    self.assertEqual(0, SinglePageBenchmark().Run(options),
  Run at tools\telemetry\telemetry\benchmark.py:95
    page_runner.Run(pt, ps, expectations, finder_options, results)
  Run at tools\telemetry\telemetry\page\page_runner.py:429
    page, credentials_path, possible_browser, results, state)
  _PrepareAndRunPage at tools\telemetry\telemetry\page\page_runner.py:259
    finder_options)
  StartBrowserIfNeeded at tools\telemetry\telemetry\page\page_runner.py:54
    test.WillStartBrowser(possible_browser.platform)
  WillStartBrowser at tools\perf\measurements\webrtc.py:21
    self._power_metric = power.PowerMetric(platform)
  __init__ at tools\perf\metrics\power.py:31
    self._MeasureQuiescentPower(quiescent_measurement_time_s)
  _MeasureQuiescentPower at tools\perf\metrics\power.py:54
    if not self._platform.CanMonitorPower() or \
  CanMonitorPower at tools\telemetry\telemetry\core\platform\__init__.py:248
    return self._platform_backend.CanMonitorPower()
  CanMonitorPower at tools\telemetry\telemetry\core\platform\win_platform_backend.py:313
    return self._power_monitor.CanMonitorPower()
  CanMonitorPower at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor.py:51
    if self._PackageEnergyJoules() <= 0:
  _PackageEnergyJoules at tools\telemetry\telemetry\core\platform\power_monitor\msr_power_monitor.py:97
    return (self._backend.ReadMsr(MSR_PKG_ENERGY_STATUS) *
  ReadMsr at tools\telemetry\telemetry\core\platform\win_platform_backend.py:347
    self._StartMsrServerIfNeeded()
  _StartMsrServerIfNeeded at tools\telemetry\telemetry\core\platform\win_platform_backend.py:338
    sys.executable, parameters, elevate_privilege=True)
  LaunchApplication at tools\telemetry\telemetry\core\platform\win_platform_backend.py:293
    if elevate_privilege and not IsCurrentProcessElevated():
  IsCurrentProcessElevated at tools\telemetry\telemetry\core\platform\win_platform_backend.py:80
    win32security.GetTokenInformation(token, win32security.TokenElevation))
error: (87, 'GetTokenInformation', 'The parameter is incorrect.')

Locals:
  handle : -1
  token  : <PyHANDLE:0>

[  FAILED  ] BenchmarkSmokeTest.webrtc.webrtc_cases (766 ms)

Original issue's description:
> [telemetry] Read MSRs from a separate privileged process.
>
> This allows us to read MSRs on Windows Vista+.
> Also move ReadMsr into PlatformBackend.
>
> BUG=336558
> TEST=tools/telemetry/run_tests msr
>
> Committed: https://crrev.com/2156b0323c83d6f11e68344ecf7e7e0c833cfa14
> Cr-Commit-Position: refs/heads/master@{#295634}

TBR=tonyg@chromium.org,thestig@chromium.org,dtu@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=336558

Review URL: https://codereview.chromium.org/582123003

Cr-Commit-Position: refs/heads/master@{#295678}

tools/telemetry/telemetry/core/platform/msr_server_win.py

-# Copyright 2014 The Chromium Authors. All rights reserved.
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-
-"""A server that serves MSR values over TCP. Takes a port as its sole parameter.
-
-The reference client for this server is msr_power_monitor.MsrPowerMonitor.
-
-Must be run as Administrator. We use TCP instead of named pipes or another IPC
-to avoid dealing with the pipe security mechanisms. We take the port as a
-parameter instead of choosing one, because it's hard to communicate the port
-number across integrity levels.
-
-Requires WinRing0 to be installed in the Python directory.
-msr_power_monitor.MsrPowerMonitor does this if needed.
-"""
-
-import argparse
-import ctypes
-import os
-import SocketServer
-import struct
-import sys
-
-
-WINRING0_STATUS_MESSAGES = (
-    'No error',
-    'Unsupported platform',
-    'Driver not loaded. You may need to run as Administrator',
-    'Driver not found',
-    'Driver unloaded by other process',
-    'Driver not loaded because of executing on Network Drive',
-    'Unknown error',
-)
-
-
-# The DLL initialization is global, so put it in a global variable.
-_winring0 = None
-
-
-class WinRing0Error(OSError):
-  pass
-
-
-def _WinRing0Path():
-  python_is_64_bit = sys.maxsize > 2 ** 32
-  dll_file_name = 'WinRing0x64.dll' if python_is_64_bit else 'WinRing0.dll'
-  return os.path.join(os.path.dirname(sys.executable), dll_file_name)
-
-
-def _Initialize():
-  global _winring0
-  if not _winring0:
-    winring0 = ctypes.WinDLL(_WinRing0Path())
-    if not winring0.InitializeOls():
-      winring0_status = winring0.GetDllStatus()
-      raise WinRing0Error(winring0_status,
-                          'Unable to initialize WinRing0: %s' %
-                          WINRING0_STATUS_MESSAGES[winring0_status])
-    _winring0 = winring0
-
-
-def _Deinitialize():
-  global _winring0
-  if _winring0:
-    _winring0.DeinitializeOls()
-    _winring0 = None
-
-
-def _ReadMsr(msr_number):
-  low = ctypes.c_uint()
-  high = ctypes.c_uint()
-  _winring0.Rdmsr(ctypes.c_uint(msr_number),
-                  ctypes.byref(low), ctypes.byref(high))
-  return high.value << 32 | low.value
-
-
-class MsrRequestHandler(SocketServer.StreamRequestHandler):
-  def handle(self):
-    msr_number = struct.unpack('I', self.rfile.read(4))[0]
-    self.wfile.write(struct.pack('Q', _ReadMsr(msr_number)))
-
-
-def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument('port', type=int)
-  args = parser.parse_args()
-
-  _Initialize()
-  try:
-    SocketServer.TCPServer.allow_reuse_address = True
-    server_address = ('127.0.0.1', args.port)
-    server = SocketServer.TCPServer(server_address, MsrRequestHandler)
-    server.serve_forever()
-  finally:
-    _Deinitialize()
-
-
-if __name__ == '__main__':
-  main()

tools/telemetry/telemetry/core/platform/platform_backend.py

@@ -207,11 +207,3 @@ class PlatformBackend(object):
 
   def StopMonitoringPower(self):
     raise NotImplementedError()
-
-  def ReadMsr(self, msr_number):
-    """Read a CPU model-specific register (MSR).
-
-    Which MSRs are available depends on the CPU model.
-    On systems with multiple CPUs, this function may run on any CPU.
-    """
-    raise NotImplementedError()

tools/telemetry/telemetry/core/platform/power_monitor/msr_power_monitor.py

@@ -2,12 +2,19 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import atexit
+import ctypes
 import logging
+import os
 import platform
 import re
+import sys
+import zipfile
 
 from telemetry import decorators
 from telemetry.core.platform import power_monitor
+from telemetry.util import cloud_storage
+from telemetry.util import path
 
 
 MSR_RAPL_POWER_UNIT = 0x606
@@ -19,6 +26,100 @@ IA32_PACKAGE_THERM_STATUS = 0x1b1
 IA32_TEMPERATURE_TARGET = 0x1a2
 
 
+WINRING0_STATUS_MESSAGES = (
+    'No error',
+    'Unsupported platform',
+    'Driver not loaded. You may need to run as Administrator',
+    'Driver not found',
+    'Driver unloaded by other process',
+    'Driver not loaded because of executing on Network Drive',
+    'Unknown error',
+)
+
+
+# The DLL initialization is global, so put it in a global variable.
+_winring0 = None
+
+
+class WinRing0Error(OSError):
+  pass
+
+
+@decorators.Cache
+def WinRing0Path():
+  python_is_64_bit = sys.maxsize > 2 ** 32
+  win_binary_dir = os.path.join(path.GetTelemetryDir(), 'bin', 'win')
+  dll_file_name = 'WinRing0x64.dll' if python_is_64_bit else 'WinRing0.dll'
+  dll_path = os.path.join(win_binary_dir, dll_file_name)
+
+  os_is_64_bit = 'PROGRAMFILES(X86)' in os.environ
+  executable_dir = os.path.dirname(sys.executable)
+  driver_file_name = 'WinRing0x64.sys' if os_is_64_bit else 'WinRing0.sys'
+  driver_path = os.path.join(executable_dir, driver_file_name)
+
+  # Check for WinRing0 and download if needed.
+  if not (os.path.exists(dll_path) and os.path.exists(driver_path)):
+    zip_path = os.path.join(win_binary_dir, 'winring0.zip')
+    cloud_storage.GetIfChanged(zip_path, bucket=cloud_storage.PUBLIC_BUCKET)
+    try:
+      with zipfile.ZipFile(zip_path, 'r') as zip_file:
+        # Install DLL.
+        if not os.path.exists(dll_path):
+          zip_file.extract(dll_file_name, win_binary_dir)
+        # Install kernel driver.
+        if not os.path.exists(driver_path):
+          zip_file.extract(driver_file_name, executable_dir)
+    finally:
+      os.remove(zip_path)
+
+  return dll_path
+
+
+def _Initialize():
+  global _winring0
+  if not _winring0:
+    winring0 = ctypes.WinDLL(WinRing0Path())
+    if not winring0.InitializeOls():
+      winring0_status = winring0.GetDllStatus()
+      raise WinRing0Error(winring0_status,
+                          'Unable to initialize WinRing0: %s' %
+                          WINRING0_STATUS_MESSAGES[winring0_status])
+    _winring0 = winring0
+    atexit.register(_Deinitialize)
+
+
+def _Deinitialize():
+  global _winring0
+  if _winring0:
+    _winring0.DeinitializeOls()
+    _winring0 = None
+
+
+def _ReadMsr(msr_number):
+  low = ctypes.c_uint()
+  high = ctypes.c_uint()
+  _winring0.Rdmsr(ctypes.c_uint(msr_number),
+                  ctypes.byref(low), ctypes.byref(high))
+  return high.value << 32 | low.value
+
+
+@decorators.Cache
+def _EnergyMultiplier():
+  return 0.5 ** ((_ReadMsr(MSR_RAPL_POWER_UNIT) >> 8) & 0x1f)
+
+
+def _PackageEnergyJoules():
+  return _ReadMsr(MSR_PKG_ENERGY_STATUS) * _EnergyMultiplier()
+
+
+def _TemperatureCelsius():
+  tcc_activation_temp = _ReadMsr(IA32_TEMPERATURE_TARGET) >> 16 & 0x7f
+  if tcc_activation_temp <= 0:
+    tcc_activation_temp = 105
+  package_temp_headroom = _ReadMsr(IA32_PACKAGE_THERM_STATUS) >> 16 & 0x7f
+  return tcc_activation_temp - package_temp_headroom
+
+
 def _JoulesToMilliwattHours(value_joules):
   return value_joules * 1000 / 3600.
 
@@ -48,15 +149,16 @@ class MsrPowerMonitor(power_monitor.PowerMonitor):
       return False
 
     try:
-      if self._PackageEnergyJoules() <= 0:
-        logging.info('Cannot monitor power: no energy readings.')
-        return False
-
-      if self._TemperatureCelsius() <= 0:
-        logging.info('Cannot monitor power: no temperature readings.')
-        return False
-    except OSError as e:
-      logging.info('Cannot monitor power: %s' % e)
+      _Initialize()
+    except OSError:
+      return False
+
+    if _PackageEnergyJoules() <= 0:
+      logging.info('Cannot monitor power: no energy readings.')
+      return False
+
+    if _TemperatureCelsius() <= 0:
+      logging.info('Cannot monitor power: no temperature readings.')
       return False
 
     return True
@@ -64,15 +166,16 @@ class MsrPowerMonitor(power_monitor.PowerMonitor):
   def StartMonitoringPower(self, browser):
     assert self._start_energy_j is None and self._start_temp_c is None, (
         'Called StartMonitoringPower() twice.')
-    self._start_energy_j = self._PackageEnergyJoules()
-    self._start_temp_c = self._TemperatureCelsius()
+    _Initialize()
+    self._start_energy_j = _PackageEnergyJoules()
+    self._start_temp_c = _TemperatureCelsius()
 
   def StopMonitoringPower(self):
     assert not(self._start_energy_j is None or self._start_temp_c is None), (
         'Called StopMonitoringPower() before StartMonitoringPower().')
 
-    energy_consumption_j = self._PackageEnergyJoules() - self._start_energy_j
-    average_temp_c = (self._TemperatureCelsius() + self._start_temp_c) / 2.
+    energy_consumption_j = _PackageEnergyJoules() - self._start_energy_j
+    average_temp_c = (_TemperatureCelsius() + self._start_temp_c) / 2.
     assert energy_consumption_j >= 0, ('Negative energy consumption. (Starting '
                                        'energy was %s.)' % self._start_energy_j)
 
@@ -88,20 +191,3 @@ class MsrPowerMonitor(power_monitor.PowerMonitor):
             },
         },
     }
-
-  @decorators.Cache
-  def _EnergyMultiplier(self):
-    return 0.5 ** ((self._backend.ReadMsr(MSR_RAPL_POWER_UNIT) >> 8) & 0x1f)
-
-  def _PackageEnergyJoules(self):
-    return (self._backend.ReadMsr(MSR_PKG_ENERGY_STATUS) *
-            self._EnergyMultiplier())
-
-  def _TemperatureCelsius(self):
-    tcc_activation_temp = (
-        self._backend.ReadMsr(IA32_TEMPERATURE_TARGET) >> 16 & 0x7f)
-    if tcc_activation_temp <= 0:
-      tcc_activation_temp = 105
-    package_temp_headroom = (
-        self._backend.ReadMsr(IA32_PACKAGE_THERM_STATUS) >> 16 & 0x7f)
-    return tcc_activation_temp - package_temp_headroom

tools/telemetry/telemetry/core/platform/power_monitor/msr_power_monitor_unittest.py

@@ -3,7 +3,6 @@
 # found in the LICENSE file.
 
 import logging
-import time
 import unittest
 
 from telemetry import decorators
@@ -13,7 +12,12 @@ from telemetry.core.platform.power_monitor import msr_power_monitor
 
 class MsrPowerMonitorTest(unittest.TestCase):
   @decorators.Enabled('win')
-  def testMsrRuns(self):
+  def testFindOrInstallWinRing0(self):
+    self.assertTrue(msr_power_monitor.WinRing0Path())
+
+  @decorators.Enabled('win')
+  def testMsrRunsWithoutErrors(self):
+    # Very basic test, doesn't validate any output data.
     platform_backend = win_platform_backend.WinPlatformBackend()
     power_monitor = msr_power_monitor.MsrPowerMonitor(platform_backend)
     if not power_monitor.CanMonitorPower():
@@ -21,9 +25,7 @@ class MsrPowerMonitorTest(unittest.TestCase):
       return
 
     power_monitor.StartMonitoringPower(None)
-    time.sleep(0.01)
     statistics = power_monitor.StopMonitoringPower()
 
     self.assertEqual(statistics['identifier'], 'msr')
     self.assertIn('energy_consumption_mwh', statistics)
-    self.assertGreater(statistics['energy_consumption_mwh'], 0)

tools/telemetry/telemetry/core/platform/win_platform_backend.py

@@ -2,28 +2,19 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-import atexit
 import collections
 import contextlib
 import ctypes
-import os
 import platform
 import re
-import socket
-import struct
 import subprocess
-import sys
 import time
-import zipfile
 
 from telemetry import decorators
 from telemetry.core import exceptions
-from telemetry.core import util
 from telemetry.core.platform import desktop_platform_backend
 from telemetry.core.platform import platform_backend
 from telemetry.core.platform.power_monitor import msr_power_monitor
-from telemetry.util import cloud_storage
-from telemetry.util import path
 
 try:
   import pywintypes  # pylint: disable=F0401
@@ -43,34 +34,6 @@ except ImportError:
   win32security = None
 
 
-def _InstallWinRing0():
-  """WinRing0 is used for reading MSRs."""
-  executable_dir = os.path.dirname(sys.executable)
-
-  python_is_64_bit = sys.maxsize > 2 ** 32
-  dll_file_name = 'WinRing0x64.dll' if python_is_64_bit else 'WinRing0.dll'
-  dll_path = os.path.join(executable_dir, dll_file_name)
-
-  os_is_64_bit = 'PROGRAMFILES(X86)' in os.environ
-  driver_file_name = 'WinRing0x64.sys' if os_is_64_bit else 'WinRing0.sys'
-  driver_path = os.path.join(executable_dir, driver_file_name)
-
-  # Check for WinRing0 and download if needed.
-  if not (os.path.exists(dll_path) and os.path.exists(driver_path)):
-    win_binary_dir = os.path.join(path.GetTelemetryDir(), 'bin', 'win')
-    zip_path = os.path.join(win_binary_dir, 'winring0.zip')
-    cloud_storage.GetIfChanged(zip_path, bucket=cloud_storage.PUBLIC_BUCKET)
-    try:
-      with zipfile.ZipFile(zip_path, 'r') as zip_file:
-        # Install DLL.
-        if not os.path.exists(dll_path):
-          zip_file.extract(dll_file_name, executable_dir)
-        # Install kernel driver.
-        if not os.path.exists(driver_path):
-          zip_file.extract(driver_file_name, executable_dir)
-    finally:
-      os.remove(zip_path)
-
 
 def IsCurrentProcessElevated():
   handle = win32process.GetCurrentProcess()
@@ -80,35 +43,11 @@ def IsCurrentProcessElevated():
         win32security.GetTokenInformation(token, win32security.TokenElevation))
 
 
-def TerminateProcess(process_handle):
-  if not process_handle:
-    return
-  if win32process.GetExitCodeProcess(process_handle) == win32con.STILL_ACTIVE:
-    win32process.TerminateProcess(process_handle, 0)
-  process_handle.close()
-
-
 class WinPlatformBackend(desktop_platform_backend.DesktopPlatformBackend):
   def __init__(self):
     super(WinPlatformBackend, self).__init__()
-    self._msr_server_handle = None
-    self._msr_server_port = None
     self._power_monitor = msr_power_monitor.MsrPowerMonitor(self)
 
-  def __del__(self):
-    self.close()
-
-  def close(self):
-    self.CloseMsrServer()
-
-  def CloseMsrServer(self):
-    if not self._msr_server_handle:
-      return
-
-    TerminateProcess(self._msr_server_handle)
-    self._msr_server_handle = None
-    self._msr_server_port = None
-
   # pylint: disable=W0613
   def StartRawDisplayFrameRateMeasurement(self):
     raise NotImplementedError()
@@ -320,38 +259,3 @@ class WinPlatformBackend(desktop_platform_backend.DesktopPlatformBackend):
 
   def StopMonitoringPower(self):
     return self._power_monitor.StopMonitoringPower()
-
-  def _StartMsrServerIfNeeded(self):
-    if self._msr_server_handle:
-      return
-
-    _InstallWinRing0()
-    self._msr_server_port = util.GetUnreservedAvailableLocalPort()
-    # It might be flaky to get a port number without reserving it atomically,
-    # but if the server process chooses a port, we have no way of getting it.
-    # The stdout of the elevated process isn't accessible.
-    parameters = (
-        os.path.join(os.path.dirname(__file__), 'msr_server_win.py'),
-        str(self._msr_server_port),
-    )
-    self._msr_server_handle = self.LaunchApplication(
-        sys.executable, parameters, elevate_privilege=True)
-    # Wait for server to start. connect has a default timeout of 1 second.
-    try:
-      socket.create_connection(('127.0.0.1', self._msr_server_port)).close()
-    except socket.error:
-      self.CloseMsrServer()
-    atexit.register(TerminateProcess, self._msr_server_handle)
-
-  def ReadMsr(self, msr_number):
-    self._StartMsrServerIfNeeded()
-    if not self._msr_server_handle:
-      raise OSError('Unable to start MSR server.')
-
-    sock = socket.create_connection(('127.0.0.1', self._msr_server_port), 0.1)
-    try:
-      sock.sendall(struct.pack('I', msr_number))
-      response = sock.recv(8)
-    finally:
-      sock.close()
-    return struct.unpack('Q', response)[0]