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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2020 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.

How will they merge? The different instances of the format string
above may/should, yes, but the different "CONFIG_xyz" literals the
callers pass won't, for example.

> I will absolutely prioritise a more helpful UI over a handful of bytes.

This "a handful of bytes doesn't matter" attitude has lead to
imo unacceptable growth of various software packages over the
years.

Anyway - I don't want to block this change over this argument,
so I'm willing to ack a patch with the helper introduced (and
preferably the "CONFIG_" part of the cfg arguments moved into
the helper's format string), as long as we plan to then make
consistent use of the helper everywhere. That said, I don't
immediately see what would be passed for "cfg" in some of
parse_iommu_param()'s cases.

Jan