* [PATCH v2] accel/kvm: Make "kernel_irqchip" default on
@ 2019-12-28 10:43 Xiaoyao Li
2019-12-30 12:01 ` Michal Prívozník
2020-01-07 10:32 ` Paolo Bonzini
0 siblings, 2 replies; 5+ messages in thread
From: Xiaoyao Li @ 2019-12-28 10:43 UTC (permalink / raw)
To: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum
Cc: Vitaly Kuznetsov, Xiaoyao Li, qemu-devel
Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
accelerator property") moves kernel_irqchip property from "-machine" to
"-accel kvm", but it forgets to set the default value of
kernel_irqchip_allowed and kernel_irqchip_split.
Also cleaning up the three useless members (kernel_irqchip_allowed,
kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")
Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Changes in v2:
- Add Reported-by tag;
- Initialize kernel_irqchip_split in init_machine();
---
accel/kvm/kvm-all.c | 3 +++
include/hw/boards.h | 3 ---
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b2f1a5bcb5ef..afbbe0a1af25 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2070,6 +2070,8 @@ static int kvm_init(MachineState *ms)
goto err;
}
+ s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
+
if (s->kernel_irqchip_allowed) {
kvm_irqchip_create(s);
}
@@ -3046,6 +3048,7 @@ static void kvm_accel_instance_init(Object *obj)
KVMState *s = KVM_STATE(obj);
s->kvm_shadow_mem = -1;
+ s->kernel_irqchip_allowed = true;
}
static void kvm_accel_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 61f8bb8e5a42..fb1b43d5b972 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -271,9 +271,6 @@ struct MachineState {
/*< public >*/
- bool kernel_irqchip_allowed;
- bool kernel_irqchip_required;
- bool kernel_irqchip_split;
char *dtb;
char *dumpdtb;
int phandle_start;
--
2.19.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] accel/kvm: Make "kernel_irqchip" default on
2019-12-28 10:43 [PATCH v2] accel/kvm: Make "kernel_irqchip" default on Xiaoyao Li
@ 2019-12-30 12:01 ` Michal Prívozník
2020-01-07 10:32 ` Paolo Bonzini
1 sibling, 0 replies; 5+ messages in thread
From: Michal Prívozník @ 2019-12-30 12:01 UTC (permalink / raw)
To: Xiaoyao Li, Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum
Cc: Vitaly Kuznetsov, qemu-devel
On 12/28/19 11:43 AM, Xiaoyao Li wrote:
> Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> accelerator property") moves kernel_irqchip property from "-machine" to
> "-accel kvm", but it forgets to set the default value of
> kernel_irqchip_allowed and kernel_irqchip_split.
>
> Also cleaning up the three useless members (kernel_irqchip_allowed,
> kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
>
> Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")
> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v2:
> - Add Reported-by tag;
> - Initialize kernel_irqchip_split in init_machine();
> ---
> accel/kvm/kvm-all.c | 3 +++
> include/hw/boards.h | 3 ---
> 2 files changed, 3 insertions(+), 3 deletions(-)
Huh, I've just converged to the same patch.
Tested-by: Michal Prívozník <mprivozn@redhat.com>
Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] accel/kvm: Make "kernel_irqchip" default on
2019-12-28 10:43 [PATCH v2] accel/kvm: Make "kernel_irqchip" default on Xiaoyao Li
2019-12-30 12:01 ` Michal Prívozník
@ 2020-01-07 10:32 ` Paolo Bonzini
2020-01-08 10:17 ` Cornelia Huck
2020-01-08 11:38 ` Xiaoyao Li
1 sibling, 2 replies; 5+ messages in thread
From: Paolo Bonzini @ 2020-01-07 10:32 UTC (permalink / raw)
To: Xiaoyao Li, Eduardo Habkost, Marcel Apfelbaum
Cc: Vitaly Kuznetsov, qemu-devel
On 28/12/19 11:43, Xiaoyao Li wrote:
> Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> accelerator property") moves kernel_irqchip property from "-machine" to
> "-accel kvm", but it forgets to set the default value of
> kernel_irqchip_allowed and kernel_irqchip_split.
>
> Also cleaning up the three useless members (kernel_irqchip_allowed,
> kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
>
> Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")
> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> Changes in v2:
> - Add Reported-by tag;
> - Initialize kernel_irqchip_split in init_machine();
Now that I am actually reviewing the patch on something other than a
phone, I think this would break "-machine kernel_irqchip=split". I'll
test, and squash if it works, something like this:
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index afbbe0a1af..ea35433170 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -98,7 +98,7 @@ struct KVMState
int kvm_shadow_mem;
bool kernel_irqchip_allowed;
bool kernel_irqchip_required;
- bool kernel_irqchip_split;
+ OnOffAuto kernel_irqchip_split;
bool sync_mmu;
bool manual_dirty_log_protect;
/* The man page (and posix) say ioctl numbers are signed int, but
@@ -1799,7 +1799,7 @@ static void kvm_irqchip_create(KVMState *s)
* in-kernel irqchip for us */
ret = kvm_arch_irqchip_create(s);
if (ret == 0) {
- if (s->kernel_irqchip_split) {
+ if (s->kernel_irqchip_split == ON_OFF_AUTO_ON) {
perror("Split IRQ chip mode not supported.");
exit(1);
} else {
@@ -2070,7 +2070,9 @@ static int kvm_init(MachineState *ms)
goto err;
}
- s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
+ if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
+ s->kernel_irqchip_split = mc->default_kernel_irqchip_split ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+ }
if (s->kernel_irqchip_allowed) {
kvm_irqchip_create(s);
@@ -3007,17 +3009,17 @@ static void kvm_set_kernel_irqchip(Object *obj, Visitor *v,
case ON_OFF_SPLIT_ON:
s->kernel_irqchip_allowed = true;
s->kernel_irqchip_required = true;
- s->kernel_irqchip_split = false;
+ s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
break;
case ON_OFF_SPLIT_OFF:
s->kernel_irqchip_allowed = false;
s->kernel_irqchip_required = false;
- s->kernel_irqchip_split = false;
+ s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
break;
case ON_OFF_SPLIT_SPLIT:
s->kernel_irqchip_allowed = true;
s->kernel_irqchip_required = true;
- s->kernel_irqchip_split = true;
+ s->kernel_irqchip_split = ON_OFF_AUTO_ON;
break;
default:
/* The value was checked in visit_type_OnOffSplit() above. If
@@ -3040,7 +3042,7 @@ bool kvm_kernel_irqchip_required(void)
bool kvm_kernel_irqchip_split(void)
{
- return kvm_state->kernel_irqchip_split;
+ return kvm_state->kernel_irqchip_split == ON_OFF_AUTO_ON;
}
static void kvm_accel_instance_init(Object *obj)
@@ -3049,6 +3051,7 @@ static void kvm_accel_instance_init(Object *obj)
s->kvm_shadow_mem = -1;
s->kernel_irqchip_allowed = true;
+ s->kernel_irqchip_split = ON_OFF_AUTO_AUTO;
}
static void kvm_accel_class_init(ObjectClass *oc, void *data)
As a follow up, kernel_irqchip_allowed and kernel_irqchip_required could also be changed to a single OnOffAuto field, I think.
Paolo
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] accel/kvm: Make "kernel_irqchip" default on
2020-01-07 10:32 ` Paolo Bonzini
@ 2020-01-08 10:17 ` Cornelia Huck
2020-01-08 11:38 ` Xiaoyao Li
1 sibling, 0 replies; 5+ messages in thread
From: Cornelia Huck @ 2020-01-08 10:17 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Eduardo Habkost, Xiaoyao Li, qemu-devel, qemu-s390x, Vitaly Kuznetsov
On Tue, 7 Jan 2020 11:32:42 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 28/12/19 11:43, Xiaoyao Li wrote:
> > Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> > accelerator property") moves kernel_irqchip property from "-machine" to
> > "-accel kvm", but it forgets to set the default value of
> > kernel_irqchip_allowed and kernel_irqchip_split.
> >
> > Also cleaning up the three useless members (kernel_irqchip_allowed,
> > kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
FWIW, this also fixes s390x, which requires kernel_irqchip=on.
> >
> > Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an accelerator property")
> > Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> > Changes in v2:
> > - Add Reported-by tag;
> > - Initialize kernel_irqchip_split in init_machine();
>
> Now that I am actually reviewing the patch on something other than a
> phone, I think this would break "-machine kernel_irqchip=split". I'll
> test, and squash if it works, something like this:
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index afbbe0a1af..ea35433170 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -98,7 +98,7 @@ struct KVMState
> int kvm_shadow_mem;
> bool kernel_irqchip_allowed;
> bool kernel_irqchip_required;
> - bool kernel_irqchip_split;
> + OnOffAuto kernel_irqchip_split;
> bool sync_mmu;
> bool manual_dirty_log_protect;
> /* The man page (and posix) say ioctl numbers are signed int, but
> @@ -1799,7 +1799,7 @@ static void kvm_irqchip_create(KVMState *s)
> * in-kernel irqchip for us */
> ret = kvm_arch_irqchip_create(s);
> if (ret == 0) {
> - if (s->kernel_irqchip_split) {
> + if (s->kernel_irqchip_split == ON_OFF_AUTO_ON) {
> perror("Split IRQ chip mode not supported.");
> exit(1);
> } else {
> @@ -2070,7 +2070,9 @@ static int kvm_init(MachineState *ms)
> goto err;
> }
>
> - s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
> + if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
> + s->kernel_irqchip_split = mc->default_kernel_irqchip_split ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> + }
>
> if (s->kernel_irqchip_allowed) {
> kvm_irqchip_create(s);
> @@ -3007,17 +3009,17 @@ static void kvm_set_kernel_irqchip(Object *obj, Visitor *v,
> case ON_OFF_SPLIT_ON:
> s->kernel_irqchip_allowed = true;
> s->kernel_irqchip_required = true;
> - s->kernel_irqchip_split = false;
> + s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
> break;
> case ON_OFF_SPLIT_OFF:
> s->kernel_irqchip_allowed = false;
> s->kernel_irqchip_required = false;
> - s->kernel_irqchip_split = false;
> + s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
> break;
> case ON_OFF_SPLIT_SPLIT:
> s->kernel_irqchip_allowed = true;
> s->kernel_irqchip_required = true;
> - s->kernel_irqchip_split = true;
> + s->kernel_irqchip_split = ON_OFF_AUTO_ON;
> break;
> default:
> /* The value was checked in visit_type_OnOffSplit() above. If
> @@ -3040,7 +3042,7 @@ bool kvm_kernel_irqchip_required(void)
>
> bool kvm_kernel_irqchip_split(void)
> {
> - return kvm_state->kernel_irqchip_split;
> + return kvm_state->kernel_irqchip_split == ON_OFF_AUTO_ON;
> }
>
> static void kvm_accel_instance_init(Object *obj)
> @@ -3049,6 +3051,7 @@ static void kvm_accel_instance_init(Object *obj)
>
> s->kvm_shadow_mem = -1;
> s->kernel_irqchip_allowed = true;
> + s->kernel_irqchip_split = ON_OFF_AUTO_AUTO;
> }
>
> static void kvm_accel_class_init(ObjectClass *oc, void *data)
These changes looks sane to me (but did not test).
>
> As a follow up, kernel_irqchip_allowed and kernel_irqchip_required could also be changed to a single OnOffAuto field, I think.
That would probably be a nice cleanup.
>
> Paolo
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] accel/kvm: Make "kernel_irqchip" default on
2020-01-07 10:32 ` Paolo Bonzini
2020-01-08 10:17 ` Cornelia Huck
@ 2020-01-08 11:38 ` Xiaoyao Li
1 sibling, 0 replies; 5+ messages in thread
From: Xiaoyao Li @ 2020-01-08 11:38 UTC (permalink / raw)
To: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum
Cc: Vitaly Kuznetsov, qemu-devel
On Tue, 2020-01-07 at 11:32 +0100, Paolo Bonzini wrote:
> On 28/12/19 11:43, Xiaoyao Li wrote:
> > Commit 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> > accelerator property") moves kernel_irqchip property from "-machine" to
> > "-accel kvm", but it forgets to set the default value of
> > kernel_irqchip_allowed and kernel_irqchip_split.
> >
> > Also cleaning up the three useless members (kernel_irqchip_allowed,
> > kernel_irqchip_required, kernel_irqchip_split) in struct MachineState.
> >
> > Fixes: 11bc4a13d1f4 ("kvm: convert "-machine kernel_irqchip" to an
> > accelerator property")
> > Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> > Changes in v2:
> > - Add Reported-by tag;
> > - Initialize kernel_irqchip_split in init_machine();
>
> Now that I am actually reviewing the patch on something other than a
> phone, I think this would break "-machine kernel_irqchip=split". I'll
> test, and squash if it works, something like this:
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index afbbe0a1af..ea35433170 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -98,7 +98,7 @@ struct KVMState
> int kvm_shadow_mem;
> bool kernel_irqchip_allowed;
> bool kernel_irqchip_required;
> - bool kernel_irqchip_split;
> + OnOffAuto kernel_irqchip_split;
> bool sync_mmu;
> bool manual_dirty_log_protect;
> /* The man page (and posix) say ioctl numbers are signed int, but
> @@ -1799,7 +1799,7 @@ static void kvm_irqchip_create(KVMState *s)
> * in-kernel irqchip for us */
> ret = kvm_arch_irqchip_create(s);
> if (ret == 0) {
> - if (s->kernel_irqchip_split) {
> + if (s->kernel_irqchip_split == ON_OFF_AUTO_ON) {
> perror("Split IRQ chip mode not supported.");
> exit(1);
> } else {
> @@ -2070,7 +2070,9 @@ static int kvm_init(MachineState *ms)
> goto err;
> }
>
> - s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
> + if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
> + s->kernel_irqchip_split = mc->default_kernel_irqchip_split ?
> ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> + }
>
> if (s->kernel_irqchip_allowed) {
> kvm_irqchip_create(s);
> @@ -3007,17 +3009,17 @@ static void kvm_set_kernel_irqchip(Object *obj,
> Visitor *v,
> case ON_OFF_SPLIT_ON:
> s->kernel_irqchip_allowed = true;
> s->kernel_irqchip_required = true;
> - s->kernel_irqchip_split = false;
> + s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
> break;
> case ON_OFF_SPLIT_OFF:
> s->kernel_irqchip_allowed = false;
> s->kernel_irqchip_required = false;
> - s->kernel_irqchip_split = false;
> + s->kernel_irqchip_split = ON_OFF_AUTO_OFF;
> break;
> case ON_OFF_SPLIT_SPLIT:
> s->kernel_irqchip_allowed = true;
> s->kernel_irqchip_required = true;
> - s->kernel_irqchip_split = true;
> + s->kernel_irqchip_split = ON_OFF_AUTO_ON;
> break;
> default:
> /* The value was checked in visit_type_OnOffSplit() above. If
> @@ -3040,7 +3042,7 @@ bool kvm_kernel_irqchip_required(void)
>
> bool kvm_kernel_irqchip_split(void)
> {
> - return kvm_state->kernel_irqchip_split;
> + return kvm_state->kernel_irqchip_split == ON_OFF_AUTO_ON;
> }
>
> static void kvm_accel_instance_init(Object *obj)
> @@ -3049,6 +3051,7 @@ static void kvm_accel_instance_init(Object *obj)
>
> s->kvm_shadow_mem = -1;
> s->kernel_irqchip_allowed = true;
> + s->kernel_irqchip_split = ON_OFF_AUTO_AUTO;
> }
>
> static void kvm_accel_class_init(ObjectClass *oc, void *data)
thanks, the above patch does fix the issue that unconditionally setting
s->kernel_irqchip_split = mc->default_kernel_irqchip_split;
in kvm_init() may change the value setting by "kernel_irqchip=split"
> As a follow up, kernel_irqchip_allowed and kernel_irqchip_required could also
> be changed to a single OnOffAuto field, I think.
In fact, kernel_irqchip_allowed and kernel_irqchip_required don't have the issue
of kernel_irqchip_split. Anyway, making them the same style looks good.
> Paolo
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-08 11:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-28 10:43 [PATCH v2] accel/kvm: Make "kernel_irqchip" default on Xiaoyao Li
2019-12-30 12:01 ` Michal Prívozník
2020-01-07 10:32 ` Paolo Bonzini
2020-01-08 10:17 ` Cornelia Huck
2020-01-08 11:38 ` Xiaoyao Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.