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