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

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

On Mon, Apr 03, 2023 at 01:26:41PM +0200, Jan Beulich wrote:
> 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.

Oh, right, sorry for the fuzz, I didn't parse that last bit of the
sentence.  I'm fine with the current wording then.

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

Hm, doesn't that first if() cause that on all systems with an RTC only
PORTS(0,1) are allowed?

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

OK, never mind then.

Thanks, Roger.