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

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

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.

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

Thanks, Roger.