[Xen-devel] Re: [Qemu-devel] [PATCH V6 04/15] Introduce -accel command option.

[Alexander Graf <agraf@xxxxxxx>]

On 21.10.2010, at 19:36, Anthony.Perard@xxxxxxxxxx wrote:

> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> This option gives the ability to switch one "accelerator" like kvm, xen
> or the default one tcg. We can specify more than one accelerator by
> separate them by a comma. QEMU will try each one and use the first whose
> works.
> 
> So,
> 
> -accel xen,kvm,tcg
> 
> which would try Xen support first, then KVM and finaly tcg if none of
> the other works.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> qemu-options.hx |   10 ++++++
> vl.c            |   86 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 85 insertions(+), 11 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 718d47a..ba8385b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1925,6 +1925,16 @@ Enable KVM full virtualization support. This option is 
> only available
> if KVM support is enabled when compiling.
> ETEXI
> 
> +DEF("accel", HAS_ARG, QEMU_OPTION_accel, \
> +    "-accel accel    use an accelerator (kvm,xen,tcg), default is tcg\n", 
> QEMU_ARCH_ALL)
> +STEXI
> +@item -accel @var{accel}[,@var{accel}[,...]]
> +@findex -accel
> +This is use to enable an accelerator, in kvm,xen,tcg.
> +By default, it use only tcg. If there a more than one accelerator
> +specified, the next one is used if the first don't work.
> +ETEXI
> +
> DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
>     "-xen-domid id   specify xen guest domain id\n", QEMU_ARCH_ALL)
> DEF("xen-create", 0, QEMU_OPTION_xen_create,
> diff --git a/vl.c b/vl.c
> index df414ef..40a26ee 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1750,6 +1750,74 @@ static int debugcon_parse(const char *devname)
>     return 0;
> }
> 
> +static struct {
> +    const char *opt_name;
> +    const char *name;
> +    int (*available)(void);
> +    int (*init)(int smp_cpus);
> +    int *allowed;
> +} accel_list[] = {
> +    { "tcg", "tcg", NULL, NULL, NULL },
> +    { "kvm", "KVM", kvm_available, kvm_init, &kvm_allowed },

Thinking about this a bit more ...

kvm_available is a function pointer that gets #defined to (0) when we don't 
have KVM available. I can imagine some compiler might throw a warning on us for 
this one day.

Is there a valid reason not to do

static inline int kvm_enabled()
{
#ifdef CONFIG_KVM
    return kvm_allowed;
#else
    return 0;
#endif
}

That should compile into the exact same code but be valid for function pointers.

> +};
> +
> +static int accel_parse_init(const char *opts)
> +{
> +    const char *p = opts;
> +    char buf[10];
> +    int i, ret;
> +    bool accel_initalised = 0;
> +    bool init_failed = 0;
> +
> +    while (!accel_initalised && *p != '\0') {
> +        if (*p == ',') {
> +            p++;
> +        }
> +        p = get_opt_name(buf, sizeof (buf), p, ',');
> +        for (i = 0; i < ARRAY_SIZE(accel_list); i++) {
> +            if (strcmp(accel_list[i].opt_name, buf) == 0) {
> +                if (accel_list[i].init) {

If you'd make the function pointers mandatory, you could also get rid of a lot 
of checks here. Just introduce a new small stub helper for tcg_init and 
tcg_allowed and always call the pointers.


I take back my Acked-by. Sorry, I guess I should have read things in more 
detail first O_o. I still believe that this patch can go in before the others, 
but I'd at least like to see some comments on the (0) pointer thing :).


Alex


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel