... 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@suse.com>
---
v6: Restore lost "return" in rtc_init(). Convert printk() to dprintk()
    in probe_cmos_alias(). Correct is_cmos_port() for hwdom.
v5: Simplify logic in is_cmos_port(). Limit the scope of a local
    variable. Adjust a comment that's being moved.
v4: Also conditionally mask top bit for guest index port accesses. Add
    missing adjustments to rtc_init(). Re-work to avoid recursive
    read_lock(). Also adjust guest_io_{read,write}(). Re-base.
v3: Re-base over change to earlier patch.
v2: Re-base.

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -27,7 +27,7 @@
 #include <asm/hvm/vpt.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/save.h>
-#include <asm/current.h>
+#include <asm/iocap.h>
 #include <xen/trace.h>
 #include <public/hvm/params.h>
 
@@ -836,9 +836,19 @@ void rtc_init(struct domain *d)
 
     if ( !has_vrtc(d) )
     {
-        if ( is_hardware_domain(d) )
-            /* Hardware domain gets mediated access to the physical RTC. */
-            register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io);
+        unsigned int port;
+
+        if ( !is_hardware_domain(d) )
+            return;
+
+        /*
+         * Hardware domain gets mediated access to the physical RTC/CMOS (of
+         * course unless we don't use it ourselves, for there being none).
+         */
+        for ( port = RTC_PORT(0); port < RTC_PORT(0) + 0x10; port += 2 )
+            if ( is_cmos_port(port, 2, d) )
+                register_portio_handler(d, port, 2, hw_rtc_io);
+
         return;
     }
 
--- a/xen/arch/x86/include/asm/mc146818rtc.h
+++ b/xen/arch/x86/include/asm/mc146818rtc.h
@@ -9,6 +9,10 @@
 
 extern spinlock_t rtc_lock;             /* serialize CMOS RAM access */
 
+struct domain;
+bool is_cmos_port(unsigned int port, unsigned int bytes,
+                  const struct domain *d);
+
 /**********************************************************************
  * register summary
  **********************************************************************/
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -208,7 +208,7 @@ static bool admin_io_okay(unsigned int p
         return false;
 
     /* We also never permit direct access to the RTC/CMOS registers. */
-    if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) )
+    if ( is_cmos_port(port, bytes, d) )
         return false;
 
     return ioports_access_permitted(d, port, port + bytes - 1);
