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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2023 17:40, Roger Pau Monné wrote:
> On Thu, Mar 23, 2023 at 05:08:43PM +0100, Jan Beulich wrote:
>> On 23.03.2023 15:49, Roger Pau Monné wrote:
>>> On Mon, Mar 20, 2023 at 09:32:26AM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -2072,37 +2072,36 @@ int __hwdom_init xen_in_range(unsigned l
>>>>  static int __hwdom_init cf_check io_bitmap_cb(
>>>>      unsigned long s, unsigned long e, void *ctx)
>>>>  {
>>>> -    struct domain *d = ctx;
>>>> +    const struct domain *d = ctx;
>>>>      unsigned int i;
>>>>  
>>>>      ASSERT(e <= INT_MAX);
>>>>      for ( i = s; i <= e; i++ )
>>>> -        __clear_bit(i, d->arch.hvm.io_bitmap);
>>>> +        /*
>>>> +         * Accesses to RTC ports also need to be trapped in order to keep
>>>> +         * consistency with PV.
>>>> +         */
>>>
>>> More than to keep consistency with PV, don't we need to trap accesses
>>> to that concurrent accesses between dom0 and Xen (when also using the
>>> device) don't overlap, as the RTC/CMOS space uses indirect indexing.
>>
>> That's what I read "consistency" to mean.
> 
> But consistency with PV?  We need to keep consistency with concurrent
> Xen (hypervisor) accesses I would think.
> 
> I would s/PV/hypervisor accesses/ in the comment above while moving
> it.

Hmm, yes, changed. (Still I'm not normally very happy about changing
comments I don't really understand the way they're written.)

>>>> +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct 
>>>> domain *d)
>>>> +{
>>>> +    if ( !is_hardware_domain(d) )
>>>> +        return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0);
>>>> +
>>>> +    if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) &&
>>>> +         port <= RTC_PORT(cmos_alias_mask | 1) && port + bytes > 
>>>> RTC_PORT(0) )
>>>> +    {
>>>> +        unsigned int cmos = RTC_PORT(0), nr = 2, step;
>>>> +
>>>> +        while ( cmos_alias_mask & nr )
>>>> +            nr <<= 1;
>>>> +        for ( step = nr << 1;
>>>> +              step < cmos_alias_mask && !(cmos_alias_mask & step); )
>>>> +            step <<= 1;
>>>> +        do {
>>>> +            if ( !(cmos & ~RTC_PORT(cmos_alias_mask)) &&
>>>> +                 port <= cmos + 1 && port + bytes > cmos )
>>>> +                return true;
>>>> +            cmos += step;
>>>> +        } while ( cmos <= RTC_PORT(cmos_alias_mask) );
>>>
>>> I would use a for loop similar to the one used in probe_cmos_alias()
>>> to check for aliased accesses?
>>>
>>> if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) )
>>>     return true;
>>>
>>> for ( offs = 2; offs < 8; offs <<= 1 )
>>> {
>>>     if ( !(offs & cmos_alias_mask) )
>>>         continue;
>>>     if ( port <= RTC_PORT(1 + off) && port + bytes > RTC_PORT(off) )
>>>         return true;
>>> }
>>>
>>> return false;
>>>
>>> So that you can also optimize for the more common case RTC_PORT(0) and
>>> RTC_PORT(1) are used?
>>>
>>> Or there's something I'm missing?
>>
>> I'll have to check carefully, but to be honest I would prefer to not
>> touch this code again unless there's clearly something wrong with it.
> 
> TBH, I think the proposed code is extremely difficult to follow, there
> are 3 loops in a row which gives me a headache when thinking about all
> the possible combinations.
> 
> I think my proposed alternative is much easier to follow because it
> has a single loop, and it's using the same bounds used to fill the
> cmos_alias_mask in the first place.  But maybe that's just my taste.

Upon re-consideration I've adjusted the code. Iirc probe_cmos_alias()
originally had something similar, so changing it in the other place as
well is only logical.

>>>> @@ -1256,7 +1333,7 @@ unsigned int rtc_guest_read(unsigned int
>>>>      unsigned long flags;
>>>>      unsigned int data = ~0;
>>>>  
>>>> -    switch ( port )
>>>> +    switch ( port & ~cmos_alias_mask )
>>>>      {
>>>>      case RTC_PORT(0):
>>>>          /*
>>>> @@ -1264,15 +1341,16 @@ unsigned int rtc_guest_read(unsigned int
>>>>           * of the first RTC port, as there's no access to the physical IO
>>>>           * ports.
>>>>           */
>>>> -        data = currd->arch.cmos_idx;
>>>> +        data = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(0)));
>>>
>>> We do allow read access to alias ports even when the underling
>>> hardware does do so,
>>
>> I'm afraid I don't understand this, so ...
>>
>>> which I think is fine, but might be worth a
>>> comment (since we already detect whether the RTC_PORT(0) alias is also
>>> readable.
>>
>> ... I can't really derive what kind of information you're after to put
>> in a comment.
> 
> Reading from ports that alias RTC_PORT(0) might not always return the
> value written to RTC_PORT(0) (you have a check for that in
> probe_cmos_alias()).  Yet in rtc_guest_read() Xen does always return
> the cached CMOS index.  Which is likely to be all fine, but needs a
> comment to note this behavior might not match what the underlying
> hardware would return.

You do realize that this is pre-existing behavior? On many chipsets
you cannot read the index value from port 70. Yet we've always
returned it, presumably on the grounds of the value either being
rubbish (often 0xff) or the index. And rather than trying to figure
out whether to return rubbish (explicitly of via accessing the port)
I guess it was deemed easier and more helpful to always return the
index value. (I've amended the comment there nevertheless.)

Jan