Project

General

Profile

Actions

Bug #8740

open

Device registers should be declared volatile in hpet_acpi.c

Added by Gary Mills almost 5 years ago. Updated over 4 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
kernel
Start date:
2017-10-27
Due date:
% Done:

0%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

While reviewing code in uts/i86pc/io/hpet_acpi.c, I discovered two functions where device registers should have been declared volatile. This is necessary to ensure that the code is resistant to compiler or optimization changes. Disassembly tests have shown that with optimization the second device read is omitted, and that the function always returns success. This is my proposed change, although I don't have an easy way to test it:

--- hpet_acpi.c    Mon Jun 12 20:38:04 2017
+++ hpet_acpi.c+    Fri Oct 27 08:42:53 2017
@@ -412,10 +412,11 @@
 static int
 hpet_start_main_counter(hpet_info_t *hip)
 {
-    uint64_t    *gcr_ptr;
+    uint64_t volatile    *gcr_ptr;
     uint64_t    gcr;

-    gcr_ptr = (uint64_t *)HPET_GEN_CONFIG_ADDRESS(hip->logical_address);
+    gcr_ptr = (uint64_t volatile *)HPET_GEN_CONFIG_ADDRESS(
+        hip->logical_address);
     gcr = *gcr_ptr;

     gcr |= HPET_GCFR_ENABLE_CNF;
@@ -428,10 +429,11 @@
 static int
 hpet_stop_main_counter(hpet_info_t *hip)
 {
-    uint64_t    *gcr_ptr;
+    uint64_t volatile    *gcr_ptr;
     uint64_t    gcr;

-    gcr_ptr = (uint64_t *)HPET_GEN_CONFIG_ADDRESS(hip->logical_address);
+    gcr_ptr = (uint64_t volatile *)HPET_GEN_CONFIG_ADDRESS(
+        hip->logical_address);
     gcr = *gcr_ptr;

     gcr &= ~HPET_GCFR_ENABLE_CNF;

I would appreciate it if somebody else could test and complete this change.


Related issues

Related to illumos gate - Bug #14555: 64-bit HPET counter read is insufficiently volatileNew

Actions
Actions #1

Updated by Gary Mills over 4 years ago

This code is never executed with an AMD CPU because the cpuid_deep_cstates_supported() function always fails for that case. The error only affects systems with an Intel CPU.

Actions #2

Updated by Joshua M. Clulow 5 months ago

  • Related to Bug #14555: 64-bit HPET counter read is insufficiently volatile added
Actions

Also available in: Atom PDF