Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as properties to QemuMachineState

Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as properties to QemuMachineState

[Marcel Apfelbaum]

On Fri, 2014-05-30 at 16:45 -0300, Eduardo Habkost wrote:
> On Mon, May 26, 2014 at 03:40:58PM +0300, Marcel Apfelbaum wrote:
> > Make machine's QemuOpts QOM properties of machine. The properties
> > are automatically filled in. This opens the possiblity to create
> > opts per machine rather than global.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/core/machine.c   | 256 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/boards.h |   6 +-
> >  vl.c                |  10 +-
> >  3 files changed, 266 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index d3ffef7..dbcf2a1 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -11,6 +11,260 @@
> >   */
> >  
> >  #include "hw/boards.h"
> > +#include "qapi/visitor.h"
> > +
> > +static char *machine_get_accel(Object *obj, Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    return g_strdup(ms->accel);
> > +}
> > +
> > +static void machine_set_accel(Object *obj, const char *value, Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +    ms->accel = g_strdup(value);
> > +}
> 
> You are not freeing the old value here and on the other string setters.
It seems to be the caller responsibility.
> 
> (Do we have some existing common wrapper to automatically free the old
> value and set the new one, or every setter should duplicate the same
> free/strdup logic?)
We don't have yet, from what I know, and this is for the moment the QMP way
to handle string attributes.

Thanks,
Marcel 
> 

Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as properties to QemuMachineState

[Marcel Apfelbaum]

On Fri, 2014-05-30 at 16:25 -0300, Eduardo Habkost wrote:
> On Mon, May 26, 2014 at 03:40:58PM +0300, Marcel Apfelbaum wrote:
> [...]
> > +static void machine_initfn(Object *obj)
> > +{
> > +    object_property_add_str(obj, "accel",
> > +                            machine_get_accel, machine_set_accel, NULL);
> > +    object_property_add_bool(obj, "kernel_irqchip",
> > +                             machine_get_kernel_irqchip,
> > +                             machine_set_kernel_irqchip,
> > +                             NULL);
> 
> In the case of kernel_irqchip, the information contained in MachineState
> is not a superset of the information contained on
> qemu_get_machine_opts().
> 
> See hw/ppc/{e500,spapr}.c. They use kernel_irqchip like this:
> 
>     bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
>                                             "kernel_irqchip", true);
>     bool irqchip_required = qemu_opt_get_bool(machine_opts,
>                                               "kernel_irqchip", false);
> 
>     if (irqchip_allowed) {
>         dev = ppce500_init_mpic_kvm(params, irqs);
>     }
> 
>     if (irqchip_required && !dev) {
>         fprintf(stderr, "%s: irqchip requested but unavailable\n",
>                 __func__);
>         abort();
>     }
> 
> This means kernel_irqchip have three possible states: "disabled", "required",
> and "allowed".
I already had a patch adding "property_is_set" to QMP, but was not accepted
as there is no way yet to "unset" a property. (I can point you to the series)

