Re: [Qemu-devel] [PATCH v3 06/13] kvm: let kvm use AccelState.global_props

[Eduardo Habkost]

On Mon, Jun 19, 2017 at 08:49:41PM +0800, Peter Xu wrote:
> Let KVM be the first user of the new AccelState.global_props field.
> Basically kvm accel only contains accel props for TYPE_X86_CPUs but not
> anything else yet.
> 
> There will be a change on how these global properties are applied for
> TYPE_X86_CPU devices. The general workflow of the global property stuff
> for TYPE_X86_CPU devices can be simplied as following (this is a example
> routine of KVM that contains both old/new workflow, similar thing apply
> to TCG, but even simpler):
> 
>    - HW_COMPAT/type_init() magic played before even entering main() [1]

What do you mean by this?  HW_COMPAT_* is used only in
MachineClass::compat_props[4], and type_init() magic is triggered
by module_call_init(MODULE_INIT_QOM) in main().

>    - main() in vl.c
>      - configure_accelerator()
>        - AccelClass.init_machine() [2]
>          - kvm_init() (for KVM accel)
>      - register global properties
>        - accel_register_compat_props(): register accel compat props [3]
>        - machine_register_compat_props(): register machine compat
>          props (here we'll apply all the HW_COMPAT magic into
>          global_props) [4]
>      - machine init()
>        - cpu init() [5]
>        - ...
> 
> Before this patch, the code setup TYPE_X86_CPU properties at [4] by
> keeping the kvm_default_props list and apply them directly to the device
> using x86_cpu_apply_props().
> 
> After this patch, the code setup the same properties in the sequence of
> [1]->[2]->[3][4]->[5]:
> 
>   - At [1] we setup machine global properties.  Note: there will be
>     properties that with value==NULL but it's okay - when it was applied
>     to global list, it'll be used to remove an entry at step [4], it
>     works just like the old kvm_default_props, but we just unified it to
>     a single place, which is the global_props list.
> 
>   - At [2] we setup accel global properties.

Why isn't AccelClass::global_props set up at class_init(), just
like we do on MachineClass::compat_props?

> 
>   - At [3]/[4] we move machine/accel properties into global property
>     list. One thing to mention is that, we do [3] before [4], since we
>     have some cross-relation properties, e.g., property that is required
>     when both PC=2.1 && ACCEL=kvm happen. For now, all this kind of
>     properties are still in machine compat properties.
> 
>   - At [5] when TYPE_X86_CPU creates, it applies the global property from
>     the global property list, which is now a merged list of three: accel
>     property list, machine property list, and user specified "-global"
>     properties.

On which category above would the x86_cpu_change_kvm_default()
calls in pc_piix.c would be?  We would need to ensure they
override the globals registered by the accel code, but they must
not override the user-provided global properties (including
-global and -cpu options).

This is where things get tricky and fragile: the translation from
-cpu to global properties is done very late inside machine init
today, but we should be able to do that much earlier, once we
refactor the -cpu parsing code.

Hence my suggestion is to not touch x86_cpu_change_kvm_default()
and just move the other properties (everything in
kvm_default_props except svm, x2apic, and kvm-pv-eoi) to a static
AccelClass::global_props field.

> 
> So it's getting more complex in workflow, but better modulized.
> 
> After the refactoring, x86_cpu_change_kvm_default() is moved directly to
> pc_piix.c since that'll be the only place to use it, also it is
> rewritten to use the global property APIs.
> 
> One defect is that we hard coded TYPE_X86_CPU (both of its definitions,
> for 32/64 bits) in accel_register_x86_cpu_props(). Comment explains
> itself.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  accel.c                | 22 ++++++++++++++++++++++
>  hw/i386/pc_piix.c      |  8 ++++++++
>  include/sysemu/accel.h |  9 +++++++++
>  kvm-all.c              | 30 ++++++++++++++++++++++++++++++
>  target/i386/cpu.c      | 42 +-----------------------------------------
>  target/i386/cpu.h      | 11 -----------
>  vl.c                   |  5 +++++
>  7 files changed, 75 insertions(+), 52 deletions(-)
> 
> diff --git a/accel.c b/accel.c
> index f747d65..ca1d0f5 100644
> --- a/accel.c
> +++ b/accel.c
> @@ -130,6 +130,28 @@ void configure_accelerator(MachineState *ms)
>      }
>  }
>  
> +void accel_register_prop(AccelState *accel, const char *driver,
> +                         const char *prop, const char *value)
> +{
> +    accel->global_props = global_prop_list_add(accel->global_props,
> +                                               driver, prop,
> +                                               value, false);
> +}

