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

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

On Mon, Oct 30, 2023 at 01:24:52PM +0100, Jan Beulich wrote:
> On 26.10.2023 17:19, Roger Pau Monné wrote:
> > On Thu, Oct 26, 2023 at 05:07:18PM +0200, Jan Beulich wrote:
> >> 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?
> > 
> > Not sure what we bypass in pvshim mode, would be good to double
> > check.
> > 
> >> - 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).
> > 
> > It being Andrew the one that made the request, I would like to get his
> > opinion on it.  UNCONSTRAINED does seem a bit weird.
> 
> I agree that the name is odd; I couldn't think of any better one (and
> this already is the result of 3 or 4 rounds of renaming). I'll be more
> than happy to consider other naming suggestions. The main issue with the
> present option is, just to re-state it here, that we have grown negative
> dependencies on it, which is a problem.
> 
> However, meanwhile I've realized that we don't really want to tie anything
> here to PV_SHIM_EXCLUSIVE, at least not directly. What we care about is
> whether we _actually_ run in shim mode, i.e. also when a full-fledged
> hypervisor is in use. My plan is now to have said new command line option,
> which - if not specified on the command line - I'd default to !pv_shim.

So that tests for aliases are run unless in pv shim mode.

> > Maybe the issue is that PV_SHIM_EXCLUSIVE shouldn't have been a
> > Kconfig option in the first place, and instead a specific Kconfig
> > config file?
> > 
> > Maybe it's not possible to achieve the same using just a Kconfig
> > config file.
> 
> I'm afraid I don't understand what you mean by "Kconfig config file". It
> can't really be just another .../Kconfig file somewhere in the tree, as
> it doesn't really matter where an option like this would be defined.

No, I was thinking of splitting what PV_SHIM_EXCLUSIVE actually
implies, for example by adding CONFIG_DOMCTL_HYPERCALLL or
CONFIG_PLATFORM_HYPERCALL and re-work the pvshim_defconfig config file
based on those, so that we don't end up with negative relations.

Note sure all usages of PV_SHIM_EXCLUSIVE can be split in such a way,
maybe we would need some compromise.

Thanks, Roger.