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

[Alexander Graf]

On 15.11.2010, at 15:47, Anthony PERARD wrote:

> On Mon, 15 Nov 2010, Anthony PERARD wrote:
> 
>> On Mon, 15 Nov 2010, Alexander Graf wrote:
>> 
>>> 
>>> On 21.10.2010, at 19:36, address@hidden wrote:
>>> 
>>>> From: Anthony PERARD <address@hidden>
>>>> 
>>>> 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 <address@hidden>
>>>> ---
>>>> qemu-options.hx |   10 ++++++
>>>> vl.c            |   86 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++-------
>>>> 2 files changed, 85 insertions(+), 11 deletions(-)
>>>> 
>>>> 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.
> 
> Actually, kvm_available is already a function, not a define.
> 
> kvm_enable can be change from define to function, but not in this patch,
> because I don't use it.

Oh no worries for stuff you don't use. As long as kvm_available is a function, 
just make sure that xen_available is one too :). The main incentive is to get 
rid of all those if (!x->available) parts, as we can guarantee there to always 
be a function behind. 

Treating tcg the same way would also help in the long term for non-tcg build of 
qemu which some people are interested in ;).


Alex