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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 15, 2020 at 11:47:56AM +0200, Jan Beulich wrote:
> ... in order to also intercept accesses through the alias ports.
> 
> Also stop intercepting accesses to the CMOS ports if we won't ourselves
> use the CMOS RTC.

I think you are missing the registration of the aliased ports in
rtc_init for a PVH hardware domain, hw_rtc_io will currently only get
called by accesses to 0x71-0x71.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Re-base.
> 
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -670,6 +670,80 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>      return ret;
>  }
>  
> +#ifndef COMPAT
> +#include <asm/mc146818rtc.h>
> +
> +unsigned int __read_mostly cmos_alias_mask;
> +
> +static int __init probe_cmos_alias(void)
> +{
> +    unsigned int i, offs;
> +
> +    if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
> +        return 0;
> +
> +    for ( offs = 2; offs < 8; offs <<= 1 )
> +    {
> +        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;

I'm missing something, why do you avoid the century register for
comparison reasons?

> @@ -2009,37 +2009,33 @@ int __hwdom_init xen_in_range(unsigned l
>  static int __hwdom_init io_bitmap_cb(unsigned long s, unsigned long e,
>                                       void *ctx)
>  {
> -    struct domain *d = ctx;
> +    const struct domain *d = ctx;

Urg, it's kind of weird to constify d ...

>      unsigned int i;
>  
>      ASSERT(e <= INT_MAX);
>      for ( i = s; i <= e; i++ )
> -        __clear_bit(i, d->arch.hvm.io_bitmap);
> +        if ( admin_io_okay(i, 1, d) )
> +            __clear_bit(i, d->arch.hvm.io_bitmap);

... when you are modifying the bitmap here.

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

You can directly use BUG_ON, no need for the if. IIRC it's safe to
call admin_io_okay (and thus ioports_access_permitted) when already
holding the rangeset lock, as both are read-lockers and can safely
recurse.

> +
> +    /*
> +     * We need to trap 4-byte accesses to 0xcf8 (see admin_io_okay(),
> +     * guest_io_read(), and guest_io_write()), which isn't covered by
> +     * the admin_io_okay() check in io_bitmap_cb().
> +     */
> +    __set_bit(0xcf8, d->arch.hvm.io_bitmap);
>  }
>  
>  /*
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1092,7 +1092,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;

Do you need to set this flag also when using the EFI runtime services
in order to get the time in get_cmos_time? In that case the RTC is not
use, and hence could be handled to the hardware domain?

> +            }
>              break;
>          }
>  
> @@ -1114,7 +1117,7 @@ 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):
>          /*
> @@ -1126,11 +1129,12 @@ unsigned int rtc_guest_read(unsigned int
>          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))),

Why do you only mask this for accesses to the non aliased ports? If
the RTC is aliased you also want to mask the aliased accesses in the
same way?

Thanks, Roger.