Re: [Xen-devel] [PATCH 7/8] x86/SVM: Introduce svm command line option

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/18 00:01, Janakarajan Natarajan wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>
> This patch introduces a new Xen command line option to enable/disable
> SVM sub-options. Currently, it support sub-option "avic", which can
> be used to enable/disable SVM AVIC feature.
>
> Signed-off-by: Suavee Suthikulpant <suravee.suthikulpanit@xxxxxxx>
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@xxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown | 16 ++++++++++++++++
>  xen/arch/x86/hvm/svm/svm.c          | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index b353352adf..60a1005c42 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1730,6 +1730,22 @@ enforces the maximum theoretically necessary timeout 
> of 670ms. Any number
>  is being interpreted as a custom timeout in milliseconds. Zero or boolean
>  false disable the quirk workaround, which is also the default.
>  
> +### svm
> +> `= List of [ avic ]`
> +
> +> Sub-options:
> +
> +> All sub-options are of boolean kind and can be prefixed with `no-` to
> +> effect the inverse meaning.
> +
> +> `avic`
> +
> +> Default: `false`
> +
> +>> This option enables Advanced Virtual Interrupt Controller (AVIC),
> +>> which is an extension of AMD Secure Virtual Machine (SVM) to virtualize
> +>> local APIC for guest VM.
> +
>  ### sync\_console
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index fdbe8e3008..0c5c26cce8 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -64,6 +64,16 @@
>  #include <asm/monitor.h>
>  #include <asm/xstate.h>
>  
> +static int parse_svm_param(const char *s);
> +
> +/*
> + * The 'svm' parameter en/dis-ables various SVM features.
> + * Optional comma separated value may contain:
> + *
> + *   avic - Enable SVM Advanced Virtual Interrupt Controller (AVIC)
> + */
> +custom_param("svm", parse_svm_param);

Move this custom_param to below the definition, and drop the forward
declaration.  You can drop the comment here, because the important
information is in the real command line doc.

> +
>  void svm_asm_do_resume(void);
>  
>  u32 svm_feature_flags;
> @@ -89,6 +99,28 @@ static bool_t amd_erratum383_found __read_mostly;
>  static uint64_t osvw_length, osvw_status;
>  static DEFINE_SPINLOCK(osvw_lock);
>  
> +static int __init parse_svm_param(const char *s)
> +{
> +    char *ss;
> +    int val;
> +
> +    do {
> +        val = !!strncmp(s, "no-", 3);
> +        if ( !val )
> +            s += 3;
> +
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';

This is writing to a const pointer.  strchr() is a particularly evil
part of the C standard library, because it drops const from its input
parameter.

See parse_xen_cpuid() for a similar piece of functionality, and please
use the parse_boolean() helper.

~Andrew

> +
> +        if ( !strcmp(s, "avic") )
> +            svm_avic = val;
> +
> +        s = ss + 1;
> +    } while ( ss );
> +
> +    return 0;
> +}
>  /* Only crash the guest if the problem originates in kernel mode. */
>  static void svm_crash_or_fault(struct vcpu *v)
>  {


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