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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2020 17:50, Andrew Cooper wrote:
> This is the start of some performance and security-hardening improvements,
> based on the fact that 32bit PV guests are few and far between these days.
> 
> Ring1 is full or architectural corner cases, such as counting as supervisor

... full of ...

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1694,7 +1694,17 @@ The following resources are available:
>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>      sum of CBMs is fixed, that means actual `cos_max` in use will 
> automatically
>      reduce to half when CDP is enabled.
> -     
> +
> +### pv
> +    = List of [ 32=<bool> ]
> +
> +    Applicability: x86
> +
> +Controls for aspects of PV guest support.
> +
> +*   The `32` boolean controls whether 32bit PV guests can be created.  It
> +    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.

Rather than ignoring in the way you do, how about ...

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -16,6 +16,39 @@
>  #include <asm/pv/domain.h>
>  #include <asm/shadow.h>
>  
> +#ifdef CONFIG_PV32
> +int8_t __read_mostly opt_pv32 = -1;
> +#endif
> +
> +static int parse_pv(const char *s)
> +{
> +    const char *ss;
> +    int val, rc = 0;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        if ( (val = parse_boolean("32", s, ss)) >= 0 )
> +        {
> +#ifdef CONFIG_PV32
> +            opt_pv32 = val;
> +#else
> +            printk(XENLOG_INFO
> +                   "CONFIG_PV32 disabled - ignoring 'pv=32' setting\n");
> +#endif
> +        }

... moving the #ifdef ahead of the if(), and the #endif here (may
want converting to "else if" with a placeholder if(0) for this
purpose), with the separate printk() dropped? 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. See parse_iommu_param() for
how I'd prefer things to look like in the long run.

Jan