I don't think we need yet another layer of indirection where
global properties are registered at runtime in AccelState before
being actually registered as global properties.

I expected this to be

  void accel_class_append_global_prop(AccelClass *acc,
                                      const char *driver,
                                      const char *prop,
                                      const char *value);

And the only places calling accel_class_append_global_prop()
would be the accel classes' class_init functions.

I would even consider making AccelClass::global_props an array,
instead of a linked list.  It would help making sure the data is
really static.

> +
> +void accel_register_x86_cpu_props(AccelState *accel, const char *prop,
> +                                  const char *value)
> +{
> +    /*
> +     * We hard-coded this for x86 cpus, trying to match TYPE_X86_CPU.
> +     * We cannot just use TYPE_X86_CPU since that's target-dependent
> +     * while accel.c is target-independent. For x86 platform, only one
> +     * of below two lines will be used, but it does not hurt to
> +     * register both. On non-x86 platforms, none of them are used.
> +     */
> +    accel_register_prop(accel, "i386-cpu", prop, value);
> +    accel_register_prop(accel, "x86_64-cpu", prop, value);

I don't know why we need this helper.  The list of
(driver, property, value) tuples could be hardcoded inside the
initialization of AccelClass::global_props, just like we already
do in MachineClass::compat_props.

> +}
> +
>  static void accel_prop_pass_to_global(gpointer data, gpointer user_data)
>  {
>      GlobalProperty *prop = data;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 46a2bc4..d1d8979 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -304,6 +304,14 @@ static void pc_init1(MachineState *machine,
>      }
>  }
>  
> +static void x86_cpu_change_kvm_default(const char *prop,
> +                                       const char *value)
> +{
> +    if (kvm_enabled()) {
> +        register_compat_prop(TYPE_X86_CPU, prop, value, false);
> +    }

This will be called very late, and can override -cpu options if
we refactor -cpu option parsing in the future.

(It is hard to ensure we have the ordering right if we have too
many register_compat_prop() calls scattered around the code.)


> +}
> +
>  /* Looking for a pc_compat_2_4() function? It doesn't exist.
>   * pc_compat_*() functions that run on machine-init time and
>   * change global QEMU state are deprecated. Please don't create
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 83bb60e..ee2fbad 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -65,8 +65,17 @@ typedef struct AccelClass {
>  
>  extern int tcg_tb_size;
>  
> +typedef struct AccelPropValue {
> +    const char *prop, *value;

Why not add a "driver" field here?

> +} AccelPropValue;
> +
>  void configure_accelerator(MachineState *ms);
>  /* Register accelerator specific global properties */
>  void accel_register_compat_props(AccelState *accel);
> +/* Register single global property to the AccessState property list */
> +void accel_register_prop(AccelState *accel, const char *driver,
> +                         const char *prop, const char *value);
> +void accel_register_x86_cpu_props(AccelState *accel, const char *prop,
> +                                  const char *value);
>  
>  #endif
> diff --git a/kvm-all.c b/kvm-all.c
> index ab8262f..ef60ddf 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1564,6 +1564,34 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
>      return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
>  }
>  
> +static AccelPropValue x86_kvm_default_props[] = {
> +    { "kvmclock", "on" },
> +    { "kvm-nopiodelay", "on" },
> +    { "kvm-asyncpf", "on" },
> +    { "kvm-steal-time", "on" },
> +    { "kvm-pv-eoi", "on" },
> +    { "kvmclock-stable-bit", "on" },
> +    { "x2apic", "on" },
> +    { "acpi", "off" },
> +    { "monitor", "off" },
> +    { "svm", "off" },
> +    { NULL, NULL },

If we add a 'driver' field here, we won't need the
accel_register_x86_cpu_props() helper anymore.

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

I suggest:

* Moving the list inside AccelClass instead of AccelState.
* Doing this inside kvm_accel_class_init() instead.
* Not dealing with x2apic right now, as its rules are more
  complex.

> +
> +    if (!kvm_irqchip_in_kernel()) {
> +        accel_register_x86_cpu_props(accel, "x2apic", "off");
> +    }
> +}
> +
>  static int kvm_init(MachineState *ms)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -1776,6 +1804,8 @@ static int kvm_init(MachineState *ms)
>  
>      cpu_interrupt_handler = kvm_handle_interrupt;
>  
> +    kvm_register_accel_props(s);
> +
>      return 0;
>  
>  err:
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1bd20e2..5214fba 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1484,23 +1484,6 @@ typedef struct PropValue {
>      const char *prop, *value;
>  } PropValue;
>  
> -/* KVM-specific features that are automatically added/removed
> - * from all CPU models when KVM is enabled.
> - */
> -static PropValue kvm_default_props[] = {
> -    { "kvmclock", "on" },
> -    { "kvm-nopiodelay", "on" },
> -    { "kvm-asyncpf", "on" },
> -    { "kvm-steal-time", "on" },
> -    { "kvm-pv-eoi", "on" },
> -    { "kvmclock-stable-bit", "on" },
> -    { "x2apic", "on" },
> -    { "acpi", "off" },
> -    { "monitor", "off" },
> -    { "svm", "off" },
> -    { NULL, NULL },
> -};
> -
>  /* TCG-specific defaults that override all CPU models when using TCG
>   */
>  static PropValue tcg_default_props[] = {
> @@ -1508,23 +1491,6 @@ static PropValue tcg_default_props[] = {
>      { NULL, NULL },
>  };
>  
> -
> -void x86_cpu_change_kvm_default(const char *prop, const char *value)
> -{
> -    PropValue *pv;
> -    for (pv = kvm_default_props; pv->prop; pv++) {
> -        if (!strcmp(pv->prop, prop)) {
> -            pv->value = value;
> -            break;
> -        }
> -    }
> -
> -    /* It is valid to call this function only for properties that
> -     * are already present in the kvm_default_props table.
> -     */
> -    assert(pv->prop);
> -}
> -
>  static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>                                                     bool migratable_only);
>  
> @@ -2335,13 +2301,7 @@ static void x86_cpu_load_def(X86CPU *cpu, 
> X86CPUDefinition *def, Error **errp)
>      }
>  
>      /* Special cases not set in the X86CPUDefinition structs: */
> -    if (kvm_enabled()) {
> -        if (!kvm_irqchip_in_kernel()) {
> -            x86_cpu_change_kvm_default("x2apic", "off");
> -        }
> -
> -        x86_cpu_apply_props(cpu, kvm_default_props);
> -    } else if (tcg_enabled()) {
> +    if (tcg_enabled()) {
>          x86_cpu_apply_props(cpu, tcg_default_props);
>      }
>  
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index de0551f..93aebfb 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1669,17 +1669,6 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess 
> access);
>  void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
>                                     TPRAccess access);
>  
> -
> -/* Change the value of a KVM-specific default
> - *
> - * If value is NULL, no default will be set and the original
> - * value from the CPU model table will be kept.
> - *
> - * It is valid to call this function only for properties that
> - * are already present in the kvm_default_props table.
> - */
> -void x86_cpu_change_kvm_default(const char *prop, const char *value);
> -
>  /* mpx_helper.c */
>  void cpu_sync_bndcs_hflags(CPUX86State *env);
>  
> diff --git a/vl.c b/vl.c
> index d3f5594..a564139 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4553,6 +4553,11 @@ int main(int argc, char **argv, char **envp)
>              exit (i == 1 ? 1 : 0);
>      }
>  
> +    /*
> +     * Here we need to first register accelerator compat properties
> +     * then machine properties, since cross-relationed properties are
> +     * setup in machine compat bits.
> +     */
>      accel_register_compat_props(current_machine->accelerator);
>      machine_register_compat_props(current_machine);

Note for later: It would be very nice if this was the only place
in the code where qdev_prop_register_global()/register_compat_prop()
is called.  Probably a register_user_provided_compat_props() call
here would make sure the ordering is always right.

-- 
Eduardo