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

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

On Thu, Mar 23, 2023 at 05:08:43PM +0100, Jan Beulich wrote:
> On 23.03.2023 15:49, Roger Pau Monné wrote:
> > On Mon, Mar 20, 2023 at 09:32:26AM +0100, Jan Beulich wrote:
> >> --- 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);
> > 
> > We seem to usually name this rtc rather than cmos, any reason to use
> > cmos for the helper naming rather than rtc?
> > 
> > If not I would rather use is_rtc_port(), so that we can keep it in
> > sync with the existing RTC_PORT() macros and the handler names
> > rtc_guest_{read,write}, hw_rtc_io.
> 
> Already when talking about just ports 70 and 71 there's more CMOS
> behind them than RTC. With extended CMOS accesses the ratio further
> shifts. So I view using "rtc" here simply as increasingly
> inappropriate.

Hm, it's your patch at the end, and such decision would likely fall
under the same bag as other style related questions.

I would prefer to keep the naming consistent, as to not confuse
readers with code dealing with the same underlying IO ports using both
RTC and CMOS, but that's just my taste.

> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -2072,37 +2072,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 PV.
> >> +         */
> > 
> > More than to keep consistency with PV, don't we need to trap accesses
> > to that concurrent accesses between dom0 and Xen (when also using the
> > device) don't overlap, as the RTC/CMOS space uses indirect indexing.
> 
> That's what I read "consistency" to mean.

But consistency with PV?  We need to keep consistency with concurrent
Xen (hypervisor) accesses I would think.

I would s/PV/hypervisor accesses/ in the comment above while moving
it.

> >> +        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();
> > 
> > You adding a call to process pending softirqs for every loop
> > iteration makes me wonder how long is each of those accesses expected
> > to take, since we could be performing a lot of them (0x80 * 3).
> 
> It seemed best to me to keep things simple here, at the expense at a
> few too many calls.
> 
> > I don't think so, but there can not be any side effects from reading
> > from the CMOS RAM I would assume, even for cases where the CMOS ports
> > are not aliases?
> 
> Well, one of the fundamental assumptions is that these read attempts
> won't have side effects. Without that assumption we simply can't do
> such probing.
> 
> > I would assume ports to be either aliased to the CMOS, or otherwise
> > reserved.  What makes me wonder if it wouldn't be simpler to just
> > passthough accesses to all the possible CMOS alias ports.
> 
> But we need to intercept writes to 70 to track the index. IOW we
> cannot simply pass through all of them, and we also cannot simply
> intercept them all and treat them all the same.

Why couldn't we intercept all the possible alias port and passthrough
all of them?  As long as there's nothing else there's no risk in doing
so?

> >> +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d)
> >> +{
> >> +    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) &&
> >> +         port <= RTC_PORT(cmos_alias_mask | 1) && port + bytes > RTC_PORT(0) )
> >> +    {
> >> +        unsigned int cmos = RTC_PORT(0), nr = 2, step;
> >> +
> >> +        while ( cmos_alias_mask & nr )
> >> +            nr <<= 1;
> >> +        for ( step = nr << 1;
> >> +              step < cmos_alias_mask && !(cmos_alias_mask & step); )
> >> +            step <<= 1;
> >> +        do {
> >> +            if ( !(cmos & ~RTC_PORT(cmos_alias_mask)) &&
> >> +                 port <= cmos + 1 && port + bytes > cmos )
> >> +                return true;
> >> +            cmos += step;
> >> +        } while ( cmos <= RTC_PORT(cmos_alias_mask) );
> > 
> > I would use a for loop similar to the one used in probe_cmos_alias()
> > to check for aliased accesses?
> > 
> > if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) )
> >     return true;
> > 
> > for ( offs = 2; offs < 8; offs <<= 1 )
> > {
> >     if ( !(offs & cmos_alias_mask) )
> >         continue;
> >     if ( port <= RTC_PORT(1 + off) && port + bytes > RTC_PORT(off) )
> >         return true;
> > }
> > 
> > return false;
> > 
> > So that you can also optimize for the more common case RTC_PORT(0) and
> > RTC_PORT(1) are used?
> > 
> > Or there's something I'm missing?
> 
> I'll have to check carefully, but to be honest I would prefer to not
> touch this code again unless there's clearly something wrong with it.

TBH, I think the proposed code is extremely difficult to follow, there
are 3 loops in a row which gives me a headache when thinking about all
the possible combinations.

I think my proposed alternative is much easier to follow because it
has a single loop, and it's using the same bounds used to fill the
cmos_alias_mask in the first place.  But maybe that's just my taste.

> >> @@ -1256,7 +1333,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):
> >>          /*
> >> @@ -1264,15 +1341,16 @@ unsigned int rtc_guest_read(unsigned int
> >>           * of the first RTC port, as there's no access to the physical IO
> >>           * ports.
> >>           */
> >> -        data = currd->arch.cmos_idx;
> >> +        data = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(0)));
> > 
> > We do allow read access to alias ports even when the underling
> > hardware does do so,
> 
> I'm afraid I don't understand this, so ...
> 
> > which I think is fine, but might be worth a
> > comment (since we already detect whether the RTC_PORT(0) alias is also
> > readable.
> 
> ... I can't really derive what kind of information you're after to put
> in a comment.

Reading from ports that alias RTC_PORT(0) might not always return the
value written to RTC_PORT(0) (you have a check for that in
probe_cmos_alias()).  Yet in rtc_guest_read() Xen does always return
the cached CMOS index.  Which is likely to be all fine, but needs a
comment to note this behavior might not match what the underlying
hardware would return.

Thanks, Roger.