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

[Marcel Apfelbaum <marcel.a@redhat.com>]

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;
>