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

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

On Thu, Apr 20, 2023 at 10:31:08AM +0200, Jan Beulich wrote:
> On 19.04.2023 17:55, Roger Pau Monné wrote:
> > On Wed, Apr 19, 2023 at 03:58:10PM +0200, Jan Beulich wrote:
> >> @@ -1342,6 +1349,17 @@ unsigned int rtc_guest_read(unsigned int
> >>           * underlying hardware would permit doing so.
> >>           */
> >>          data = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(0)));
> >> +
> >> +        /*
> >> +         * When there's (supposedly) no RTC/CMOS, we don't intercept the 
> >> other
> >> +         * ports. While reading the index register isn't normally 
> >> possible,
> >> +         * play safe and return back whatever can be read (just in case a 
> >> value
> >> +         * written through an alias would be attempted to be read back 
> >> here).
> >> +         */
> >> +        if ( port == RTC_PORT(0) &&
> >> +             (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) &&
> >> +             ioports_access_permitted(currd, port, port) )
> >> +            data = inb(port) & 0x7f;
> > 
> > Do we really need to mask the high bit here?  We don't allow setting
> > that bit in the first place.
> 
> I think it's more consistent to mask it off, specifically with the code
> visible in context right above the insertion. The doc isn't really clear
> about readability of that bit: On one hand in says R/W for port 0x70 in
> the NMI_EN section, yet otoh in the RTC section it says "Note that port
> 70h is not directly readable. The only way to read this register is
> through Alt Access mode." (I think the NMI_EN section is more trustworthy,
> but still.) Plus if we were to ever make use of the NMI disable, we
> wouldn't want Dom0 see the bit set.

I guess so, at the end Xen itself doesn't use the bit so far.  Maybe
at some point we would want to expose the value of the bit to dom0 if
Xen starts using it (most than anything for informative purposes if
NMIs are disabled).

Feel free to fold the diff to the existing patch and keep the RB.

I guess you will also add something to the commit message about the
special handling of the NMI enable bit even when the RTC/CMOS is not
present?

Thanks, Roger.