Re: [Qemu-devel] [PATCH v6 02/17] Add accelerator id and model name to CPUState

[Eduardo Habkost]

On Thu, May 07, 2015 at 09:55:45AM +0200, Michael Mueller wrote:
> On Wed, 6 May 2015 08:41:50 -0300
> Eduardo Habkost <address@hidden> wrote:
[...]
> > > > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > > > index 108bfa2..457afc7 100644
> > > > > --- a/qom/cpu.c
> > > > > +++ b/qom/cpu.c
> > > > > @@ -67,6 +67,20 @@ CPUState *cpu_generic_init(const char *typename, 
> > > > > const char *cpu_model)
> > > > >          goto out;
> > > > >      }
> > > > >  
> > > > > +    if (tcg_enabled()) {
> > > > > +        cpu->accel_id = ACCEL_ID_TCG;
> > > > > +    } else if (kvm_enabled()) {
> > > > > +        cpu->accel_id = ACCEL_ID_KVM;
> > > > > +    }
> > > > > +#ifdef CONFIG_XEN
> > > > > +    else if (xen_enabled()) {
> > > > > +        cpu->accel_id = ACCEL_ID_XEN;
> > > > > +    }
> > > > > +#endif
> > > > > +    else {
> > > > > +        cpu->accel_id = ACCEL_ID_QTEST;
> > > > > +    }
> > > > 
> > > > You can simply use ACCEL_GET_CLASS(current_machine->accelerator)->name
> > > > here. If we really want an enum, we can add an AccelId field to
> > > > AccelClass, and initialize it properly on the accel class_init
> > > > functions.
> > > 
> > > The AccelClass (ac = ACCEL_GET_CLASS(current_machine->accelerator) would 
> > > be ok though.
> > > That will allow to access the ac->accel_id (not yet there) at places 
> > > where required.
> > > 
> > > I'm just not sure how to access current_machine here.
> > 
> > It is a global variable declared in hw/boards.h. But it makes sense only
> > if !CONFIG_USER.
> > 
> > But I am starting to wonder if we shouldn't simply expose accel info as
> > a /mmachine property, instead of per-CPU information. It may become
> > per-CPU in the future, but right now we really don't support having
> > multiple accelerators so why are we trying to pretend we do?
> > 
> 
> That brings me back into the direction I was before with the proposed 
> query-cpu-model
> QMP call... but that's OK. In the end that holds true also for the cpu model 
> name as
> well...

That's probably true with the current command-line options and current
machine implementations, but CPU model information is already inside the
CPU objects. Accel information, on the other hand, is still in global
variables such as kvm_allowed and current_machine.

I still believe per-CPU accel links are the right way to go in the
future, but it's not how the accelerator code works today.

That means I am OK with both approaches: exposing CPU::accel info, or
MachineState::accel info. The difference is that /machine/accel is
simpler to implement today.

-- 
Eduardo