Re: [Qemu-devel] [PATCH v3 04/13] accel: introduce AccelState.global_props

[Eduardo Habkost]

On Tue, Jun 20, 2017 at 09:20:36PM +0800, Peter Xu wrote:
> On Mon, Jun 19, 2017 at 01:17:39PM -0300, Eduardo Habkost wrote:
> > On Mon, Jun 19, 2017 at 08:49:39PM +0800, Peter Xu wrote:
> > > Introduce this new field for the accelerator instances so that each
> > > specific accelerator in the future can register its own global
> > > properties to be used further by the system.  It works just like how the
> > > old machine compatible properties do, but only tailored for
> > > accelerators.
> > > 
> > > Use the newly exported register_compat_prop() to pass the accelerator
> > > global properties to the global_props list.
> > > 
> > > Signed-off-by: Peter Xu <address@hidden>
> > 
> > As noted on other patches, I believe this should be
> > AccelClass::global_props (initialized statically in class_init)
> > instead of AccelState::global_props (built at runtime).
> > Otherwise, I don't see the benefit of the new field.
> 
> The reason is that there is property that can only be defined after
> user specified all the parameters (when kvm irqchip is enabled). See:
> 
> static void kvm_register_accel_props(KVMState *kvm)
> {
>     AccelState *accel = ACCEL(kvm);
>     AccelPropValue *entry;
> 
>     for (entry = x86_kvm_default_props; entry->prop; entry++) {
>         accel_register_x86_cpu_props(accel, entry->prop, entry->value);
>     }
> 
>     if (!kvm_irqchip_in_kernel()) {
>         accel_register_x86_cpu_props(accel, "x2apic", "off");
>     }
> }
> 
> So the list is not really static when class init.  Thanks,

I suggest leaving x2apic for later, and address the ones that are
static first.  I'm not sure a dynamic list on AccelState is the
right solution for x2apic.  I still wish to find a way to
represent the x2apic/svm/kvm-pv-eoi rules using static data.

-- 
Eduardo