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