Re: [Xen-devel] [PATCH] common/gnttab: Introduce command line feature controls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.02.18 at 15:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -916,6 +916,19 @@ Controls EPT related features.
>  
>  Specify which console gdbstub should use. See **console**.
>  
> +### gnttab
> +> `= List of [ max_ver:<integer>, transitive=<bool> ]`

I realize you don't want to change this as people already use it, but
I'd still like to give my usual comment: I'd prefer if we could avoid
introducing further underscore-containing (sub)options. I really don't
understand why everyone does this: Dashes are easier to type on
all keyboards I'm aware of, and there's no need to mimic C identifier
names for command line options.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -97,6 +97,40 @@ static unsigned int __read_mostly max_maptrack_frames =
>                                                 DEFAULT_MAX_MAPTRACK_FRAMES;
>  integer_runtime_param("gnttab_max_maptrack_frames", max_maptrack_frames);
>  
> +static unsigned int __read_mostly opt_gnttab_max_version = 2;
> +static bool __read_mostly opt_transitive_grants = true;
> +
> +static int __init parse_gnttab(const char *s)
> +{
> +    const char *ss;
> +    int val, rc = 0;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        if ( !strncmp(s, "max_ver:", 8) )
> +        {
> +            long ver = simple_strtol(s + 8, NULL, 10);

In particular with you already having determined the intended end
of the number, wouldn't it make sense to refuse non-number input,
by checking ss against what simple_strtol() would provide if the
middle parameter wasn't NULL?

> @@ -3424,7 +3459,10 @@ do_grant_table_op(
>          break;
>  
>      case GNTTABOP_set_version:
> -        rc = gnttab_set_version(guest_handle_cast(uop, 
> gnttab_set_version_t));
> +        if ( opt_gnttab_max_version == 1 )
> +            rc = -ENOSYS; /* Behave as before set_version was introduced. */
> +        else
> +            rc = gnttab_set_version(guest_handle_cast(uop, 
> gnttab_set_version_t));
>          break;

I can sort of see why you want it this way, but why do you mean to
penalize any guest simply setting the version to 1 regardless of its
current setting (like might e.g. be needed in kexec-like situations)?
Guests may assume the availability of set_version by looking at the
Xen version number (whether that's an entirely valid thing to do is
a separate question).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel