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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> And likely to avoid dom0 from disabling NMIs.

I can add that to the comment, but as you say ...

> I see that you copied the existing comment, but not sure it's fully
> accurate?

... I've merely moved it.

>> @@ -1249,6 +1252,80 @@ 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 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;
> 
> You can limit the scope of i to the inner for loop.

Sure. It has been long ago when I wrote this, so I guess I was after it
being one line less of code.

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

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

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

Jan