@@ -278,7 +278,7 @@ static uint32_t guest_io_read(unsigned i
         {
             sub_data = pv_pit_handler(port, 0, 0);
         }
-        else if ( port == RTC_PORT(0) || port == RTC_PORT(1) )
+        else if ( is_cmos_port(port, 1, currd) )
         {
             sub_data = rtc_guest_read(port);
         }
@@ -424,7 +424,7 @@ static void guest_io_write(unsigned int
         {
             pv_pit_handler(port, (uint8_t)data, 1);
         }
-        else if ( port == RTC_PORT(0) || port == RTC_PORT(1) )
+        else if ( is_cmos_port(port, 1, currd) )
         {
             rtc_guest_write(port, data);
         }
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -2130,37 +2130,36 @@ int __hwdom_init xen_in_range(unsigned l
 static int __hwdom_init cf_check io_bitmap_cb(
     unsigned long s, unsigned long e, void *ctx)
 {
-    struct domain *d = ctx;
+    const struct domain *d = ctx;
     unsigned int i;
 
     ASSERT(e <= INT_MAX);
     for ( i = s; i <= e; i++ )
-        __clear_bit(i, d->arch.hvm.io_bitmap);
+        /*
+         * Accesses to RTC ports also need to be trapped in order to keep
+         * consistency with hypervisor accesses.
+         */
+        if ( !is_cmos_port(i, 1, d) )
+            __clear_bit(i, d->arch.hvm.io_bitmap);
 
     return 0;
 }
 
 void __hwdom_init setup_io_bitmap(struct domain *d)
 {
-    int rc;
+    if ( !is_hvm_domain(d) )
+        return;
 
-    if ( is_hvm_domain(d) )
-    {
-        bitmap_fill(d->arch.hvm.io_bitmap, 0x10000);
-        rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
-                                    io_bitmap_cb, d);
-        BUG_ON(rc);
-        /*
-         * NB: we need to trap accesses to 0xcf8 in order to intercept
-         * 4 byte accesses, that need to be handled by Xen in order to
-         * keep consistency.
-         * Access to 1 byte RTC ports also needs to be trapped in order
-         * to keep consistency with PV.
-         */
-        __set_bit(0xcf8, d->arch.hvm.io_bitmap);
-        __set_bit(RTC_PORT(0), d->arch.hvm.io_bitmap);
-        __set_bit(RTC_PORT(1), d->arch.hvm.io_bitmap);
-    }
+    bitmap_fill(d->arch.hvm.io_bitmap, 0x10000);
+    if ( rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
+                                io_bitmap_cb, d) )
+        BUG();
+
+    /*
+     * We need to trap 4-byte accesses to 0xcf8 (see admin_io_okay(),
+     * guest_io_read(), and guest_io_write()).
+     */
+    __set_bit(0xcf8, d->arch.hvm.io_bitmap);
 }
 
 /*
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1234,7 +1234,10 @@ static unsigned long get_cmos_time(void)
         if ( seconds < 60 )
         {
             if ( rtc.sec != seconds )
+            {
                 cmos_rtc_probe = false;
+                acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
+            }
             break;
         }
 
@@ -1249,6 +1252,79 @@ static unsigned long get_cmos_time(void)
     return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
 }
 
+static unsigned int __ro_after_init cmos_alias_mask;
+
+static int __init cf_check probe_cmos_alias(void)
+{
+    unsigned int offs;
+
+    if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
+        return 0;
+
+    for ( offs = 2; offs < 8; offs <<= 1 )
+    {
+        unsigned int i;
+        bool read = true;
+
+        for ( i = RTC_REG_D + 1; i < 0x80; ++i )
+        {
+            uint8_t normal, alt;
+            unsigned long flags;
+
+            if ( i == acpi_gbl_FADT.century )
+                continue;
+
+            spin_lock_irqsave(&rtc_lock, flags);
+
+            normal = CMOS_READ(i);
+            if ( inb(RTC_PORT(offs)) != i )
+                read = false;
+
+            alt = inb(RTC_PORT(offs + 1));
+
+            spin_unlock_irqrestore(&rtc_lock, flags);
+
+            if ( normal != alt )
+                break;
+
+            process_pending_softirqs();
+        }
+        if ( i == 0x80 )
+        {
+            cmos_alias_mask |= offs;
+            dprintk(XENLOG_INFO, "CMOS aliased at %02x, index %s\n",
+                    RTC_PORT(offs), read ? "r/w" : "w/o");
+        }
+    }
+
+    return 0;
+}
+__initcall(probe_cmos_alias);
+
+bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d)
+{
+    unsigned int offs;
+
+    if ( !is_hardware_domain(d) )
+        return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0);
+
+    if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
+        return false;
+
+    if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) )
+        return true;
+
+    for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 )
+    {
+        if ( !(offs & cmos_alias_mask) )
+            continue;
+        if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) )
+            return true;
+    }
+
+    return false;
+}
+
 /* Helpers for guest accesses to the physical RTC. */
 unsigned int rtc_guest_read(unsigned int port)
 {
@@ -1256,23 +1332,25 @@ unsigned int rtc_guest_read(unsigned int
     unsigned long flags;
     unsigned int data = ~0;
 
-    switch ( port )
+    switch ( port & ~cmos_alias_mask )
     {
     case RTC_PORT(0):
         /*
          * All PV domains (and PVH dom0) are allowed to read the latched value
          * of the first RTC port, as there's no access to the physical IO
-         * ports.
+         * ports.  Note that we return the index value regardless of whether
+         * underlying hardware would permit doing so.
          */
-        data = currd->arch.cmos_idx;
+        data = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(0)));
         break;
 
     case RTC_PORT(1):
-        if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
+        if ( !ioports_access_permitted(currd, port - 1, port) )
             break;
         spin_lock_irqsave(&rtc_lock, flags);
-        outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
-        data = inb(RTC_PORT(1));
+        outb(currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(1))),
+             port - 1);
+        data = inb(port);
         spin_unlock_irqrestore(&rtc_lock, flags);
         break;
 
@@ -1288,9 +1366,10 @@ void rtc_guest_write(unsigned int port,
     struct domain *currd = current->domain;
     unsigned long flags;
 
-    switch ( port )
+    switch ( port & ~cmos_alias_mask )
     {
         typeof(pv_rtc_handler) hook;
+        unsigned int idx;
 
     case RTC_PORT(0):
         /*
@@ -1298,20 +1377,22 @@ void rtc_guest_write(unsigned int port,
          * value of the first RTC port, as there's no access to the physical IO
          * ports.
          */
-        currd->arch.cmos_idx = data;
+        currd->arch.cmos_idx = data & (0xff >> (port == RTC_PORT(0)));
         break;
 
     case RTC_PORT(1):
-        if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
+        if ( !ioports_access_permitted(currd, port - 1, port) )
             break;
 
+        idx = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(1)));
+
         hook = ACCESS_ONCE(pv_rtc_handler);
         if ( hook )
