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:
>> @@ -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.
> 
>> +
>> +        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).
> 
> 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?
> 
> 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.

I'm afraid this assumption doesn't hold, as can be seen from the ICH10
datasheet that I did point you at the other day. There ports 72/73
serve a purpose different from ports 70/71 (and their aliases at 74/75).
Unless (as the datasheet calls it) U128E is clear (wherever that bit
lives), in which case 72/73 (and 76/77) will also be aliases of 70/71.
So we won't get away without probing, and if we deem probing too risky,
then all I can do is drop this patch.

Jan