| Message ID | 20181224115235.16881-2-wainersm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qom-get access to kernel-irqchip property | expand |
On Mon, Dec 24, 2018 at 06:52:35AM -0500, Wainer dos Santos Moschetta wrote: > Allows to access the kernel-irqchip property of a Machine > Class object via QOM get method. > > Before this patch the property cannot be read although it is > listed by qom-list: > > qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split (...) > -> {"execute": "qom-list", "arguments": {"path": "/machine"}} > <- {"return": [{"name": "type", "type": "string"}, (...) , {"name": "kernel-irqchip", "type": "on|off|split"} (...)} > -> {"execute": "qom-get", "arguments": {"path": "/machine", "property": "kernel-irqchip"}} > <- {"error": {"class": "GenericError", "desc": "Insufficient permission to perform this operation"}} > > It is implemented the machine_get_kernel_irqchip() method, > which evaluates the kernel_irqchip_allowed, *_required, and > *_split fields to determine the value of kernel-irqchip. Note: we > assume there is not inconsistency on the value of those fields. > > Then with this change in place, it works as expected: > > qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split (...) > -> {"execute": "qom-get", "arguments": {"path": "/machine", "property": "kernel-irqchip"}} > <- {"return": "split"} > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > --- > hw/core/machine.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index c51423b647..f61003f8f2 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -37,6 +37,21 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) > ms->accel = g_strdup(value); > } > > +static void machine_get_kernel_irqchip(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + MachineState *ms = MACHINE(obj); > + OnOffSplit mode; > + > + if (ms->kernel_irqchip_split) { > + mode = ON_OFF_SPLIT_SPLIT; > + } else { > + mode = (ms->kernel_irqchip_allowed && ms->kernel_irqchip_required) ? > + ON_OFF_SPLIT_ON : ON_OFF_SPLIT_OFF; Hi, Wainer, The new interface seems to be a good thing, though the implementation might be incorrect here. AFAIU these parameters only decide "how we want to choose the kernel-irqchip parameter" rather than the real result, which could depend on more (e.g., whether KVM is used). IMHO you should simply fetch the results from kvm_irqchip_in_kernel() and kvm_irqchip_is_split() where the real results are stored. Regards,
On Tue, Dec 25, 2018 at 01:20:05PM +0800, Peter Xu wrote: > On Mon, Dec 24, 2018 at 06:52:35AM -0500, Wainer dos Santos Moschetta wrote: > > Allows to access the kernel-irqchip property of a Machine > > Class object via QOM get method. > > > > Before this patch the property cannot be read although it is > > listed by qom-list: > > > > qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split (...) > > -> {"execute": "qom-list", "arguments": {"path": "/machine"}} > > <- {"return": [{"name": "type", "type": "string"}, (...) , {"name": "kernel-irqchip", "type": "on|off|split"} (...)} > > -> {"execute": "qom-get", "arguments": {"path": "/machine", "property": "kernel-irqchip"}} > > <- {"error": {"class": "GenericError", "desc": "Insufficient permission to perform this operation"}} > > > > It is implemented the machine_get_kernel_irqchip() method, > > which evaluates the kernel_irqchip_allowed, *_required, and > > *_split fields to determine the value of kernel-irqchip. Note: we > > assume there is not inconsistency on the value of those fields. > > > > Then with this change in place, it works as expected: > > > > qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split (...) > > -> {"execute": "qom-get", "arguments": {"path": "/machine", "property": "kernel-irqchip"}} > > <- {"return": "split"} > > > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > > --- > > hw/core/machine.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index c51423b647..f61003f8f2 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -37,6 +37,21 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) > > ms->accel = g_strdup(value); > > } > > > > +static void machine_get_kernel_irqchip(Object *obj, Visitor *v, > > + const char *name, void *opaque, > > + Error **errp) > > +{ > > + MachineState *ms = MACHINE(obj); > > + OnOffSplit mode; > > + > > + if (ms->kernel_irqchip_split) { > > + mode = ON_OFF_SPLIT_SPLIT; > > + } else { > > + mode = (ms->kernel_irqchip_allowed && ms->kernel_irqchip_required) ? > > + ON_OFF_SPLIT_ON : ON_OFF_SPLIT_OFF; > > Hi, Wainer, > > The new interface seems to be a good thing, though the implementation > might be incorrect here. AFAIU these parameters only decide "how we > want to choose the kernel-irqchip parameter" rather than the real > result, which could depend on more (e.g., whether KVM is used). > > IMHO you should simply fetch the results from kvm_irqchip_in_kernel() > and kvm_irqchip_is_split() where the real results are stored. Agreed. Knowing the actual values of kvm_irqchip_*() would be even more useful for test code (this way we would be testing all the KVM irqchip initialization code, not just a few lines of property getter/seter code).
diff --git a/hw/core/machine.c b/hw/core/machine.c index c51423b647..f61003f8f2 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -37,6 +37,21 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) ms->accel = g_strdup(value); } +static void machine_get_kernel_irqchip(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + MachineState *ms = MACHINE(obj); + OnOffSplit mode; + + if (ms->kernel_irqchip_split) { + mode = ON_OFF_SPLIT_SPLIT; + } else { + mode = (ms->kernel_irqchip_allowed && ms->kernel_irqchip_required) ? + ON_OFF_SPLIT_ON : ON_OFF_SPLIT_OFF; + } + visit_type_OnOffSplit(v, name, &mode, errp); +} static void machine_set_kernel_irqchip(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) @@ -540,7 +555,7 @@ static void machine_class_init(ObjectClass *oc, void *data) "Accelerator list", &error_abort); object_class_property_add(oc, "kernel-irqchip", "on|off|split", - NULL, machine_set_kernel_irqchip, + machine_get_kernel_irqchip, machine_set_kernel_irqchip, NULL, NULL, &error_abort); object_class_property_set_description(oc, "kernel-irqchip", "Configure KVM in-kernel irqchip", &error_abort);
Allows to access the kernel-irqchip property of a Machine Class object via QOM get method. Before this patch the property cannot be read although it is listed by qom-list: qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split (...) -> {"execute": "qom-list", "arguments": {"path": "/machine"}} <- {"return": [{"name": "type", "type": "string"}, (...) , {"name": "kernel-irqchip", "type": "on|off|split"} (...)} -> {"execute": "qom-get", "arguments": {"path": "/machine", "property": "kernel-irqchip"}} <- {"error": {"class": "GenericError", "desc": "Insufficient permission to perform this operation"}} It is implemented the machine_get_kernel_irqchip() method, which evaluates the kernel_irqchip_allowed, *_required, and *_split fields to determine the value of kernel-irqchip. Note: we assume there is not inconsistency on the value of those fields. Then with this change in place, it works as expected: qemu-system-x86_64 -M q35,accel=kvm,kernel-irqchip=split (...) -> {"execute": "qom-get", "arguments": {"path": "/machine", "property": "kernel-irqchip"}} <- {"return": "split"} Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> --- hw/core/machine.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)