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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2023 13:09, Roger Pau Monné wrote:
> On Thu, Mar 30, 2023 at 12:40:38PM +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.
> 
> So it's fine for dom0 to switch off NMIs if Xen isn't using the RTC?
> Seems like a weird side-effect of Xen not using the RTC (seeing as we
> would otherwise mask bit 8 from dom0 RTC accesses).

I haven't been able to find documentation on this single bit in the
absence of RTC / CMOS.

> Also I'm worried that when Xen doesn't intercept RTC ports accesses
> from dom0 could be interrupted for example by the vCPU being scheduled
> out, so a vCPU might perform a write to the index port, and be
> scheduled out, leaving the RTC in an undefined state.

I did specifically add "because of there being none" to the sentence
to clarify in which case we avoid intercepting.

> I've read claims online that the RTC is not reset by the firmware, and
> since it has a battery the state is kept across reboots, so
> interrupting an access like that cold leave the RTC in a broken state
> across reboots.

I can easily imagine such firmware exists.

>> @@ -836,10 +836,18 @@ void rtc_init(struct domain *d)
>>  
>>      if ( !has_vrtc(d) )
>>      {
>> -        if ( is_hardware_domain(d) )
>> -            /* Hardware domain gets mediated access to the physical RTC. */
>> -            register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io);
>> -        return;
>> +        unsigned int port;
>> +
>> +        if ( !is_hardware_domain(d) )
>> +            return;
>> +
>> +        /*
>> +         * Hardware domain gets mediated access to the physical RTC/CMOS (of
>> +         * course unless we don't use it ourselves, for there being none).
>> +         */
>> +        for ( port = RTC_PORT(0); port < RTC_PORT(0) + 0x10; port += 2 )
>> +            if ( is_cmos_port(port, 2, d) )
>> +                register_portio_handler(d, port, 2, hw_rtc_io);
> 
> You seem to have dropped a return from here, as for PVH dom0 the
> initialization below shouldn't be done.

Oh, indeed, thanks for spotting. (The excess init is benign afaict, but
I still shouldn't have dropped that "return".)

>> @@ -1249,6 +1252,77 @@ 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 offs;
>> +
>> +    if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
>> +        return 0;
>> +
>> +    for ( offs = 2; offs < 8; offs <<= 1 )
>> +    {
>> +        unsigned int i;
>> +        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;
>> +
>> +            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();
>> +        }
>> +        if ( i == 0x80 )
>> +        {
>> +            cmos_alias_mask |= offs;
>> +            printk(XENLOG_INFO "CMOS aliased at %02x, index %s\n",
>> +                   RTC_PORT(offs), read ? "r/w" : "w/o");
> 
> I would consider making this a DEBUG message, not sure it's that
> useful for a normal end user, and printing to the console can be slow.

Can do, sure.

>> +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct 
>> domain *d)
>> +{
>> +    unsigned int offs;
>> +
>> +    if ( !is_hardware_domain(d) ||
>> +         !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) )
>> +        return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0);
>> +
>> +    if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC )
>> +        return false;
>> +
>> +    for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 )
>> +    {
>> +        if ( !(offs & cmos_alias_mask) )
>> +            continue;
>> +        if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) )
>> +            return true;
>> +    }
> 
> Maybe I'm confused, but doesn't this loop start at RTC_PORT(2), and
> hence you need to check for the RTC_PORT(0,1) pair outside of the
> loop?

The loop starts at offset 2, yes, but see the initial if() in the
function. Or at least I thought I got that right, but it looks like
I didn't (failed attempt to try to address a specific request of
yours, iirc).

>> @@ -1256,23 +1330,25 @@ unsigned int rtc_guest_read(unsigned int
>>      unsigned long flags;
>>      unsigned int data = ~0;
>>  
>> -    switch ( port )
>> +    switch ( port & ~cmos_alias_mask )
> 
> Given that the call is gated with is_cmos_port() it would be clearer
> to just use RTC_PORT(1) as the mask here IMO.

Hmm, personally I wouldn't consider RTC_PORT(1) to be reasonable to
use as a mask (even if technically it would be okay).

Jan