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.
