[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as propert
From: |
Marcel Apfelbaum |
Subject: |
Re: [Qemu-devel] [PATCH V2 4/4] hw/machine: qemu machine opts as properties to QemuMachineState |
Date: |
Thu, 05 Jun 2014 14:49:30 +0300 |
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;
>