* 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