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
