* [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
@ 2020-01-20 13:24 Thomas Huth
2020-01-20 16:23 ` Matthew Rosato
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Thomas Huth @ 2020-01-20 13:24 UTC (permalink / raw)
To: Cornelia Huck, Christian Borntraeger, qemu-devel, Matthew Rosato
Cc: Halil Pasic, qemu-s390x, David Hildenbrand
The AIS feature has been disabled late in the v2.10 development cycle since
there were some issues with migration (see commit 3f2d07b3b01ea61126b -
"s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
to enable it again for newer machine types, but apparently we forgot to do
this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
While at it, also add a more verbose comment why we need the *_allowed()
wrappers in s390-virtio-ccw.c.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
include/hw/s390x/s390-virtio-ccw.h | 3 +++
target/s390x/kvm.c | 9 ++++++---
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e0e28139a2..76254e8447 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -452,6 +452,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
s390mc->cpu_model_allowed = true;
s390mc->css_migration_enabled = true;
s390mc->hpage_1m_allowed = true;
+ s390mc->kvm_ais_allowed = true;
mc->init = ccw_init;
mc->reset = s390_machine_reset;
mc->hot_add_cpu = s390_hot_add_cpu;
@@ -505,6 +506,14 @@ static inline void machine_set_dea_key_wrap(Object *obj, bool value,
static S390CcwMachineClass *current_mc;
+/*
+ * Get the class of the s390-ccw-virtio machine that is currently in use.
+ * Note: libvirt is using the "none" machine to probe for the features of the
+ * host CPU, so in case this is called with the "none" machine, the function
+ * returns the TYPE_S390_CCW_MACHINE base class. In this base class, all the
+ * various "*_allowed" variables are enabled, so that the *_allowed() wrappers
+ * below return the correct default value for the "none" machine.
+ */
static S390CcwMachineClass *get_machine_class(void)
{
if (unlikely(!current_mc)) {
@@ -521,22 +530,24 @@ static S390CcwMachineClass *get_machine_class(void)
bool ri_allowed(void)
{
- /* for "none" machine this results in true */
return get_machine_class()->ri_allowed;
}
bool cpu_model_allowed(void)
{
- /* for "none" machine this results in true */
return get_machine_class()->cpu_model_allowed;
}
bool hpage_1m_allowed(void)
{
- /* for "none" machine this results in true */
return get_machine_class()->hpage_1m_allowed;
}
+bool kvm_ais_allowed(void)
+{
+ return get_machine_class()->kvm_ais_allowed;
+}
+
static char *machine_get_loadparm(Object *obj, Error **errp)
{
S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
@@ -658,8 +669,11 @@ static void ccw_machine_4_2_instance_options(MachineState *machine)
static void ccw_machine_4_2_class_options(MachineClass *mc)
{
+ S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
+
ccw_machine_5_0_class_options(mc);
compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+ s390mc->kvm_ais_allowed = false;
}
DEFINE_CCW_MACHINE(4_2, "4.2", false);
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8aa27199c9..e3ba3b88b1 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -40,6 +40,7 @@ typedef struct S390CcwMachineClass {
bool cpu_model_allowed;
bool css_migration_enabled;
bool hpage_1m_allowed;
+ bool kvm_ais_allowed;
} S390CcwMachineClass;
/* runtime-instrumentation allowed by the machine */
@@ -48,6 +49,8 @@ bool ri_allowed(void);
bool cpu_model_allowed(void);
/* 1M huge page mappings allowed by the machine */
bool hpage_1m_allowed(void);
+/* adapter-interrupt suppression allowed by the machine? */
+bool kvm_ais_allowed(void);
/**
* Returns true if (vmstate based) migration of the channel subsystem
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 15260aeb9a..cf4fb4f2d9 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
/*
* The migration interface for ais was introduced with kernel 4.13
* but the capability itself had been active since 4.12. As migration
- * support is considered necessary let's disable ais in the 2.10
- * machine.
+ * support is considered necessary, we only try to enable this for
+ * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
*/
- /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
+ if (kvm_ais_allowed() &&
+ kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
+ kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
+ }
kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
return 0;
--
2.18.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-20 13:24 [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again Thomas Huth
@ 2020-01-20 16:23 ` Matthew Rosato
2020-01-20 16:29 ` Cornelia Huck
2020-01-20 16:24 ` Cornelia Huck
2020-01-20 17:27 ` Cornelia Huck
2 siblings, 1 reply; 12+ messages in thread
From: Matthew Rosato @ 2020-01-20 16:23 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck, Christian Borntraeger, qemu-devel
Cc: Halil Pasic, qemu-s390x, David Hildenbrand
On 1/20/20 8:24 AM, Thomas Huth wrote:
> The AIS feature has been disabled late in the v2.10 development cycle since
> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> to enable it again for newer machine types, but apparently we forgot to do
> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>
> While at it, also add a more verbose comment why we need the *_allowed()
> wrappers in s390-virtio-ccw.c.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
Took it for a spin with vfio-pci. With this patch applied, I see the
appropriate change reflected in guest /proc/cpuinfo. I did some tracing
and see the expected behavior changes (ex: hits in host
kvm_s390_injrect_airq that show suppression occurring). Data transfer
tests worked fine. Also sanity-tested that ais=off behaves as expected.
Looks good to me.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-20 16:23 ` Matthew Rosato
@ 2020-01-20 16:29 ` Cornelia Huck
2020-01-20 16:32 ` Matthew Rosato
0 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2020-01-20 16:29 UTC (permalink / raw)
To: Matthew Rosato
Cc: Thomas Huth, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
On Mon, 20 Jan 2020 11:23:37 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> On 1/20/20 8:24 AM, Thomas Huth wrote:
> > The AIS feature has been disabled late in the v2.10 development cycle since
> > there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> > "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> > to enable it again for newer machine types, but apparently we forgot to do
> > this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
> >
> > While at it, also add a more verbose comment why we need the *_allowed()
> > wrappers in s390-virtio-ccw.c.
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
>
> Took it for a spin with vfio-pci. With this patch applied, I see the
> appropriate change reflected in guest /proc/cpuinfo. I did some tracing
> and see the expected behavior changes (ex: hits in host
> kvm_s390_injrect_airq that show suppression occurring). Data transfer
> tests worked fine. Also sanity-tested that ais=off behaves as expected.
>
> Looks good to me.
>
Excellent, thanks for testing!
Should I add a Tested-by: ?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-20 16:29 ` Cornelia Huck
@ 2020-01-20 16:32 ` Matthew Rosato
0 siblings, 0 replies; 12+ messages in thread
From: Matthew Rosato @ 2020-01-20 16:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: Thomas Huth, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
On 1/20/20 11:29 AM, Cornelia Huck wrote:
> On Mon, 20 Jan 2020 11:23:37 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
>> On 1/20/20 8:24 AM, Thomas Huth wrote:
>>> The AIS feature has been disabled late in the v2.10 development cycle since
>>> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
>>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
>>> to enable it again for newer machine types, but apparently we forgot to do
>>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>>>
>>> While at it, also add a more verbose comment why we need the *_allowed()
>>> wrappers in s390-virtio-ccw.c.
>>>
>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>
>> Took it for a spin with vfio-pci. With this patch applied, I see the
>> appropriate change reflected in guest /proc/cpuinfo. I did some tracing
>> and see the expected behavior changes (ex: hits in host
>> kvm_s390_injrect_airq that show suppression occurring). Data transfer
>> tests worked fine. Also sanity-tested that ais=off behaves as expected.
>>
>> Looks good to me.
>>
>
> Excellent, thanks for testing!
>
> Should I add a Tested-by: ?
>
Sure, go ahead.
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-20 13:24 [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again Thomas Huth
2020-01-20 16:23 ` Matthew Rosato
@ 2020-01-20 16:24 ` Cornelia Huck
2020-01-20 17:27 ` Cornelia Huck
2 siblings, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2020-01-20 16:24 UTC (permalink / raw)
To: Thomas Huth
Cc: Matthew Rosato, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
On Mon, 20 Jan 2020 14:24:41 +0100
Thomas Huth <thuth@redhat.com> wrote:
> The AIS feature has been disabled late in the v2.10 development cycle since
> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> to enable it again for newer machine types, but apparently we forgot to do
> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>
> While at it, also add a more verbose comment why we need the *_allowed()
> wrappers in s390-virtio-ccw.c.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
We also might want to move the others in a followup patch, just to
avoid bad examples to copy/paste.
>
> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
> include/hw/s390x/s390-virtio-ccw.h | 3 +++
> target/s390x/kvm.c | 9 ++++++---
> 3 files changed, 26 insertions(+), 6 deletions(-)
Looks good to me, will queue when I get a positive test result.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-20 13:24 [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again Thomas Huth
2020-01-20 16:23 ` Matthew Rosato
2020-01-20 16:24 ` Cornelia Huck
@ 2020-01-20 17:27 ` Cornelia Huck
2020-01-21 14:33 ` Matthew Rosato
2 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2020-01-20 17:27 UTC (permalink / raw)
To: Thomas Huth
Cc: Matthew Rosato, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
On Mon, 20 Jan 2020 14:24:41 +0100
Thomas Huth <thuth@redhat.com> wrote:
> The AIS feature has been disabled late in the v2.10 development cycle since
> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> to enable it again for newer machine types, but apparently we forgot to do
> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>
> While at it, also add a more verbose comment why we need the *_allowed()
> wrappers in s390-virtio-ccw.c.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
>
> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
> include/hw/s390x/s390-virtio-ccw.h | 3 +++
> target/s390x/kvm.c | 9 ++++++---
> 3 files changed, 26 insertions(+), 6 deletions(-)
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 15260aeb9a..cf4fb4f2d9 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> /*
> * The migration interface for ais was introduced with kernel 4.13
> * but the capability itself had been active since 4.12. As migration
> - * support is considered necessary let's disable ais in the 2.10
> - * machine.
> + * support is considered necessary, we only try to enable this for
> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
> */
> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> + if (kvm_ais_allowed() &&
> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
work; else we'll fail with
qemu-system-s390x: Failed to inject airq with AIS supported
in the kernel_irqchip=off case, as we won't have an I/O adapter
registered.
Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
comments?
> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> + }
>
> kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
> return 0;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-20 17:27 ` Cornelia Huck
@ 2020-01-21 14:33 ` Matthew Rosato
2020-01-21 14:46 ` Cornelia Huck
0 siblings, 1 reply; 12+ messages in thread
From: Matthew Rosato @ 2020-01-21 14:33 UTC (permalink / raw)
To: Cornelia Huck, Thomas Huth
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel,
David Hildenbrand
On 1/20/20 12:27 PM, Cornelia Huck wrote:
> On Mon, 20 Jan 2020 14:24:41 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
>> The AIS feature has been disabled late in the v2.10 development cycle since
>> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
>> to enable it again for newer machine types, but apparently we forgot to do
>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>>
>> While at it, also add a more verbose comment why we need the *_allowed()
>> wrappers in s390-virtio-ccw.c.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
>>
>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
>> include/hw/s390x/s390-virtio-ccw.h | 3 +++
>> target/s390x/kvm.c | 9 ++++++---
>> 3 files changed, 26 insertions(+), 6 deletions(-)
>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 15260aeb9a..cf4fb4f2d9 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>> /*
>> * The migration interface for ais was introduced with kernel 4.13
>> * but the capability itself had been active since 4.12. As migration
>> - * support is considered necessary let's disable ais in the 2.10
>> - * machine.
>> + * support is considered necessary, we only try to enable this for
>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
>> */
>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>> + if (kvm_ais_allowed() &&
>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>
> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
> work; else we'll fail with
>
> qemu-system-s390x: Failed to inject airq with AIS supported
>
> in the kernel_irqchip=off case, as we won't have an I/O adapter
> registered.
>
> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
> comments?
>
In spirit, I agree with this idea. But, a quick test shows that putting
this check here results in ais=off for the 'none' machine case (libvirt
capabilities detection). I think we have to only look at
kvm_kernel_irqchip_required() when working with a real machine.
>> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>> + }
>>
>> kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
>> return 0;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-21 14:33 ` Matthew Rosato
@ 2020-01-21 14:46 ` Cornelia Huck
2020-01-21 15:22 ` Thomas Huth
0 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2020-01-21 14:46 UTC (permalink / raw)
To: Matthew Rosato
Cc: Thomas Huth, David Hildenbrand, qemu-devel, Halil Pasic,
Christian Borntraeger, qemu-s390x
On Tue, 21 Jan 2020 09:33:02 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> On 1/20/20 12:27 PM, Cornelia Huck wrote:
> > On Mon, 20 Jan 2020 14:24:41 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >
> >> The AIS feature has been disabled late in the v2.10 development cycle since
> >> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> >> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> >> to enable it again for newer machine types, but apparently we forgot to do
> >> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
> >>
> >> While at it, also add a more verbose comment why we need the *_allowed()
> >> wrappers in s390-virtio-ccw.c.
> >>
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> >> Reviewed-by: David Hildenbrand <david@redhat.com>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
> >>
> >> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
> >> include/hw/s390x/s390-virtio-ccw.h | 3 +++
> >> target/s390x/kvm.c | 9 ++++++---
> >> 3 files changed, 26 insertions(+), 6 deletions(-)
> >
> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index 15260aeb9a..cf4fb4f2d9 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >> /*
> >> * The migration interface for ais was introduced with kernel 4.13
> >> * but the capability itself had been active since 4.12. As migration
> >> - * support is considered necessary let's disable ais in the 2.10
> >> - * machine.
> >> + * support is considered necessary, we only try to enable this for
> >> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
> >> */
> >> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> >> + if (kvm_ais_allowed() &&
> >> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
> >
> > Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
> > work; else we'll fail with
> >
> > qemu-system-s390x: Failed to inject airq with AIS supported
> >
> > in the kernel_irqchip=off case, as we won't have an I/O adapter
> > registered.
> >
> > Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
> > comments?
> >
>
> In spirit, I agree with this idea. But, a quick test shows that putting
> this check here results in ais=off for the 'none' machine case (libvirt
> capabilities detection). I think we have to only look at
> kvm_kernel_irqchip_required() when working with a real machine.
Sigh, I think you're right again. We need to check for the 'none'
machine here; but I can't think of a non-ugly way to do so...
>
> >> + kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> >> + }
> >>
> >> kvm_set_max_memslot_size(KVM_SLOT_MAX_BYTES);
> >> return 0;
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-21 14:46 ` Cornelia Huck
@ 2020-01-21 15:22 ` Thomas Huth
2020-01-21 16:05 ` Matthew Rosato
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Huth @ 2020-01-21 15:22 UTC (permalink / raw)
To: Cornelia Huck, Matthew Rosato
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel,
David Hildenbrand
On 21/01/2020 15.46, Cornelia Huck wrote:
> On Tue, 21 Jan 2020 09:33:02 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
>> On 1/20/20 12:27 PM, Cornelia Huck wrote:
>>> On Mon, 20 Jan 2020 14:24:41 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> The AIS feature has been disabled late in the v2.10 development cycle since
>>>> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
>>>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
>>>> to enable it again for newer machine types, but apparently we forgot to do
>>>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>>>>
>>>> While at it, also add a more verbose comment why we need the *_allowed()
>>>> wrappers in s390-virtio-ccw.c.
>>>>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
>>>>
>>>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
>>>> include/hw/s390x/s390-virtio-ccw.h | 3 +++
>>>> target/s390x/kvm.c | 9 ++++++---
>>>> 3 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index 15260aeb9a..cf4fb4f2d9 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>> /*
>>>> * The migration interface for ais was introduced with kernel 4.13
>>>> * but the capability itself had been active since 4.12. As migration
>>>> - * support is considered necessary let's disable ais in the 2.10
>>>> - * machine.
>>>> + * support is considered necessary, we only try to enable this for
>>>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
>>>> */
>>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>>> + if (kvm_ais_allowed() &&
>>>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>>>
>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
>>> work; else we'll fail with
>>>
>>> qemu-system-s390x: Failed to inject airq with AIS supported
>>>
>>> in the kernel_irqchip=off case, as we won't have an I/O adapter
>>> registered.
>>>
>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
>>> comments?
>>>
>>
>> In spirit, I agree with this idea. But, a quick test shows that putting
>> this check here results in ais=off for the 'none' machine case (libvirt
>> capabilities detection). I think we have to only look at
>> kvm_kernel_irqchip_required() when working with a real machine.
>
> Sigh, I think you're right again. We need to check for the 'none'
> machine here; but I can't think of a non-ugly way to do so...
I think it might work when using kvm_kernel_irqchip_allowed() instead of
kvm_kernel_irqchip_required() ... Matthew, could you please give it a
try with this patch on top of mine:
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -368,7 +368,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
* support is considered necessary, we only try to enable this for
* newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
*/
- if (kvm_ais_allowed() &&
+ if (kvm_ais_allowed() && kvm_kernel_irqchip_allowed() &&
kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
}
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-21 15:22 ` Thomas Huth
@ 2020-01-21 16:05 ` Matthew Rosato
2020-01-21 16:11 ` Thomas Huth
2020-01-21 16:12 ` Cornelia Huck
0 siblings, 2 replies; 12+ messages in thread
From: Matthew Rosato @ 2020-01-21 16:05 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel,
David Hildenbrand
On 1/21/20 10:22 AM, Thomas Huth wrote:
> On 21/01/2020 15.46, Cornelia Huck wrote:
>> On Tue, 21 Jan 2020 09:33:02 -0500
>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>
>>> On 1/20/20 12:27 PM, Cornelia Huck wrote:
>>>> On Mon, 20 Jan 2020 14:24:41 +0100
>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>>> The AIS feature has been disabled late in the v2.10 development cycle since
>>>>> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
>>>>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
>>>>> to enable it again for newer machine types, but apparently we forgot to do
>>>>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>>>>>
>>>>> While at it, also add a more verbose comment why we need the *_allowed()
>>>>> wrappers in s390-virtio-ccw.c.
>>>>>
>>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
>>>>>
>>>>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
>>>>> include/hw/s390x/s390-virtio-ccw.h | 3 +++
>>>>> target/s390x/kvm.c | 9 ++++++---
>>>>> 3 files changed, 26 insertions(+), 6 deletions(-)
>>>>
>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>> index 15260aeb9a..cf4fb4f2d9 100644
>>>>> --- a/target/s390x/kvm.c
>>>>> +++ b/target/s390x/kvm.c
>>>>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>>> /*
>>>>> * The migration interface for ais was introduced with kernel 4.13
>>>>> * but the capability itself had been active since 4.12. As migration
>>>>> - * support is considered necessary let's disable ais in the 2.10
>>>>> - * machine.
>>>>> + * support is considered necessary, we only try to enable this for
>>>>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
>>>>> */
>>>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>>>> + if (kvm_ais_allowed() &&
>>>>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>>>>
>>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
>>>> work; else we'll fail with
>>>>
>>>> qemu-system-s390x: Failed to inject airq with AIS supported
>>>>
>>>> in the kernel_irqchip=off case, as we won't have an I/O adapter
>>>> registered.
>>>>
>>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
>>>> comments?
>>>>
>>>
>>> In spirit, I agree with this idea. But, a quick test shows that putting
>>> this check here results in ais=off for the 'none' machine case (libvirt
>>> capabilities detection). I think we have to only look at
>>> kvm_kernel_irqchip_required() when working with a real machine.
>>
>> Sigh, I think you're right again. We need to check for the 'none'
>> machine here; but I can't think of a non-ugly way to do so...
>
> I think it might work when using kvm_kernel_irqchip_allowed() instead of
> kvm_kernel_irqchip_required() ... Matthew, could you please give it a
> try with this patch on top of mine:
>
Sure.
Libvirt detection works with this patch.
Alternatively, if I run qemu with kernel_irqchip=off and ais=true, I get:
qemu-system-s390x: Some features requested in the CPU model are not
available in the configuration: ais
Which was the same result as Connie's proposal.
It reads a bit odd to me at first, but looking at the code quick I think
this is the right answer - kvm_kernel_irqchip_allowed() will only return
false when kernel_irqchip has been forced off as above, whereas
kernel_irqchip_required will also return false in the case where no
setting was specified (this is what tripped libvirt up).
Looks good to me, thanks Thomas.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-21 16:05 ` Matthew Rosato
@ 2020-01-21 16:11 ` Thomas Huth
2020-01-21 16:12 ` Cornelia Huck
1 sibling, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2020-01-21 16:11 UTC (permalink / raw)
To: Matthew Rosato, Cornelia Huck
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel,
David Hildenbrand
On 21/01/2020 17.05, Matthew Rosato wrote:
> On 1/21/20 10:22 AM, Thomas Huth wrote:
>> On 21/01/2020 15.46, Cornelia Huck wrote:
>>> On Tue, 21 Jan 2020 09:33:02 -0500
>>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>>
>>>> On 1/20/20 12:27 PM, Cornelia Huck wrote:
>>>>> On Mon, 20 Jan 2020 14:24:41 +0100
>>>>> Thomas Huth <thuth@redhat.com> wrote:
>>>>>
>>>>>> The AIS feature has been disabled late in the v2.10 development
>>>>>> cycle since
>>>>>> there were some issues with migration (see commit
>>>>>> 3f2d07b3b01ea61126b -
>>>>>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally
>>>>>> wanted
>>>>>> to enable it again for newer machine types, but apparently we
>>>>>> forgot to do
>>>>>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
>>>>>>
>>>>>> While at it, also add a more verbose comment why we need the
>>>>>> *_allowed()
>>>>>> wrappers in s390-virtio-ccw.c.
>>>>>>
>>>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the
>>>>>> function
>>>>>>
>>>>>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
>>>>>> include/hw/s390x/s390-virtio-ccw.h | 3 +++
>>>>>> target/s390x/kvm.c | 9 ++++++---
>>>>>> 3 files changed, 26 insertions(+), 6 deletions(-)
>>>>>
>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>>> index 15260aeb9a..cf4fb4f2d9 100644
>>>>>> --- a/target/s390x/kvm.c
>>>>>> +++ b/target/s390x/kvm.c
>>>>>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState
>>>>>> *s)
>>>>>> /*
>>>>>> * The migration interface for ais was introduced with
>>>>>> kernel 4.13
>>>>>> * but the capability itself had been active since 4.12. As
>>>>>> migration
>>>>>> - * support is considered necessary let's disable ais in the 2.10
>>>>>> - * machine.
>>>>>> + * support is considered necessary, we only try to enable
>>>>>> this for
>>>>>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is
>>>>>> available.
>>>>>> */
>>>>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>>>>> + if (kvm_ais_allowed() &&
>>>>>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>>>>>
>>>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
>>>>> work; else we'll fail with
>>>>>
>>>>> qemu-system-s390x: Failed to inject airq with AIS supported
>>>>>
>>>>> in the kernel_irqchip=off case, as we won't have an I/O adapter
>>>>> registered.
>>>>>
>>>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
>>>>> comments?
>>>>>
>>>>
>>>> In spirit, I agree with this idea. But, a quick test shows that
>>>> putting
>>>> this check here results in ais=off for the 'none' machine case (libvirt
>>>> capabilities detection). I think we have to only look at
>>>> kvm_kernel_irqchip_required() when working with a real machine.
>>>
>>> Sigh, I think you're right again. We need to check for the 'none'
>>> machine here; but I can't think of a non-ugly way to do so...
>>
>> I think it might work when using kvm_kernel_irqchip_allowed() instead of
>> kvm_kernel_irqchip_required() ... Matthew, could you please give it a
>> try with this patch on top of mine:
>>
>
> Sure.
>
> Libvirt detection works with this patch.
>
> Alternatively, if I run qemu with kernel_irqchip=off and ais=true, I get:
> qemu-system-s390x: Some features requested in the CPU model are not
> available in the configuration: ais
>
> Which was the same result as Connie's proposal.
>
> It reads a bit odd to me at first, but looking at the code quick I think
> this is the right answer - kvm_kernel_irqchip_allowed() will only return
> false when kernel_irqchip has been forced off as above, whereas
> kernel_irqchip_required will also return false in the case where no
> setting was specified (this is what tripped libvirt up).
>
> Looks good to me, thanks Thomas.
Great, thanks for testing!
Cornelia, could you squash that kvm_kernel_irqchip_allowed() into the
patch, or do you prefer if I send a v4 ?
Thomas
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again
2020-01-21 16:05 ` Matthew Rosato
2020-01-21 16:11 ` Thomas Huth
@ 2020-01-21 16:12 ` Cornelia Huck
1 sibling, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2020-01-21 16:12 UTC (permalink / raw)
To: Matthew Rosato, Thomas Huth
Cc: Halil Pasic, Christian Borntraeger, qemu-s390x, qemu-devel,
David Hildenbrand
On Tue, 21 Jan 2020 11:05:18 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> On 1/21/20 10:22 AM, Thomas Huth wrote:
> > On 21/01/2020 15.46, Cornelia Huck wrote:
> >> On Tue, 21 Jan 2020 09:33:02 -0500
> >> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> >>
> >>> On 1/20/20 12:27 PM, Cornelia Huck wrote:
> >>>> On Mon, 20 Jan 2020 14:24:41 +0100
> >>>> Thomas Huth <thuth@redhat.com> wrote:
> >>>>
> >>>>> The AIS feature has been disabled late in the v2.10 development cycle since
> >>>>> there were some issues with migration (see commit 3f2d07b3b01ea61126b -
> >>>>> "s390x/ais: for 2.10 stable: disable ais facility"). We originally wanted
> >>>>> to enable it again for newer machine types, but apparently we forgot to do
> >>>>> this so far. Let's do it for the new s390-ccw-virtio-5.0 machine now.
> >>>>>
> >>>>> While at it, also add a more verbose comment why we need the *_allowed()
> >>>>> wrappers in s390-virtio-ccw.c.
> >>>>>
> >>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> >>>>> Reviewed-by: David Hildenbrand <david@redhat.com>
> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>>> ---
> >>>>> v3: Moved "s390mc->kvm_ais_allowed = false" to the end of the function
> >>>>>
> >>>>> hw/s390x/s390-virtio-ccw.c | 20 +++++++++++++++++---
> >>>>> include/hw/s390x/s390-virtio-ccw.h | 3 +++
> >>>>> target/s390x/kvm.c | 9 ++++++---
> >>>>> 3 files changed, 26 insertions(+), 6 deletions(-)
> >>>>
> >>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>>> index 15260aeb9a..cf4fb4f2d9 100644
> >>>>> --- a/target/s390x/kvm.c
> >>>>> +++ b/target/s390x/kvm.c
> >>>>> @@ -365,10 +365,13 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>>>> /*
> >>>>> * The migration interface for ais was introduced with kernel 4.13
> >>>>> * but the capability itself had been active since 4.12. As migration
> >>>>> - * support is considered necessary let's disable ais in the 2.10
> >>>>> - * machine.
> >>>>> + * support is considered necessary, we only try to enable this for
> >>>>> + * newer machine types if KVM_CAP_S390_AIS_MIGRATION is available.
> >>>>> */
> >>>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> >>>>> + if (kvm_ais_allowed() &&
> >>>>> + kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
> >>>>
> >>>> Hnm, we actually need a kernel irqchip with the kvm flic to get ais to
> >>>> work; else we'll fail with
> >>>>
> >>>> qemu-system-s390x: Failed to inject airq with AIS supported
> >>>>
> >>>> in the kernel_irqchip=off case, as we won't have an I/O adapter
> >>>> registered.
> >>>>
> >>>> Adding 'kvm_kernel_irqchip_required() &&' seems to do the trick;
> >>>> comments?
> >>>>
> >>>
> >>> In spirit, I agree with this idea. But, a quick test shows that putting
> >>> this check here results in ais=off for the 'none' machine case (libvirt
> >>> capabilities detection). I think we have to only look at
> >>> kvm_kernel_irqchip_required() when working with a real machine.
> >>
> >> Sigh, I think you're right again. We need to check for the 'none'
> >> machine here; but I can't think of a non-ugly way to do so...
> >
> > I think it might work when using kvm_kernel_irqchip_allowed() instead of
> > kvm_kernel_irqchip_required() ... Matthew, could you please give it a
> > try with this patch on top of mine:
> >
>
> Sure.
>
> Libvirt detection works with this patch.
Excellent.
>
> Alternatively, if I run qemu with kernel_irqchip=off and ais=true, I get:
> qemu-system-s390x: Some features requested in the CPU model are not
> available in the configuration: ais
>
> Which was the same result as Connie's proposal.
Yep, that's the expected behaviour.
>
> It reads a bit odd to me at first, but looking at the code quick I think
> this is the right answer - kvm_kernel_irqchip_allowed() will only return
> false when kernel_irqchip has been forced off as above, whereas
> kernel_irqchip_required will also return false in the case where no
> setting was specified (this is what tripped libvirt up).
>
> Looks good to me, thanks Thomas.
Thanks for testing!
Thomas, I guess you'll send a v4?
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-01-21 16:14 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 13:24 [PATCH v3] target/s390x/kvm: Enable adapter interruption suppression again Thomas Huth
2020-01-20 16:23 ` Matthew Rosato
2020-01-20 16:29 ` Cornelia Huck
2020-01-20 16:32 ` Matthew Rosato
2020-01-20 16:24 ` Cornelia Huck
2020-01-20 17:27 ` Cornelia Huck
2020-01-21 14:33 ` Matthew Rosato
2020-01-21 14:46 ` Cornelia Huck
2020-01-21 15:22 ` Thomas Huth
2020-01-21 16:05 ` Matthew Rosato
2020-01-21 16:11 ` Thomas Huth
2020-01-21 16:12 ` Cornelia Huck
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.