-            hook(currd->arch.cmos_idx & 0x7f, data);
+            hook(idx, data);
 
         spin_lock_irqsave(&rtc_lock, flags);
-        outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
-        outb(data, RTC_PORT(1));
+        outb(idx, port - 1);
+        outb(data, port);
         spin_unlock_irqrestore(&rtc_lock, flags);
         break;

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@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v6: Restore lost "return" in rtc_init(). Convert printk() to dprintk()
>     in probe_cmos_alias(). Correct is_cmos_port() for hwdom.
> v5: Simplify logic in is_cmos_port(). Limit the scope of a local
>     variable. Adjust a comment that's being moved.
> v4: Also conditionally mask top bit for guest index port accesses. Add
>     missing adjustments to rtc_init(). Re-work to avoid recursive
>     read_lock(). Also adjust guest_io_{read,write}(). Re-base.
> v3: Re-base over change to earlier patch.
> v2: Re-base.
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -27,7 +27,7 @@
>  #include <asm/hvm/vpt.h>
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/save.h>
> -#include <asm/current.h>
> +#include <asm/iocap.h>
>  #include <xen/trace.h>
>  #include <public/hvm/params.h>
>  
> @@ -836,9 +836,19 @@ void rtc_init(struct domain *d)
>  
>      if ( !has_vrtc(d) )
>      {
> -        if ( is_hardware_domain(d) )
> -            /* Hardware domain gets mediated access to the physical RTC. */
> -            register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io);
> +        unsigned int port;
> +
> +        if ( !is_hardware_domain(d) )
> +            return;
> +
> +        /*
> +         * Hardware domain gets mediated access to the physical RTC/CMOS (of
> +         * course unless we don't use it ourselves, for there being none).
> +         */
> +        for ( port = RTC_PORT(0); port < RTC_PORT(0) + 0x10; port += 2 )
> +            if ( is_cmos_port(port, 2, d) )
> +                register_portio_handler(d, port, 2, hw_rtc_io);
> +
>          return;
>      }
>  
> --- a/xen/arch/x86/include/asm/mc146818rtc.h
> +++ b/xen/arch/x86/include/asm/mc146818rtc.h
> @@ -9,6 +9,10 @@
>  
>  extern spinlock_t rtc_lock;             /* serialize CMOS RAM access */
>  
> +struct domain;
> +bool is_cmos_port(unsigned int port, unsigned int bytes,
> +                  const struct domain *d);
> +
>  /**********************************************************************
>   * register summary
>   **********************************************************************/
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -208,7 +208,7 @@ static bool admin_io_okay(unsigned int p
>          return false;
>  
>      /* We also never permit direct access to the RTC/CMOS registers. */

Hm, it's unclear to me whether the comment above would need updating:
we don't allow direct access to the RTC/CMOS registers, but we allow
direct access to the RTC/CMOS ports if there's no device behind.

Thanks, Roger.

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@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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):

On Wed, Apr 19, 2023 at 03:58:10PM +0200, Jan Beulich wrote:
> 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@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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?

It was unclear to me whether port 0x70 also had this NMI disabling
behavior when the RTC/CMOS is not present but it seems that port is
shared between the RTC index and the NMI logic, so lack of RTC doesn't
imply lack of the NMI bit.

> 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;

It might make it clearer to move this after the !is_hardware_domain(d)
check, as non-hardware domains don't get access to that port anyway?

> +
>      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;

Do we really need to mask the high bit here?  We don't allow setting
that bit in the first place.

Thanks, Roger.

On 19.04.2023 17:55, Roger Pau Monné wrote:
> On Wed, Apr 19, 2023 at 03:58:10PM +0200, Jan Beulich wrote:
>> 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@suse.com>
>>>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> 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?
> 
> It was unclear to me whether port 0x70 also had this NMI disabling
> behavior when the RTC/CMOS is not present but it seems that port is
> shared between the RTC index and the NMI logic, so lack of RTC doesn't
> imply lack of the NMI bit.

Right. My earlier oversight was that the datasheet I had pointed you
at actually explicitly mentions the NMI disable bit (really NMI# enable
there, named NMI_EN) in a separate section.

>> --- 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;
> 
> It might make it clearer to move this after the !is_hardware_domain(d)
> check, as non-hardware domains don't get access to that port anyway?

I've done this; I had is earlier because when moved ...

>> +
>>      if ( !is_hardware_domain(d) )
>>          return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0);

... below here it becomes yet more odd with the 2nd of following if()s.
But I guess that's still "acceptably odd".

>> @@ -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;
> 
> Do we really need to mask the high bit here?  We don't allow setting
> that bit in the first place.

