WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Anthony.Perard@xxxxxxxxxx
Subject: [Xen-devel] Re: [Qemu-devel] [PATCH V6 04/15] Introduce -accel command option.
From: Alexander Graf <agraf@xxxxxxx>
Date: Mon, 15 Nov 2010 11:46:55 +0100
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, QEMU-devel Developers <qemu-devel@xxxxxxxxxx>, Anthony Liguori <anthony@xxxxxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Delivery-date: Mon, 15 Nov 2010 02:47:53 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1287682587-18642-5-git-send-email-anthony.perard@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1287682587-18642-1-git-send-email-anthony.perard@xxxxxxxxxx> <1287682587-18642-5-git-send-email-anthony.perard@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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