Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Igor Mammedov]

On Thu,  7 Nov 2013 20:11:51 +1100
Alexey Kardashevskiy <address@hidden> wrote:

> On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb 
> Paolo Bonzini:
> >> Il 05/11/2013 10:16, Alexander Graf ha scritto:
> >>>
> >>> On 05.11.2013, at 10:06, Paolo Bonzini <address@hidden> wrote:
> >>>
> >>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
> >>>>>>> Why is the option under -machine instead of -cpu?
> >>>>> Because it is still the same CPU and the guest will still read the real
> >>>>> PVR from the hardware (which it may not support but this is why we need
> >>>>> compatibility mode).
> >>>>
> >>>> How do you support migration from a newer to an older CPU then?  I think
> >>>> the guest should never see anything about the hardware CPU model.
> >>>
> >>> POWER can't model that. It always leaks the host CPU information into the 
> >>> guest. It's the guest kernel's responsibility to not expose that change 
> >>> to user space.
> >>>
> >>> Yes, it's broken :). I'm not even sure there is any sensible way to do 
> >>> live migration between different CPU types.
> >>
> >> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
> >> just a "virtual" CPU model.
> >
> > PowerPC currently does not have -cpu option parsing. If you need to
> > implement it, I would ask for a generic hook in CPUClass set by
> > TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
> > and for the p=v parsing logic to be so generic as to just set property p
> > to value v on the CPU instance. I.e. please make the compatibility
> > settings a static property or dynamic property of the CPU.
> >
> > Maybe the parsing code could even live in generic qom/cpu.c, overridden
> > by x86/sparc and reused for arm? Somewhere down my to-do list but
> > patches appreciated...
> 
> 
> I spent some time today trying to digest what you said, still having problems
> with understanding of what you meant and what Igor meant about global 
> variables
> (I just do not see the point in them).
> 
> Below is the result of my excercise. At the moment I would just like to know
> if I am going in the right direction or not.

what I've had in mind was a bit simpler and more implicit approach instead of
setting properties on each CPU instance explicitly. It could done using
existing "global properties" mechanism.

in current code -cpu type,foo1=x,foo2=y... are saved into cpu_model string
which is parsed by target specific cpu_init() effectively parsing cpu_model
each time when creating a CPU.

So to avoid fixing every target I suggest to leave cpu_model be as it is and
add translation hook that will convert "type,foo1=x,foo2=y..." virtually
into a set of following options:
 "-global type.foo1=x -global type.foo2=y ..."
these options when registered are transparently applied to each new CPU
instance (see device_post_init() for details).

now why do we need translation hook,
 * not every target is able to handle "type,foo1=x,foo2=y" in terms of
   properties yet
 * legacy feature string might be in non canonical format, like
   "+foo1,-foo2..." so for compatibility reasons with CLI we need target
   specific code to convert to canonical format when it becomes available.
 * for targets that don't have feature string handling and implementing
   new features as properties we can implement generic default hook that
   will convert canonical feature string into global properties.

as result we eventually would be able drop cpu_model and use global
properties to store CPU features.

see comments below for pseudo code:
> And few question while we are here:
> 1. the proposed common code handles both static and dynamic properties.
> What is the current QEMU trend about choosing static vs. dynamic? Can do
> both in POWERPC, both have benifits.
I prefer static, since it's usually less boilerplate code.


[...]

> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7739e00..a17cd73 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState 
> *cpu, vaddr addr)
>  #endif
>  
>  /**
> + * cpu_common_set_properties:
> + * @cpu: The CPU whose properties to initialize from the command line.
> + */
> +int cpu_common_set_properties(Object *obj);

cpu_translate_features_compat(classname, cpu_model_str)


> diff --git a/qom/cpu.c b/qom/cpu.c
> index 818fb26..6516c63 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -24,6 +24,8 @@
>  #include "qemu/notify.h"
>  #include "qemu/log.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/qmp/qerror.h"
>  
>  bool cpu_exists(int64_t id)
>  {
> @@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
>      }
>  }
>  
> +static int cpu_set_property(const char *name, const char *value, void 
> *opaque)
> +{
> +    DeviceState *dev = opaque;
> +    Error *err = NULL;
> +
> +    if (strcmp(name, "type") == 0)
> +        return 0;
> +
> +    qdev_prop_parse(dev, name, value, &err);
> +    if (err != NULL) {
> +        qerror_report_err(err);
> +        error_free(err);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +int cpu_common_set_properties(Object *obj)
> +{
> +    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
> +}
replace ^^^ with:

void cpu_translate_features_compat(classname, cpu_model_str) {
    for_each_foo in cpu_model_str {
        qdev_prop_register_global(classname.foo=val)
    }
}

and set default hook to NULL for every target that does custom
parsing (i.e. x86/sparc) so hook will be NOP there.


> diff --git a/vl.c b/vl.c
> index d5c5d8c..a5fbc38 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
>      },
>  };
>  

>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */

cpu_model = optarg;
classname = GET_CLASS_NAME_FROM_TYPE(cpu_model) <= not sure how implement it 
since naming is target specific, maybe Andreas has an idea
CPUClass = get_cpu_class_by_name(classname)
class->cpu_translate_features_compat(classname, cpu_model_str)

>                  break;
>              case QEMU_OPTION_hda:
>                  {