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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2023 17:55, Roger Pau Monné wrote:
> On Wed, Apr 19, 2023 at 03:58:10PM +0200, Jan Beulich wrote:
>> On 18.04.2023 13:35, Roger Pau Monné wrote:
>>> On Tue, Apr 18, 2023 at 11:24:19AM +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.
>>>>
>>>> Note that rtc_init() deliberately uses 16 as the upper loop bound,
>>>> despite probe_cmos_alias() using 8: The higher bound is benign now, but
>>>> would save us touching the code (or, worse, missing to touch it) in case
>>>> the lower one was doubled.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Before committing I went back to read through doc and earlier comments,
>> in particular regarding the NMI disable. As a result I'm now inclined
>> to follow your earlier request and fold in the change below. Thoughts?
> 
> It was unclear to me whether port 0x70 also had this NMI disabling
> behavior when the RTC/CMOS is not present but it seems that port is
> shared between the RTC index and the NMI logic, so lack of RTC doesn't
> imply lack of the NMI bit.

Right. My earlier oversight was that the datasheet I had pointed you
at actually explicitly mentions the NMI disable bit (really NMI# enable
there, named NMI_EN) in a separate section.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1305,6 +1305,13 @@ bool is_cmos_port(unsigned int port, uns
>>  {
>>      unsigned int offs;
>>  
>> +    /*
>> +     * While not really CMOS-related, port 0x70 always needs intercepting
>> +     * to deal with the NMI disable bit.
>> +     */
>> +    if ( port <= RTC_PORT(0) && port + bytes > RTC_PORT(0) )
>> +        return true;
> 
> It might make it clearer to move this after the !is_hardware_domain(d)
> check, as non-hardware domains don't get access to that port anyway?

I've done this; I had is earlier because when moved ...

>> +
>>      if ( !is_hardware_domain(d) )
>>          return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0);

... below here it becomes yet more odd with the 2nd of following if()s.
But I guess that's still "acceptably odd".

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

Jan