Re: [PATCH 4/7] x86: detect PIC aliasing on ports other than 0x[2A][01]

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2023 15:24, Roger Pau Monné wrote:
> On Thu, Oct 26, 2023 at 11:03:42AM +0200, Jan Beulich wrote:
>> On 26.10.2023 10:34, Roger Pau Monné wrote:
>>> On Thu, May 11, 2023 at 02:06:46PM +0200, Jan Beulich wrote:
>>>> ... in order to also deny Dom0 access through the alias ports. Without
>>>> this it is only giving the impression of denying access to both PICs.
>>>> Unlike for CMOS/RTC, do detection very early, to avoid disturbing normal
>>>> operation later on.
>>>>
>>>> Like for CMOS/RTC a fundamental assumption of the probing is that reads
>>>> from the probed alias port won't have side effects in case it does not
>>>> alias the respective PIC's one.
>>>
>>> I'm slightly concerned about this probing.
>>>
>>> Also I'm unsure we can fully isolate the hardware domain like this.
>>> Preventing access to the non-aliased ports is IMO helpful for domains
>>> to realize the PIT is not available, but in any case such accesses
>>> shouldn't happen in the first place, as dom0 must be modified to run
>>> in such mode.
>>
>> That's true for PV Dom0, but not necessarily for PVH. Plus by denying
>> access to the aliases we also guard against bugs in Dom0, if some
>> component thinks there's something else at those ports (as they
>> indeed were used for other purposes by various vendors).
> 
> I think it would be safe to add a command line option to disable the
> probing, as we would at least like to avoid it in pvshim mode.  Maybe
> ut would be interesting to make it a Kconfig option so that exclusive
> pvshim Kconfig can avoid all this?
> 
> Otherwise it will just make booting the pvshim slower.

I've taken note to introduce such an option (not sure yet whether just
cmdline or also Kconfig). Still
- Shouldn't we already be bypassing related init logic in shim mode?
- A Kconfig option interfacing with PV_SHIM_EXCLUSIVE will collide with
  my patch inverting that option's sense (and renaming it), so it would
  be nice to have that sorted/accepted first (see
  https://lists.xen.org/archives/html/xen-devel/2023-03/msg00040.html).

>>>> @@ -492,10 +492,17 @@ int __init dom0_setup_permissions(struct
>>>>  
>>>>      /* Modify I/O port access permissions. */
>>>>  
>>>> -    /* Master Interrupt Controller (PIC). */
>>>> -    rc |= ioports_deny_access(d, 0x20, 0x21);
>>>> -    /* Slave Interrupt Controller (PIC). */
>>>> -    rc |= ioports_deny_access(d, 0xA0, 0xA1);
>>>> +    for ( offs = 0, i = pic_alias_mask & -pic_alias_mask ?: 2;
>>>> +          offs <= pic_alias_mask; offs += i )
>>>
>>> I'm a bit lost with this, specifically:
>>>
>>> i = pic_alias_mask & -pic_alias_mask ?: 2
>>>
>>> Which is then used as the increment step in
>>>
>>> offs += i
>>>
>>> I could see the usage of pic_alias_mask & -pic_alias_mask in order to
>>> find the first offset, but afterwards don't you need to increment at
>>> single bit left shifts in order to test all possibly set bits in
>>> pic_alias_mask?
>>
>> No, the smallest sensible increment is the lowest bit set in
>> pic_alias_mask. There's specifically no shifting involved here (just
>> mentioning it because you use the word). E.g. if the aliasing was at
>> bits 2 and 3 (pic_alias_mask=0x0c), we'd need to deny access to 20/21,
>> 24/25, 28/29, and 2C/2D, i.e. at an increment of 4.
> 
> Right, it took me a bit to realize.
> 
> We assume that aliases are based on fused address pins, so for example
> we don't explicitly test for an alias at port 0x34, but expect one if
> there's an alias at port 0x30 and another one at port 0x24.

Well, I wouldn't have called it "fused pins", but "not decoded address
bits". But yes. The same was already assumed in the CMOS/RTC patch.

Jan