Re: [PATCH v6] x86: detect CMOS aliasing on ports other than 0x70/0x71

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2023 13:35, Roger Pau Monné wrote:
> On Tue, Apr 18, 2023 at 11:24:19AM +0200, Jan Beulich wrote:
>> ... in order to also intercept Dom0 accesses through the alias ports.
>>
>> Also stop intercepting accesses to the CMOS ports if we won't ourselves
>> use the CMOS RTC, because of there being none.
>>
>> Note that rtc_init() deliberately uses 16 as the upper loop bound,
>> despite probe_cmos_alias() using 8: The higher bound is benign now, but
>> would save us touching the code (or, worse, missing to touch it) in case
>> the lower one was doubled.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Before committing I went back to read through doc and earlier comments,
in particular regarding the NMI disable. As a result I'm now inclined
to follow your earlier request and fold in the change below. Thoughts?

Jan

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1305,6 +1305,13 @@ bool is_cmos_port(unsigned int port, uns
 {
     unsigned int offs;
 
+    /*
+     * While not really CMOS-related, port 0x70 always needs intercepting
+     * to deal with the NMI disable bit.
+     */
+    if ( port <= RTC_PORT(0) && port + bytes > RTC_PORT(0) )
+        return true;
+
     if ( !is_hardware_domain(d) )
         return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0);
 
@@ -1342,6 +1349,17 @@ unsigned int rtc_guest_read(unsigned int
          * underlying hardware would permit doing so.
          */
         data = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(0)));
+
+        /*
+         * When there's (supposedly) no RTC/CMOS, we don't intercept the other
+         * ports. While reading the index register isn't normally possible,
+         * play safe and return back whatever can be read (just in case a value
+         * written through an alias would be attempted to be read back here).
+         */
+        if ( port == RTC_PORT(0) &&
+             (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) &&
+             ioports_access_permitted(currd, port, port) )
+            data = inb(port) & 0x7f;
         break;
 
     case RTC_PORT(1):
@@ -1378,6 +1396,16 @@ void rtc_guest_write(unsigned int port,
          * ports.
          */
         currd->arch.cmos_idx = data & (0xff >> (port == RTC_PORT(0)));
+
+        /*
+         * When there's (supposedly) no RTC/CMOS, we don't intercept the other
+         * ports. Therefore the port write, with the NMI disable bit zapped,
+         * needs carrying out right away.
+         */
+        if ( port == RTC_PORT(0) &&
+             (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) &&
+             ioports_access_permitted(currd, port, port) )
+            outb(data & 0x7f, port);
         break;
 
     case RTC_PORT(1):