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
