| Message ID | 20200227091031.102993-1-mimu@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: introduce module parameter kvm.use_gisa | expand |
On 27.02.20 10:10, Michael Mueller wrote: > The boolean module parameter "kvm.use_gisa" controls if newly > created guests will use the GISA facility if provided by the > host system. The default is yes. > > # cat /sys/module/kvm/parameters/use_gisa > Y > > The parameter can be changed on the fly. > > # echo N > /sys/module/kvm/parameters/use_gisa > > Already running guests are not affected by this change. > > The kvm s390 debug feature shows if a guest is running with GISA. > > # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf > 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized > 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 > ... > 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared > > In general, that value should not be changed as the GISA facility > enhances interruption delivery performance. > > A reason to switch the GISA facility off might be a performance > comparison run or debugging. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index d7ff30e45589..5c2081488024 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; > module_param(halt_poll_max_steal, byte, 0644); > MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); > > +/* if set to true, the GISA will be initialized and used if available */ > +static bool use_gisa = true; > +module_param(use_gisa, bool, 0644); > +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); > + > /* > * For now we handle at most 16 double words as this is what the s390 base > * kernel handles and stores in the prefix page. If we ever need to go beyond > @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.use_skf = sclp.has_skey; > spin_lock_init(&kvm->arch.start_stop_lock); > kvm_s390_vsie_init(kvm); > - kvm_s390_gisa_init(kvm); > + if (use_gisa) > + kvm_s390_gisa_init(kvm); Looks sane to me. gi->origin will remain NULL and act like css_general_characteristics.aiv wouldn't be around. I assume initializing the gib is fine, and having some guests use it and others not? I do wonder if it would be even clearer/cleaner to not allow to change this property on the fly, and to also not init the gib if disabled. If you want to perform performance tests, simply unload+reload KVM.
On Thu, 27 Feb 2020 10:10:31 +0100 Michael Mueller <mimu@linux.ibm.com> wrote: > The boolean module parameter "kvm.use_gisa" controls if newly > created guests will use the GISA facility if provided by the > host system. The default is yes. > > # cat /sys/module/kvm/parameters/use_gisa > Y > > The parameter can be changed on the fly. > > # echo N > /sys/module/kvm/parameters/use_gisa > > Already running guests are not affected by this change. > > The kvm s390 debug feature shows if a guest is running with GISA. > > # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf > 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized > 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 > ... > 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared Maybe log something as well if it is off due to this kernel parameter? > > In general, that value should not be changed as the GISA facility > enhances interruption delivery performance. > > A reason to switch the GISA facility off might be a performance > comparison run or debugging. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index d7ff30e45589..5c2081488024 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; > module_param(halt_poll_max_steal, byte, 0644); > MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); > > +/* if set to true, the GISA will be initialized and used if available */ > +static bool use_gisa = true; > +module_param(use_gisa, bool, 0644); > +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); I probably would have inverted the logic (i.e. introduce a disable_gisa parameter that is off by default), as you want KVM to use the gisa, except in special circumstances. > + > /* > * For now we handle at most 16 double words as this is what the s390 base > * kernel handles and stores in the prefix page. If we ever need to go beyond > @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.use_skf = sclp.has_skey; > spin_lock_init(&kvm->arch.start_stop_lock); > kvm_s390_vsie_init(kvm); > - kvm_s390_gisa_init(kvm); > + if (use_gisa) > + kvm_s390_gisa_init(kvm); I assume we're fine with no gisa but a gib (i.e. doesn't hurt?) > KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); > > return 0;
On 27.02.20 10:26, David Hildenbrand wrote: > On 27.02.20 10:10, Michael Mueller wrote: >> The boolean module parameter "kvm.use_gisa" controls if newly >> created guests will use the GISA facility if provided by the >> host system. The default is yes. >> >> # cat /sys/module/kvm/parameters/use_gisa >> Y >> >> The parameter can be changed on the fly. >> >> # echo N > /sys/module/kvm/parameters/use_gisa >> >> Already running guests are not affected by this change. >> >> The kvm s390 debug feature shows if a guest is running with GISA. >> >> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >> ... >> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >> >> In general, that value should not be changed as the GISA facility >> enhances interruption delivery performance. >> >> A reason to switch the GISA facility off might be a performance >> comparison run or debugging. >> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index d7ff30e45589..5c2081488024 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; >> module_param(halt_poll_max_steal, byte, 0644); >> MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); >> >> +/* if set to true, the GISA will be initialized and used if available */ >> +static bool use_gisa = true; >> +module_param(use_gisa, bool, 0644); >> +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); >> + >> /* >> * For now we handle at most 16 double words as this is what the s390 base >> * kernel handles and stores in the prefix page. If we ever need to go beyond >> @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >> kvm->arch.use_skf = sclp.has_skey; >> spin_lock_init(&kvm->arch.start_stop_lock); >> kvm_s390_vsie_init(kvm); >> - kvm_s390_gisa_init(kvm); >> + if (use_gisa) >> + kvm_s390_gisa_init(kvm); > > Looks sane to me. gi->origin will remain NULL and act like > css_general_characteristics.aiv wouldn't be around. right > > I assume initializing the gib is fine, and having some guests use it and > others not? Is fine as well. > > I do wonder if it would be even clearer/cleaner to not allow to change > this property on the fly, and to also not init the gib if disabled. > > If you want to perform performance tests, simply unload+reload KVM. That would work if kvm is build as module but not in-kernel, then a reboot would be required with kvm.use_gisa=Y/N I tend to leave it as it is. Thanks, Michael >
On 27.02.20 10:47, Cornelia Huck wrote: > On Thu, 27 Feb 2020 10:10:31 +0100 > Michael Mueller <mimu@linux.ibm.com> wrote: > >> The boolean module parameter "kvm.use_gisa" controls if newly >> created guests will use the GISA facility if provided by the >> host system. The default is yes. >> >> # cat /sys/module/kvm/parameters/use_gisa >> Y >> >> The parameter can be changed on the fly. >> >> # echo N > /sys/module/kvm/parameters/use_gisa >> >> Already running guests are not affected by this change. >> >> The kvm s390 debug feature shows if a guest is running with GISA. >> >> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >> ... >> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared > > Maybe log something as well if it is off due to this kernel parameter? There is also no message when the host does not support it. > >> >> In general, that value should not be changed as the GISA facility >> enhances interruption delivery performance. >> >> A reason to switch the GISA facility off might be a performance >> comparison run or debugging. >> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index d7ff30e45589..5c2081488024 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; >> module_param(halt_poll_max_steal, byte, 0644); >> MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); >> >> +/* if set to true, the GISA will be initialized and used if available */ >> +static bool use_gisa = true; >> +module_param(use_gisa, bool, 0644); >> +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); > > I probably would have inverted the logic (i.e. introduce a disable_gisa > parameter that is off by default), as you want KVM to use the gisa, > except in special circumstances. Hm, in my opinion "use it := no" is more explicit and natural than "don't use it := yes" > >> + >> /* >> * For now we handle at most 16 double words as this is what the s390 base >> * kernel handles and stores in the prefix page. If we ever need to go beyond >> @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >> kvm->arch.use_skf = sclp.has_skey; >> spin_lock_init(&kvm->arch.start_stop_lock); >> kvm_s390_vsie_init(kvm); >> - kvm_s390_gisa_init(kvm); >> + if (use_gisa) >> + kvm_s390_gisa_init(kvm); > > I assume we're fine with no gisa but a gib (i.e. doesn't hurt?) The GIB is a host entity and is required for guests with GISA that want to use AP with interruptions in guest. > >> KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); >> >> return 0; > Thanks a lot! Michael
On 27.02.20 10:10, Michael Mueller wrote: > The boolean module parameter "kvm.use_gisa" controls if newly > created guests will use the GISA facility if provided by the > host system. The default is yes. > > # cat /sys/module/kvm/parameters/use_gisa > Y > > The parameter can be changed on the fly. > > # echo N > /sys/module/kvm/parameters/use_gisa > > Already running guests are not affected by this change. > > The kvm s390 debug feature shows if a guest is running with GISA. > > # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf > 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized > 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 > ... > 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared > > In general, that value should not be changed as the GISA facility > enhances interruption delivery performance. > > A reason to switch the GISA facility off might be a performance > comparison run or debugging. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> Looks good to me. Regarding the other comments, I think allowing for dynamic changes and keeping use_gisa vs disable_gisa makes sense. So I would think that the patch as is makes sense. The only question is: shall we set use_gisa to 0 when the machine does not support it (e.g. VSIE?) and then also forbid setting it to 1? Could be overkill. > --- > arch/s390/kvm/kvm-s390.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index d7ff30e45589..5c2081488024 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; > module_param(halt_poll_max_steal, byte, 0644); > MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); > > +/* if set to true, the GISA will be initialized and used if available */ > +static bool use_gisa = true; > +module_param(use_gisa, bool, 0644); > +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); > + > /* > * For now we handle at most 16 double words as this is what the s390 base > * kernel handles and stores in the prefix page. If we ever need to go beyond > @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.use_skf = sclp.has_skey; > spin_lock_init(&kvm->arch.start_stop_lock); > kvm_s390_vsie_init(kvm); > - kvm_s390_gisa_init(kvm); > + if (use_gisa) > + kvm_s390_gisa_init(kvm); > KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); > > return 0; >
On 27.02.20 13:04, Michael Mueller wrote: > > > On 27.02.20 10:26, David Hildenbrand wrote: >> On 27.02.20 10:10, Michael Mueller wrote: >>> The boolean module parameter "kvm.use_gisa" controls if newly >>> created guests will use the GISA facility if provided by the >>> host system. The default is yes. >>> >>> # cat /sys/module/kvm/parameters/use_gisa >>> Y >>> >>> The parameter can be changed on the fly. >>> >>> # echo N > /sys/module/kvm/parameters/use_gisa >>> >>> Already running guests are not affected by this change. >>> >>> The kvm s390 debug feature shows if a guest is running with GISA. >>> >>> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >>> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >>> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >>> ... >>> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >>> >>> In general, that value should not be changed as the GISA facility >>> enhances interruption delivery performance. >>> >>> A reason to switch the GISA facility off might be a performance >>> comparison run or debugging. >>> >>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >>> --- >>> arch/s390/kvm/kvm-s390.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index d7ff30e45589..5c2081488024 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; >>> module_param(halt_poll_max_steal, byte, 0644); >>> MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); >>> >>> +/* if set to true, the GISA will be initialized and used if available */ >>> +static bool use_gisa = true; >>> +module_param(use_gisa, bool, 0644); >>> +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); >>> + >>> /* >>> * For now we handle at most 16 double words as this is what the s390 base >>> * kernel handles and stores in the prefix page. If we ever need to go beyond >>> @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >>> kvm->arch.use_skf = sclp.has_skey; >>> spin_lock_init(&kvm->arch.start_stop_lock); >>> kvm_s390_vsie_init(kvm); >>> - kvm_s390_gisa_init(kvm); >>> + if (use_gisa) >>> + kvm_s390_gisa_init(kvm); >> >> Looks sane to me. gi->origin will remain NULL and act like >> css_general_characteristics.aiv wouldn't be around. > > right > >> >> I assume initializing the gib is fine, and having some guests use it and >> others not? > > Is fine as well. > >> >> I do wonder if it would be even clearer/cleaner to not allow to change >> this property on the fly, and to also not init the gib if disabled. >> >> If you want to perform performance tests, simply unload+reload KVM. > > That would work if kvm is build as module but not in-kernel, then Right, but AFAIK that's not an usual setup (at least in distros) - and for testing purposes not an issue as well. Anyhow, I'm fine with this Reviewed-by: David Hildenbrand <david@redhat.com> Thanks!
On 27.02.20 13:27, Christian Borntraeger wrote: > > > On 27.02.20 10:10, Michael Mueller wrote: >> The boolean module parameter "kvm.use_gisa" controls if newly >> created guests will use the GISA facility if provided by the >> host system. The default is yes. >> >> # cat /sys/module/kvm/parameters/use_gisa >> Y >> >> The parameter can be changed on the fly. >> >> # echo N > /sys/module/kvm/parameters/use_gisa >> >> Already running guests are not affected by this change. >> >> The kvm s390 debug feature shows if a guest is running with GISA. >> >> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >> ... >> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >> >> In general, that value should not be changed as the GISA facility >> enhances interruption delivery performance. >> >> A reason to switch the GISA facility off might be a performance >> comparison run or debugging. >> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > > Looks good to me. Regarding the other comments, I think allowing for dynamic changes > and keeping use_gisa vs disable_gisa makes sense. So I would think that the patch > as is makes sense. > > The only question is: shall we set use_gisa to 0 when the machine does not support > it (e.g. VSIE?) and then also forbid setting it to 1? Could be overkill. Then I would rename the parameter to "try_to_use_gisa" instead. (a joke ;) ) In that case we exit gisa_init() because of the missing AIV facility. void kvm_s390_gisa_init(struct kvm *kvm) { struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int; --> if (!css_general_characteristics.aiv) return; gi->origin = &kvm->arch.sie_page2->gisa; gi->alert.mask = 0; ... } Thanks, Michael > > >> --- >> arch/s390/kvm/kvm-s390.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index d7ff30e45589..5c2081488024 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; >> module_param(halt_poll_max_steal, byte, 0644); >> MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); >> >> +/* if set to true, the GISA will be initialized and used if available */ >> +static bool use_gisa = true; >> +module_param(use_gisa, bool, 0644); >> +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); >> + >> /* >> * For now we handle at most 16 double words as this is what the s390 base >> * kernel handles and stores in the prefix page. If we ever need to go beyond >> @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >> kvm->arch.use_skf = sclp.has_skey; >> spin_lock_init(&kvm->arch.start_stop_lock); >> kvm_s390_vsie_init(kvm); >> - kvm_s390_gisa_init(kvm); >> + if (use_gisa) >> + kvm_s390_gisa_init(kvm); >> KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); >> >> return 0; >> >
On Thu, 27 Feb 2020 13:27:10 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 27.02.20 10:10, Michael Mueller wrote: > > The boolean module parameter "kvm.use_gisa" controls if newly > > created guests will use the GISA facility if provided by the > > host system. The default is yes. > > > > # cat /sys/module/kvm/parameters/use_gisa > > Y > > > > The parameter can be changed on the fly. > > > > # echo N > /sys/module/kvm/parameters/use_gisa > > > > Already running guests are not affected by this change. > > > > The kvm s390 debug feature shows if a guest is running with GISA. > > > > # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf > > 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized > > 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 > > ... > > 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared > > > > In general, that value should not be changed as the GISA facility > > enhances interruption delivery performance. > > > > A reason to switch the GISA facility off might be a performance > > comparison run or debugging. > > > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > > Looks good to me. Regarding the other comments, I think allowing for dynamic changes > and keeping use_gisa vs disable_gisa makes sense. So I would think that the patch > as is makes sense. use_gisa vs disable_gisa is more a personal preference; I don't mind keeping it as use_gisa. > > The only question is: shall we set use_gisa to 0 when the machine does not support > it (e.g. VSIE?) and then also forbid setting it to 1? Could be overkill. I don't think you should try to overload a debug knob like that; it's now simple enough, adding more code also adds to the potential for errors. > > > > --- > > arch/s390/kvm/kvm-s390.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > > index d7ff30e45589..5c2081488024 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; > > module_param(halt_poll_max_steal, byte, 0644); > > MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); > > > > +/* if set to true, the GISA will be initialized and used if available */ > > +static bool use_gisa = true; > > +module_param(use_gisa, bool, 0644); > > +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); Especially as the description explicitly says "if the host supports it" -- that's good enough for a new knob. > > + > > /* > > * For now we handle at most 16 double words as this is what the s390 base > > * kernel handles and stores in the prefix page. If we ever need to go beyond > > @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > kvm->arch.use_skf = sclp.has_skey; > > spin_lock_init(&kvm->arch.start_stop_lock); > > kvm_s390_vsie_init(kvm); > > - kvm_s390_gisa_init(kvm); > > + if (use_gisa) > > + kvm_s390_gisa_init(kvm); > > KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); > > > > return 0; > > > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 27.02.20 13:48, Cornelia Huck wrote: > On Thu, 27 Feb 2020 13:27:10 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 27.02.20 10:10, Michael Mueller wrote: >>> The boolean module parameter "kvm.use_gisa" controls if newly >>> created guests will use the GISA facility if provided by the >>> host system. The default is yes. >>> >>> # cat /sys/module/kvm/parameters/use_gisa >>> Y >>> >>> The parameter can be changed on the fly. >>> >>> # echo N > /sys/module/kvm/parameters/use_gisa >>> >>> Already running guests are not affected by this change. >>> >>> The kvm s390 debug feature shows if a guest is running with GISA. >>> >>> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >>> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >>> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >>> ... >>> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >>> >>> In general, that value should not be changed as the GISA facility >>> enhances interruption delivery performance. >>> >>> A reason to switch the GISA facility off might be a performance >>> comparison run or debugging. >>> >>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >> >> Looks good to me. Regarding the other comments, I think allowing for dynamic changes >> and keeping use_gisa vs disable_gisa makes sense. So I would think that the patch >> as is makes sense. > > use_gisa vs disable_gisa is more a personal preference; I don't mind > keeping it as use_gisa. > >> >> The only question is: shall we set use_gisa to 0 when the machine does not support >> it (e.g. VSIE?) and then also forbid setting it to 1? Could be overkill. > > I don't think you should try to overload a debug knob like that; it's > now simple enough, adding more code also adds to the potential for > errors. > >> >> >>> --- >>> arch/s390/kvm/kvm-s390.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index d7ff30e45589..5c2081488024 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; >>> module_param(halt_poll_max_steal, byte, 0644); >>> MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); >>> >>> +/* if set to true, the GISA will be initialized and used if available */ >>> +static bool use_gisa = true; >>> +module_param(use_gisa, bool, 0644); >>> +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); > > Especially as the description explicitly says "if the host supports it" > -- that's good enough for a new knob. > >>> + >>> /* >>> * For now we handle at most 16 double words as this is what the s390 base >>> * kernel handles and stores in the prefix page. If we ever need to go beyond >>> @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >>> kvm->arch.use_skf = sclp.has_skey; >>> spin_lock_init(&kvm->arch.start_stop_lock); >>> kvm_s390_vsie_init(kvm); >>> - kvm_s390_gisa_init(kvm); >>> + if (use_gisa) >>> + kvm_s390_gisa_init(kvm); >>> KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); >>> >>> return 0; >>> >> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Thanks!
On 27.02.20 13:31, David Hildenbrand wrote: > On 27.02.20 13:04, Michael Mueller wrote: >> >> >> On 27.02.20 10:26, David Hildenbrand wrote: >>> On 27.02.20 10:10, Michael Mueller wrote: >>>> The boolean module parameter "kvm.use_gisa" controls if newly >>>> created guests will use the GISA facility if provided by the >>>> host system. The default is yes. >>>> >>>> # cat /sys/module/kvm/parameters/use_gisa >>>> Y >>>> >>>> The parameter can be changed on the fly. >>>> >>>> # echo N > /sys/module/kvm/parameters/use_gisa >>>> >>>> Already running guests are not affected by this change. >>>> >>>> The kvm s390 debug feature shows if a guest is running with GISA. >>>> >>>> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >>>> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >>>> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >>>> ... >>>> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >>>> >>>> In general, that value should not be changed as the GISA facility >>>> enhances interruption delivery performance. >>>> >>>> A reason to switch the GISA facility off might be a performance >>>> comparison run or debugging. >>>> >>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >>>> --- >>>> arch/s390/kvm/kvm-s390.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index d7ff30e45589..5c2081488024 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; >>>> module_param(halt_poll_max_steal, byte, 0644); >>>> MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); >>>> >>>> +/* if set to true, the GISA will be initialized and used if available */ >>>> +static bool use_gisa = true; >>>> +module_param(use_gisa, bool, 0644); >>>> +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); >>>> + >>>> /* >>>> * For now we handle at most 16 double words as this is what the s390 base >>>> * kernel handles and stores in the prefix page. If we ever need to go beyond >>>> @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >>>> kvm->arch.use_skf = sclp.has_skey; >>>> spin_lock_init(&kvm->arch.start_stop_lock); >>>> kvm_s390_vsie_init(kvm); >>>> - kvm_s390_gisa_init(kvm); >>>> + if (use_gisa) >>>> + kvm_s390_gisa_init(kvm); >>> >>> Looks sane to me. gi->origin will remain NULL and act like >>> css_general_characteristics.aiv wouldn't be around. >> >> right >> >>> >>> I assume initializing the gib is fine, and having some guests use it and >>> others not? >> >> Is fine as well. >> >>> >>> I do wonder if it would be even clearer/cleaner to not allow to change >>> this property on the fly, and to also not init the gib if disabled. >>> >>> If you want to perform performance tests, simply unload+reload KVM. >> >> That would work if kvm is build as module but not in-kernel, then > > Right, but AFAIK that's not an usual setup (at least in distros) - and > for testing purposes not an issue as well. > > Anyhow, I'm fine with this > > Reviewed-by: David Hildenbrand <david@redhat.com> > Thank you! > Thanks! >
On 27.02.20 13:43, Michael Mueller wrote: > > > On 27.02.20 13:27, Christian Borntraeger wrote: >> >> >> On 27.02.20 10:10, Michael Mueller wrote: >>> The boolean module parameter "kvm.use_gisa" controls if newly >>> created guests will use the GISA facility if provided by the >>> host system. The default is yes. >>> >>> # cat /sys/module/kvm/parameters/use_gisa >>> Y >>> >>> The parameter can be changed on the fly. >>> >>> # echo N > /sys/module/kvm/parameters/use_gisa >>> >>> Already running guests are not affected by this change. >>> >>> The kvm s390 debug feature shows if a guest is running with GISA. >>> >>> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >>> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >>> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >>> ... >>> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >>> >>> In general, that value should not be changed as the GISA facility >>> enhances interruption delivery performance. >>> >>> A reason to switch the GISA facility off might be a performance >>> comparison run or debugging. >>> >>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >> >> Looks good to me. Regarding the other comments, I think allowing for dynamic changes >> and keeping use_gisa vs disable_gisa makes sense. So I would think that the patch >> as is makes sense. >> >> The only question is: shall we set use_gisa to 0 when the machine does not support >> it (e.g. VSIE?) and then also forbid setting it to 1? Could be overkill. > > Then I would rename the parameter to "try_to_use_gisa" instead. (a joke ;) ) > > In that case we exit gisa_init() because of the missing AIV facility. > > void kvm_s390_gisa_init(struct kvm *kvm) > { > struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int; > > --> if (!css_general_characteristics.aiv) > return; > gi->origin = &kvm->arch.sie_page2->gisa; > gi->alert.mask = 0; > ... > } > I know. My point was more: "can we expose this". But this is probably overkill.
On 27.02.20 14:24, Christian Borntraeger wrote: > > > On 27.02.20 13:43, Michael Mueller wrote: >> >> >> On 27.02.20 13:27, Christian Borntraeger wrote: >>> >>> >>> On 27.02.20 10:10, Michael Mueller wrote: >>>> The boolean module parameter "kvm.use_gisa" controls if newly >>>> created guests will use the GISA facility if provided by the >>>> host system. The default is yes. >>>> >>>> # cat /sys/module/kvm/parameters/use_gisa >>>> Y >>>> >>>> The parameter can be changed on the fly. >>>> >>>> # echo N > /sys/module/kvm/parameters/use_gisa >>>> >>>> Already running guests are not affected by this change. >>>> >>>> The kvm s390 debug feature shows if a guest is running with GISA. >>>> >>>> # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf >>>> 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized >>>> 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 >>>> ... >>>> 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared >>>> >>>> In general, that value should not be changed as the GISA facility >>>> enhances interruption delivery performance. >>>> >>>> A reason to switch the GISA facility off might be a performance >>>> comparison run or debugging. >>>> >>>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >>> >>> Looks good to me. Regarding the other comments, I think allowing for dynamic changes >>> and keeping use_gisa vs disable_gisa makes sense. So I would think that the patch >>> as is makes sense. >>> >>> The only question is: shall we set use_gisa to 0 when the machine does not support >>> it (e.g. VSIE?) and then also forbid setting it to 1? Could be overkill. >> >> Then I would rename the parameter to "try_to_use_gisa" instead. (a joke ;) ) >> >> In that case we exit gisa_init() because of the missing AIV facility. >> >> void kvm_s390_gisa_init(struct kvm *kvm) >> { >> struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int; >> >> --> if (!css_general_characteristics.aiv) >> return; >> gi->origin = &kvm->arch.sie_page2->gisa; >> gi->alert.mask = 0; >> ... >> } >> > > I know. My point was more: "can we expose this". But this is probably overkill. I agree with Connie here, that would make the whole thing just more error-prone. That way the messages are at least consistent. >
On 27.02.20 10:10, Michael Mueller wrote: > The boolean module parameter "kvm.use_gisa" controls if newly > created guests will use the GISA facility if provided by the > host system. The default is yes. > > # cat /sys/module/kvm/parameters/use_gisa > Y > > The parameter can be changed on the fly. > > # echo N > /sys/module/kvm/parameters/use_gisa > > Already running guests are not affected by this change. > > The kvm s390 debug feature shows if a guest is running with GISA. > > # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf > 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized > 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 > ... > 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared > > In general, that value should not be changed as the GISA facility > enhances interruption delivery performance. > > A reason to switch the GISA facility off might be a performance > comparison run or debugging. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> Thanks applied.
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index d7ff30e45589..5c2081488024 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -184,6 +184,11 @@ static u8 halt_poll_max_steal = 10; module_param(halt_poll_max_steal, byte, 0644); MODULE_PARM_DESC(halt_poll_max_steal, "Maximum percentage of steal time to allow polling"); +/* if set to true, the GISA will be initialized and used if available */ +static bool use_gisa = true; +module_param(use_gisa, bool, 0644); +MODULE_PARM_DESC(use_gisa, "Use the GISA if the host supports it."); + /* * For now we handle at most 16 double words as this is what the s390 base * kernel handles and stores in the prefix page. If we ever need to go beyond @@ -2504,7 +2509,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) kvm->arch.use_skf = sclp.has_skey; spin_lock_init(&kvm->arch.start_stop_lock); kvm_s390_vsie_init(kvm); - kvm_s390_gisa_init(kvm); + if (use_gisa) + kvm_s390_gisa_init(kvm); KVM_EVENT(3, "vm 0x%pK created by pid %u", kvm, current->pid); return 0;
The boolean module parameter "kvm.use_gisa" controls if newly created guests will use the GISA facility if provided by the host system. The default is yes. # cat /sys/module/kvm/parameters/use_gisa Y The parameter can be changed on the fly. # echo N > /sys/module/kvm/parameters/use_gisa Already running guests are not affected by this change. The kvm s390 debug feature shows if a guest is running with GISA. # grep gisa /sys/kernel/debug/s390dbf/kvm-$pid/sprintf 00 01582725059:843303 3 - 08 00000000e119bc01 gisa 0x00000000c9ac2642 initialized 00 01582725059:903840 3 - 11 000000004391ee22 00[0000000000000000-0000000000000000]: AIV gisa format-1 enabled for cpu 000 ... 00 01582725059:916847 3 - 08 0000000094fff572 gisa 0x00000000c9ac2642 cleared In general, that value should not be changed as the GISA facility enhances interruption delivery performance. A reason to switch the GISA facility off might be a performance comparison run or debugging. Signed-off-by: Michael Mueller <mimu@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)