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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2023 16:31, Roger Pau Monné wrote:
> 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.

Thanks.

> 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?

Of course, albeit not more than a sentence, as the code comments provide
the details.

Jan