Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/04/2020 06:28, Jürgen Groß wrote:
> On 23.04.20 19:35, Andrew Cooper wrote:
>> On 21/04/2020 07:02, Jan Beulich wrote:
>>> On 20.04.2020 20:05, Andrew Cooper wrote:
>>>> On 20/04/2020 15:05, Jan Beulich wrote:
>>>>> I'm in particular
>>>>> concerned that we may gain a large number of such printk()s over
>>>>> time, if we added them in such cases.
>>>> The printk() was a bit of an afterthought, but deliberately
>>>> avoiding the
>>>> -EINVAL path was specifically not.
>>>>
>>>> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
>>>> they should see something other than
>>>>
>>>> (XEN) parameter "pv=no-32" unknown!
>>> Why - to this specific build of Xen the parameter is unknown.
>>
>> Because it is unnecessarily problematic and borderline obnoxious to
>> users, as well as occasional Xen developers.
>>
>> "you've not got the correct CONFIG_$X for that to be meaningful" is
>> specifically useful to separate from "I've got no idea".
>>
>>>> I don't think overloading the return value is a clever move, because
>>>> then every parse function has to take care of ensuring that
>>>> -EOPNOTSUPP
>>>> (or ENODEV?) never clobbers -EINVAL.
>>> I didn't suggest overloading the return value. Instead I
>>> specifically want this to go the -EINVAL path.
>>>
>>>> We could have a generic helper which looks like:
>>>>
>>>> static inline void ignored_param(const char *cfg, const char *name,
>>>> const char *s, const char *ss)
>>>> {
>>>>      printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
>>>> cfg, name, s, (int)(ss - s));
>>>> }
>>>>
>>>> which at least would keep all the users consistent.
>>> Further bloating the binary with (almost) useless string literals.
>>> I'd specifically like to avoid this.
>>
>> I don't accept that as a valid argument.
>>
>> We're talking about literally tens of bytes (which will merge anyway, so
>> 0 in practice), and a resulting UI which helps people get out of
>> problems rather than penalises them for having gotten into a problem to
>> begin with.
>>
>> I will absolutely prioritise a more helpful UI over a handful of bytes.
>
> What about a kconfig option (defaulting to "yes") enabling this feature?

IMO, that's literally not worth the bytes taken to implement.

> That way an embedded variant can be made smaller while a server one is
> more user friendly.

There is far lower hanging fruit for an embedded usecase, and its not at
all a foregone conclusion that such a usecase would want the less user
friendly version.  (Embedded very definitely doesn't mean that it won't
have users interacting with the command line).

I would certainly recommend against attempting to speculatively fix
something which isn't a problem, based on guesswork about what a
hypothetical group of people might want while totally ignoring the far
larger savings to be gained by e.g. making CONFIG_INTEL/AMD work.

~Andrew