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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, 15 Nov 2010, Alexander Graf wrote:

>
> 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.

I will do this change, as well as for the two others patches.

> > +};
> > +
> > +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.

Yes, this will make the code more readable.

> 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 
> :).

And I will send the patch out of the patch series.

Thanks,

-- 
Anthony PERARD

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