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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.