> 
> This means that MachineState.kernel_irqchip is not usable by current
> code that uses the kernel_irqchip option. I suppose we plan to address
> this on MachineState, too, to not get stuck with a global
> qemu_get_machine_opts() forever?
I completely agree with you and I already had a patch tackling it,
based on "property_is_set", but was no accepted yet, obviously.
I was instructed to set the default value in the machine init function
and some way (I don't remember now) to emulate  required/allowed.

I do plan to get back to this.

Thanks,
Marcel
> 

Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as properties to QemuMachineState

[Eduardo Habkost]

On Sun, Jun 01, 2014 at 11:21:49AM +0300, Marcel Apfelbaum wrote:
> On Fri, 2014-05-30 at 16:25 -0300, Eduardo Habkost wrote:
> > On Mon, May 26, 2014 at 03:40:58PM +0300, Marcel Apfelbaum wrote:
> > [...]
> > > +static void machine_initfn(Object *obj)
> > > +{
> > > +    object_property_add_str(obj, "accel",
> > > +                            machine_get_accel, machine_set_accel, NULL);
> > > +    object_property_add_bool(obj, "kernel_irqchip",
> > > +                             machine_get_kernel_irqchip,
> > > +                             machine_set_kernel_irqchip,
> > > +                             NULL);
> > 
> > In the case of kernel_irqchip, the information contained in MachineState
> > is not a superset of the information contained on
> > qemu_get_machine_opts().
> > 
> > See hw/ppc/{e500,spapr}.c. They use kernel_irqchip like this:
> > 
> >     bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> >                                             "kernel_irqchip", true);
> >     bool irqchip_required = qemu_opt_get_bool(machine_opts,
> >                                               "kernel_irqchip", false);
> > 
> >     if (irqchip_allowed) {
> >         dev = ppce500_init_mpic_kvm(params, irqs);
> >     }
> > 
> >     if (irqchip_required && !dev) {
> >         fprintf(stderr, "%s: irqchip requested but unavailable\n",
> >                 __func__);
> >         abort();
> >     }
> > 
> > This means kernel_irqchip have three possible states: "disabled", "required",
> > and "allowed".
> I already had a patch adding "property_is_set" to QMP, but was not accepted
> as there is no way yet to "unset" a property. (I can point you to the series)
> 
> > 
> > This means that MachineState.kernel_irqchip is not usable by current
> > code that uses the kernel_irqchip option. I suppose we plan to address
> > this on MachineState, too, to not get stuck with a global
> > qemu_get_machine_opts() forever?
> I completely agree with you and I already had a patch tackling it,
> based on "property_is_set", but was no accepted yet, obviously.
> I was instructed to set the default value in the machine init function
> and some way (I don't remember now) to emulate  required/allowed.

I don't see a need to change to the object model and API. Just add
MachineState-specific properties/fields that are capable of representing
the state we need.

I see two simple solutions:

* Two boolean properties: require-kernel-irqchip and
disable-kernel-irqchip. The default being both set to false (meaning
irqchip is enabled automatically if available). We may still have a
third kernel_irqchip property for compatibility, that will change both
require-kernel-irqchip and disable-kernel-irqchip at the same time when
set.

* A string kernel_irqchip property which accepts three values: "on",
"off", and "auto".

Example of partial implementation of the first approach, below. I still
didn't add the two extra properties, and just let the code access the
require_kernel_irqchip and disable_kernel_irqchip fields directly.

Note that this is on top of some other changes I have been experimenting
with, changing the accelerator init functions to get MachineState as
argument. Git tree containing all work in progress can be seen at:
https://github.com/ehabkost/qemu-hacks/commits/work/machine-irqchip-tristate

    diff --git a/hw/core/machine.c b/hw/core/machine.c
    index cbba679..0797bc1 100644
    --- a/hw/core/machine.c
    +++ b/hw/core/machine.c
    @@ -31,14 +31,15 @@ static bool machine_get_kernel_irqchip(Object *obj, Error **errp)
     {
         MachineState *ms = MACHINE(obj);
     
    -    return ms->kernel_irqchip;
    +    return !ms->disable_kernel_irqchip;
     }
     
     static void machine_set_kernel_irqchip(Object *obj, bool value, Error **errp)
     {
         MachineState *ms = MACHINE(obj);
     
    -    ms->kernel_irqchip = value;
    +    ms->require_kernel_irqchip = value;
    +    ms->disable_kernel_irqchip = !value;
     }
     
     static void machine_get_kvm_shadow_mem(Object *obj, Visitor *v,
    diff --git a/include/hw/boards.h b/include/hw/boards.h
    index 0389933..4a2daee 100644
    --- a/include/hw/boards.h
    +++ b/include/hw/boards.h
    @@ -99,7 +99,8 @@ struct MachineState {
         /*< public >*/
     
         char *accel;
    -    bool kernel_irqchip;
    +    bool require_kernel_irqchip;
    +    bool disable_kernel_irqchip;
         int kvm_shadow_mem;
         char *dtb;
         char *dumpdtb;
    diff --git a/kvm-all.c b/kvm-all.c
    index d2f4d7f..120bf70 100644
    --- a/kvm-all.c
    +++ b/kvm-all.c
    @@ -1315,11 +1315,11 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
                false);
     }
     
    -static int kvm_irqchip_create(KVMState *s)
    +static int kvm_irqchip_create(MachineState *ms, KVMState *s)
     {
         int ret;
     
    -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
    +    if (ms->disable_kernel_irqchip ||
             (!kvm_check_extension(s, KVM_CAP_IRQCHIP) &&
              (kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0) < 0))) {
             return 0;
    @@ -1545,7 +1545,7 @@ void kvm_init(MachineState *ms, Error **errp)
             goto err;
         }
     
    -    ret = kvm_irqchip_create(s);
    +    ret = kvm_irqchip_create(ms, s);
         if (ret < 0) {
             error_setg_errno(&err, -ret, "kvm_irqchip_create failed");
             goto err;

-- 
Eduardo

Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as properties to QemuMachineState

[Marcel Apfelbaum]

On Mon, 2014-06-02 at 12:21 -0300, Eduardo Habkost wrote:
> On Sun, Jun 01, 2014 at 11:21:49AM +0300, Marcel Apfelbaum wrote:
> > On Fri, 2014-05-30 at 16:25 -0300, Eduardo Habkost wrote:
> > > On Mon, May 26, 2014 at 03:40:58PM +0300, Marcel Apfelbaum wrote:
> > > [...]
> > > > +static void machine_initfn(Object *obj)
> > > > +{
> > > > +    object_property_add_str(obj, "accel",
> > > > +                            machine_get_accel, machine_set_accel, NULL);
> > > > +    object_property_add_bool(obj, "kernel_irqchip",
> > > > +                             machine_get_kernel_irqchip,
> > > > +                             machine_set_kernel_irqchip,
> > > > +                             NULL);
> > > 
> > > In the case of kernel_irqchip, the information contained in MachineState
> > > is not a superset of the information contained on
> > > qemu_get_machine_opts().
> > > 
> > > See hw/ppc/{e500,spapr}.c. They use kernel_irqchip like this:
> > > 
> > >     bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> > >                                             "kernel_irqchip", true);
> > >     bool irqchip_required = qemu_opt_get_bool(machine_opts,
> > >                                               "kernel_irqchip", false);
> > > 
> > >     if (irqchip_allowed) {
> > >         dev = ppce500_init_mpic_kvm(params, irqs);
> > >     }
> > > 
> > >     if (irqchip_required && !dev) {
> > >         fprintf(stderr, "%s: irqchip requested but unavailable\n",
> > >                 __func__);
> > >         abort();
> > >     }
> > > 
> > > This means kernel_irqchip have three possible states: "disabled", "required",
> > > and "allowed".
> > I already had a patch adding "property_is_set" to QMP, but was not accepted
> > as there is no way yet to "unset" a property. (I can point you to the series)
> > 
> > > 
> > > This means that MachineState.kernel_irqchip is not usable by current
> > > code that uses the kernel_irqchip option. I suppose we plan to address
> > > this on MachineState, too, to not get stuck with a global
> > > qemu_get_machine_opts() forever?
> > I completely agree with you and I already had a patch tackling it,
> > based on "property_is_set", but was no accepted yet, obviously.
> > I was instructed to set the default value in the machine init function
> > and some way (I don't remember now) to emulate  required/allowed.
> 
> I don't see a need to change to the object model and API. Just add
> MachineState-specific properties/fields that are capable of representing
> the state we need.
> 
> I see two simple solutions:
> 
> * Two boolean properties: require-kernel-irqchip and
> disable-kernel-irqchip. The default being both set to false (meaning
> irqchip is enabled automatically if available). We may still have a
> third kernel_irqchip property for compatibility, that will change both
> require-kernel-irqchip and disable-kernel-irqchip at the same time when
> set.
> 
> * A string kernel_irqchip property which accepts three values: "on",
> "off", and "auto".
> 
> Example of partial implementation of the first approach, below. I still
> didn't add the two extra properties, and just let the code access the
> require_kernel_irqchip and disable_kernel_irqchip fields directly.
> 
> Note that this is on top of some other changes I have been experimenting
> with, changing the accelerator init functions to get MachineState as
> argument. Git tree containing all work in progress can be seen at:
> https://github.com/ehabkost/qemu-hacks/commits/work/machine-irqchip-tristate

Hi Eduardo, thanks for the example.

I would also chose with the first solution, but use {require, allowed}
instead of {required, disabled}, but in the end is the same logic.

Thanks,
Marcel

> 
>     diff --git a/hw/core/machine.c b/hw/core/machine.c
>     index cbba679..0797bc1 100644
>     --- a/hw/core/machine.c
>     +++ b/hw/core/machine.c
>     @@ -31,14 +31,15 @@ static bool machine_get_kernel_irqchip(Object *obj, Error **errp)
>      {
>          MachineState *ms = MACHINE(obj);
>      
>     -    return ms->kernel_irqchip;
>     +    return !ms->disable_kernel_irqchip;
>      }
>      
>      static void machine_set_kernel_irqchip(Object *obj, bool value, Error **errp)
>      {
>          MachineState *ms = MACHINE(obj);
>      
>     -    ms->kernel_irqchip = value;
>     +    ms->require_kernel_irqchip = value;
>     +    ms->disable_kernel_irqchip = !value;
>      }
>      
>      static void machine_get_kvm_shadow_mem(Object *obj, Visitor *v,
>     diff --git a/include/hw/boards.h b/include/hw/boards.h
>     index 0389933..4a2daee 100644
>     --- a/include/hw/boards.h
>     +++ b/include/hw/boards.h
>     @@ -99,7 +99,8 @@ struct MachineState {
>          /*< public >*/
>      
>          char *accel;
>     -    bool kernel_irqchip;
>     +    bool require_kernel_irqchip;
>     +    bool disable_kernel_irqchip;
>          int kvm_shadow_mem;
>          char *dtb;
>          char *dumpdtb;
>     diff --git a/kvm-all.c b/kvm-all.c
>     index d2f4d7f..120bf70 100644
>     --- a/kvm-all.c
>     +++ b/kvm-all.c
>     @@ -1315,11 +1315,11 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
>                 false);
>      }
>      
>     -static int kvm_irqchip_create(KVMState *s)
>     +static int kvm_irqchip_create(MachineState *ms, KVMState *s)
>      {
>          int ret;
>      
>     -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
>     +    if (ms->disable_kernel_irqchip ||
>              (!kvm_check_extension(s, KVM_CAP_IRQCHIP) &&
>               (kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0) < 0))) {
>              return 0;
>     @@ -1545,7 +1545,7 @@ void kvm_init(MachineState *ms, Error **errp)
>              goto err;
>          }
>      
>     -    ret = kvm_irqchip_create(s);
>     +    ret = kvm_irqchip_create(ms, s);
>          if (ret < 0) {
>              error_setg_errno(&err, -ret, "kvm_irqchip_create failed");
>              goto err;
>