Re: [PATCH 2/3] x86/pv-shim: don't even allow enabling GRANT_TABLE

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@suse.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Roger Pau Monne <roger.pau@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/3] x86/pv-shim: don't even allow enabling GRANT_TABLE
Date: Wed, 7 Dec 2022 10:35:18 +0100	[thread overview]
Message-ID: <780ba615-a375-f32c-705d-64560133085b@suse.com> (raw)
In-Reply-To: <e5fa33d5-a82a-606c-2ab0-013a9ed72c08@suse.com>

On 07.12.2022 10:11, Juergen Gross wrote:
> On 07.12.22 09:55, Jan Beulich wrote:
>> On 07.12.2022 08:21, Jan Beulich wrote:
>>> On 06.12.2022 21:26, Andrew Cooper wrote:
>>>> On 06/12/2022 14:30, Jan Beulich wrote:
>>>>> Grant table code is unused in shim mode, so there's no point in
>>>>> building it in the first place for shim-exclusive mode.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> nack.
>>>>
>>>> This is bogus, as is every other "depends on !PV_SHIM_EXCLUSIVE".
>>>
>>> But why? Doing things like this in Kconfig is exactly ...
>>>
>>>> The only reason I haven't reverting the others so `make allyesconfig`
>>>> doesn't disable CONFIG_HVM, is because I haven't had time.  This change
>>>> further breaks allyesconfig by disabling GRANT_TABLE too.
>>>>
>>>> PV_SHIM_EXCLUSIVE is a simple option for a bit of dead code
>>>> elimination.  It is not valid to be used like this.
>>>
>>> ... for the purpose of dead code elimination. By nack-ing a change like
>>> this (and by having voiced opposition to earlier ones) you simply call
>>> for yet more entirely unhelpful #ifdef-ary. See the last paragraph of
>>> the description of patch 1, half of which this change rectifies. The
>>> solution on the evtchn side, unfortunately, looks to be #ifdef-ary,
>>> short of there being a suitable Kconfig option.
>>>
>>> Furthermore Kconfig, in my view, is specifically intended to allow to
>>> prevent the user from selecting entirely bogus option combinations
>>> (and even more so suggest entirely bogus configurations by bogus
>>> default settings). If you disagree, then I'm afraid we have a 2nd
>>> Kconfig usage topic which we need to settle on in a project-wide
>>> manner. If only we ever made any progress on such ...
>>>
>>> As to allyesconfig - I can see your point there, but then arrangements
>>> need to be invented to avoid this kind of unhelpful behavior.
>>
>> Thinking more about it, if allyesconfig is meant to be useful, then
>> any "depends on !..." (other than for base architecture identifiers)
>> would be wrong (see e.g. COVERAGE depending on !LIVEPATCH or
>> ARM_SMMU_V3 depending on !ACPI). And this would extend to Linux as
>> well - how do they deal with that?
> 
> Isn't allyesconfig for new options only? At least in Linux it is
> documented this way.

Is it? I only find

     "make allyesconfig"
                        Create a ./.config file by setting symbol
                        values to 'y' as much as possible.

and a couple of instances of "New config where all options are accepted
with yes". What you refer to ...

> Otherwise options like CONFIG_X86_32 (which depends on !X86_64) would make
> no sense either.
> 
> So the way to go seems to have some default minimal configs (in our case
> e.g. shim and no-shim), which can then be expanded via allyesconfig or
> allnoconfig.

... looks to be optional behavior with KCONFIG_ALLCONFIG (which of
course we could leverage for our CI in order to avoid introducing
restrictions on what may be used in "depends on").

Jan