I think it's more consistent to mask it off, specifically with the code
visible in context right above the insertion. The doc isn't really clear
about readability of that bit: On one hand in says R/W for port 0x70 in
the NMI_EN section, yet otoh in the RTC section it says "Note that port
70h is not directly readable. The only way to read this register is
through Alt Access mode." (I think the NMI_EN section is more trustworthy,
but still.) Plus if we were to ever make use of the NMI disable, we
wouldn't want Dom0 see the bit set.

Jan

On Thu, Apr 20, 2023 at 10:31:08AM +0200, Jan Beulich wrote:
> On 19.04.2023 17:55, Roger Pau Monné wrote:
> > On Wed, Apr 19, 2023 at 03:58:10PM +0200, Jan Beulich wrote:
> >> @@ -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;
> > 
> > Do we really need to mask the high bit here?  We don't allow setting
> > that bit in the first place.
> 
> I think it's more consistent to mask it off, specifically with the code
> visible in context right above the insertion. The doc isn't really clear
> about readability of that bit: On one hand in says R/W for port 0x70 in
> the NMI_EN section, yet otoh in the RTC section it says "Note that port
> 70h is not directly readable. The only way to read this register is
> through Alt Access mode." (I think the NMI_EN section is more trustworthy,
> but still.) Plus if we were to ever make use of the NMI disable, we
> wouldn't want Dom0 see the bit set.

I guess so, at the end Xen itself doesn't use the bit so far.  Maybe
at some point we would want to expose the value of the bit to dom0 if
Xen starts using it (most than anything for informative purposes if
NMIs are disabled).

Feel free to fold the diff to the existing patch and keep the RB.

I guess you will also add something to the commit message about the
special handling of the NMI enable bit even when the RTC/CMOS is not
present?

Thanks, Roger.

On 20.04.2023 16:31, Roger Pau Monné wrote:
> On Thu, Apr 20, 2023 at 10:31:08AM +0200, Jan Beulich wrote:
>> On 19.04.2023 17:55, Roger Pau Monné wrote:
>>> On Wed, Apr 19, 2023 at 03:58:10PM +0200, Jan Beulich wrote:
>>>> @@ -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;
>>>
>>> Do we really need to mask the high bit here?  We don't allow setting
>>> that bit in the first place.
>>
>> I think it's more consistent to mask it off, specifically with the code
>> visible in context right above the insertion. The doc isn't really clear
>> about readability of that bit: On one hand in says R/W for port 0x70 in
>> the NMI_EN section, yet otoh in the RTC section it says "Note that port
>> 70h is not directly readable. The only way to read this register is
>> through Alt Access mode." (I think the NMI_EN section is more trustworthy,
>> but still.) Plus if we were to ever make use of the NMI disable, we
>> wouldn't want Dom0 see the bit set.
> 
> I guess so, at the end Xen itself doesn't use the bit so far.  Maybe
> at some point we would want to expose the value of the bit to dom0 if
> Xen starts using it (most than anything for informative purposes if
> NMIs are disabled).
> 
> Feel free to fold the diff to the existing patch and keep the RB.

Thanks.

> I guess you will also add something to the commit message about the
> special handling of the NMI enable bit even when the RTC/CMOS is not
> present?

Of course, albeit not more than a sentence, as the code comments provide
the details.

Jan

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@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -208,7 +208,7 @@ static bool admin_io_okay(unsigned int p
>>          return false;
>>  
>>      /* We also never permit direct access to the RTC/CMOS registers. */
> 
> Hm, it's unclear to me whether the comment above would need updating:
> we don't allow direct access to the RTC/CMOS registers, but we allow
> direct access to the RTC/CMOS ports if there's no device behind.

Right, but those ports then don't allow access to said registers. So
I think the comment is fine as is.

Jan

On Wed, Apr 19, 2023 at 09:56:35AM +0200, Jan Beulich wrote:
> 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@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> --- a/xen/arch/x86/pv/emul-priv-op.c
> >> +++ b/xen/arch/x86/pv/emul-priv-op.c
> >> @@ -208,7 +208,7 @@ static bool admin_io_okay(unsigned int p
> >>          return false;
> >>  
> >>      /* We also never permit direct access to the RTC/CMOS registers. */
> > 
> > Hm, it's unclear to me whether the comment above would need updating:
> > we don't allow direct access to the RTC/CMOS registers, but we allow
> > direct access to the RTC/CMOS ports if there's no device behind.
> 
> Right, but those ports then don't allow access to said registers. So
> I think the comment is fine as is.

Yes, that's why I wasn't really sure whether to comment.  The comment
is formally correct, but it might lead to confusion if one doesn't
carefully read 'RTC/CMOS registers' (vs RTC/CMOS IO ports).

Anyway, sorry for the noise.

Thanks, Roger.