hw/s390x/s390-virtio-ccw.c | 4 ++++ include/hw/s390x/s390-virtio-ccw.h | 4 ++++ target/s390x/kvm.c | 11 ++++++++--- 3 files changed, 16 insertions(+), 3 deletions(-)
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.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/s390x/s390-virtio-ccw.c | 4 ++++
include/hw/s390x/s390-virtio-ccw.h | 4 ++++
target/s390x/kvm.c | 11 ++++++++---
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e7eadd14e8..6f43136396 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -456,6 +456,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;
@@ -662,6 +663,9 @@ 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);
+
+ s390mc->kvm_ais_allowed = false;
ccw_machine_5_0_class_options(mc);
compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
}
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8aa27199c9..f142d379c6 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -21,6 +21,9 @@
#define S390_MACHINE_CLASS(klass) \
OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
+#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE)
+
typedef struct S390CcwMachineState {
/*< private >*/
MachineState parent_obj;
@@ -40,6 +43,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 */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 15260aeb9a..4c1c8c0208 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
int kvm_arch_init(MachineState *ms, KVMState *s)
{
+ S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
+
object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
false, NULL);
@@ -365,10 +367,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 enable this for newer
+ * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
*/
- /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
+ if (smc->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
On Thu, 16 Jan 2020 13:20:26 +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.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 ++++
> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
> target/s390x/kvm.c | 11 ++++++++---
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> @@ -365,10 +367,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 enable this for newer
s/necessary we only enable this/necessary, we only try to enable this/
> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
maybe s/and if/if/
> */
> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> + if (smc->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;
Looks good.
Remind me again: ais only made a difference for pci devices, right? Is
it enough to give this a quick whirl with virtio-pci devices?
On 16/01/2020 13.50, Cornelia Huck wrote:
> On Thu, 16 Jan 2020 13:20:26 +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.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>> target/s390x/kvm.c | 11 ++++++++---
>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>
>
>> @@ -365,10 +367,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 enable this for newer
>
> s/necessary we only enable this/necessary, we only try to enable this/
>
>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
>
> maybe s/and if/if/
Sure ... could you fix it up when picking up the patch (in case I don't
have to respin), or do you want me to send a v2?
>> */
>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>> + if (smc->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;
>
> Looks good.
>
> Remind me again: ais only made a difference for pci devices, right? Is
> it enough to give this a quick whirl with virtio-pci devices?
I don't remember the details, Christian, could you please answer this
question?
Thomas
On 16.01.20 13:55, Thomas Huth wrote:
> On 16/01/2020 13.50, Cornelia Huck wrote:
>> On Thu, 16 Jan 2020 13:20:26 +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.
>>>
>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>> target/s390x/kvm.c | 11 ++++++++---
>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>
>>> @@ -365,10 +367,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 enable this for newer
>>
>> s/necessary we only enable this/necessary, we only try to enable this/
>>
>>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
>>
>> maybe s/and if/if/
>
> Sure ... could you fix it up when picking up the patch (in case I don't
> have to respin), or do you want me to send a v2?
>
>>> */
>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>> + if (smc->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;
>>
>> Looks good.
>>
>> Remind me again: ais only made a difference for pci devices, right? Is
>> it enough to give this a quick whirl with virtio-pci devices?
>
> I don't remember the details, Christian, could you please answer this
> question?
Yes, IIRC AIS was there for PCI, but not for Crypto or virtio.
The patch looks sane, but it would be good if someone could try
the AIS stuff.
Matt, can you have a look?
On Thu, 16 Jan 2020 14:22:15 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 16.01.20 13:55, Thomas Huth wrote: > > On 16/01/2020 13.50, Cornelia Huck wrote: > >> On Thu, 16 Jan 2020 13:20:26 +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. > >>> > >>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946 > >>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>> --- > >>> hw/s390x/s390-virtio-ccw.c | 4 ++++ > >>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++ > >>> target/s390x/kvm.c | 11 ++++++++--- > >>> 3 files changed, 16 insertions(+), 3 deletions(-) > >>> > >> > >>> @@ -365,10 +367,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 enable this for newer > >> > >> s/necessary we only enable this/necessary, we only try to enable this/ > >> > >>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available. > >> > >> maybe s/and if/if/ > > > > Sure ... could you fix it up when picking up the patch (in case I don't > > have to respin), or do you want me to send a v2? Sure, can do that.
On 1/16/20 8:22 AM, Christian Borntraeger wrote:
>
>
> On 16.01.20 13:55, Thomas Huth wrote:
>> On 16/01/2020 13.50, Cornelia Huck wrote:
>>> On Thu, 16 Jan 2020 13:20:26 +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.
>>>>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>>> target/s390x/kvm.c | 11 ++++++++---
>>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>
>>>> @@ -365,10 +367,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 enable this for newer
>>>
>>> s/necessary we only enable this/necessary, we only try to enable this/
>>>
>>>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
>>>
>>> maybe s/and if/if/
>>
>> Sure ... could you fix it up when picking up the patch (in case I don't
>> have to respin), or do you want me to send a v2?
>>
>>>> */
>>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>>> + if (smc->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;
>>>
>>> Looks good.
>>>
>>> Remind me again: ais only made a difference for pci devices, right? Is
>>> it enough to give this a quick whirl with virtio-pci devices?
>>
>> I don't remember the details, Christian, could you please answer this
>> question?
>
> Yes, IIRC AIS was there for PCI, but not for Crypto or virtio.
This matches my understanding as well.
> The patch looks sane, but it would be good if someone could try
> the AIS stuff.
>
> Matt, can you have a look?
Sure. But my PCI environment is currently down for a maintenance
window, will try again later today.
On 1/16/20 7:20 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.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 ++++
> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
> target/s390x/kvm.c | 11 ++++++++---
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e7eadd14e8..6f43136396 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -456,6 +456,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;
> @@ -662,6 +663,9 @@ 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);
> +
> + s390mc->kvm_ais_allowed = false;
> ccw_machine_5_0_class_options(mc);
> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> }
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 8aa27199c9..f142d379c6 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -21,6 +21,9 @@
> #define S390_MACHINE_CLASS(klass) \
> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>
> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE)
> +
> typedef struct S390CcwMachineState {
> /*< private >*/
> MachineState parent_obj;
> @@ -40,6 +43,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 */
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 15260aeb9a..4c1c8c0208 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>
> int kvm_arch_init(MachineState *ms, KVMState *s)
> {
> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
> +
I still can't run a proper test due to unavailable hw but in the
meantime I tried to virsh define a libvirt guest pointed at qemu (master
+ this patch). Regardless of machine type (s390-ccw-virtio-5.0 or
s390-ccw-virtio-4.2) I get:
virsh define guest.xml
error: Failed to define domain from /path/to/guest.xml
error: invalid argument: could not find capabilities for arch=s390x
domaintype=kvm
Similarly:
virsh domcapabilities
error: failed to get emulator capabilities
error: invalid argument: unable to find any emulator to serve 's390x'
architecture
Rolling back to qemu master, the define and domcapabilities work (with
no ais of course).
So: there is some incompatibility between the way libvirt invokes qemu
to detect capabilities and this code. The above line seems to be the
root problem - if I take your patch and remove 'smc' then libvirt works
as expected and I can see ais in the domcapabilities.
Looking at those wrappers David mentioned... I suspect you need this
for the 'none' machine case. I tried a quick hack with the following:
bool ais_allowed(void)
{
/* for "none" machine this results in true */
return get_machine_class()->kvm_ais_allowed;
}
and
if (ais_allowed() &&
kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
}
This works and doesn't break libvirt compatibility detection.
> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
> false, NULL);
>
> @@ -365,10 +367,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 enable this for newer
> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
> */
> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> + if (smc->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;
>
On Thu, 16 Jan 2020 15:19:13 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> On 1/16/20 7:20 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.
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> > hw/s390x/s390-virtio-ccw.c | 4 ++++
> > include/hw/s390x/s390-virtio-ccw.h | 4 ++++
> > target/s390x/kvm.c | 11 ++++++++---
> > 3 files changed, 16 insertions(+), 3 deletions(-)
(...)
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index 15260aeb9a..4c1c8c0208 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
> >
> > int kvm_arch_init(MachineState *ms, KVMState *s)
> > {
> > + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
> > +
>
> I still can't run a proper test due to unavailable hw but in the
> meantime I tried to virsh define a libvirt guest pointed at qemu (master
> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or
> s390-ccw-virtio-4.2) I get:
>
> virsh define guest.xml
> error: Failed to define domain from /path/to/guest.xml
> error: invalid argument: could not find capabilities for arch=s390x
> domaintype=kvm
>
> Similarly:
>
> virsh domcapabilities
> error: failed to get emulator capabilities
> error: invalid argument: unable to find any emulator to serve 's390x'
> architecture
>
> Rolling back to qemu master, the define and domcapabilities work (with
> no ais of course).
>
> So: there is some incompatibility between the way libvirt invokes qemu
> to detect capabilities and this code. The above line seems to be the
> root problem - if I take your patch and remove 'smc' then libvirt works
> as expected and I can see ais in the domcapabilities.
>
> Looking at those wrappers David mentioned... I suspect you need this
> for the 'none' machine case. I tried a quick hack with the following:
>
> bool ais_allowed(void)
> {
> /* for "none" machine this results in true */
> return get_machine_class()->kvm_ais_allowed;
> }
>
> and
>
> if (ais_allowed() &&
> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> }
>
> This works and doesn't break libvirt compatibility detection.
Oh, "none" machine fun again... I think you're on the right track, and
we really need a wrapper.
On 16/01/2020 21.26, Cornelia Huck wrote:
> On Thu, 16 Jan 2020 15:19:13 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>
>> On 1/16/20 7:20 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.
>>>
>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>> target/s390x/kvm.c | 11 ++++++++---
>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> (...)
>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 15260aeb9a..4c1c8c0208 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>>
>>> int kvm_arch_init(MachineState *ms, KVMState *s)
>>> {
>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
>>> +
>>
>> I still can't run a proper test due to unavailable hw but in the
>> meantime I tried to virsh define a libvirt guest pointed at qemu (master
>> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or
>> s390-ccw-virtio-4.2) I get:
>>
>> virsh define guest.xml
>> error: Failed to define domain from /path/to/guest.xml
>> error: invalid argument: could not find capabilities for arch=s390x
>> domaintype=kvm
>>
>> Similarly:
>>
>> virsh domcapabilities
>> error: failed to get emulator capabilities
>> error: invalid argument: unable to find any emulator to serve 's390x'
>> architecture
>>
>> Rolling back to qemu master, the define and domcapabilities work (with
>> no ais of course).
>>
>> So: there is some incompatibility between the way libvirt invokes qemu
>> to detect capabilities and this code. The above line seems to be the
>> root problem - if I take your patch and remove 'smc' then libvirt works
>> as expected and I can see ais in the domcapabilities.
>>
>> Looking at those wrappers David mentioned... I suspect you need this
>> for the 'none' machine case. I tried a quick hack with the following:
>>
>> bool ais_allowed(void)
>> {
>> /* for "none" machine this results in true */
>> return get_machine_class()->kvm_ais_allowed;
>> }
>>
>> and
>>
>> if (ais_allowed() &&
>> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>> }
>>
>> This works and doesn't break libvirt compatibility detection.
>
> Oh, "none" machine fun again... I think you're on the right track, and
> we really need a wrapper.
D'oh, so this is the real reason for the wrappers ... ok, I'll respin my
patch accordingly.
Thanks a lot for the testing, Matthew!
Thomas
On 17.01.20 11:38, Thomas Huth wrote:
> On 16/01/2020 21.26, Cornelia Huck wrote:
>> On Thu, 16 Jan 2020 15:19:13 -0500
>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>
>>> On 1/16/20 7:20 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.
>>>>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>>> target/s390x/kvm.c | 11 ++++++++---
>>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> (...)
>>
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index 15260aeb9a..4c1c8c0208 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>>>
>>>> int kvm_arch_init(MachineState *ms, KVMState *s)
>>>> {
>>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
>>>> +
>>>
>>> I still can't run a proper test due to unavailable hw but in the
>>> meantime I tried to virsh define a libvirt guest pointed at qemu (master
>>> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or
>>> s390-ccw-virtio-4.2) I get:
>>>
>>> virsh define guest.xml
>>> error: Failed to define domain from /path/to/guest.xml
>>> error: invalid argument: could not find capabilities for arch=s390x
>>> domaintype=kvm
>>>
>>> Similarly:
>>>
>>> virsh domcapabilities
>>> error: failed to get emulator capabilities
>>> error: invalid argument: unable to find any emulator to serve 's390x'
>>> architecture
>>>
>>> Rolling back to qemu master, the define and domcapabilities work (with
>>> no ais of course).
>>>
>>> So: there is some incompatibility between the way libvirt invokes qemu
>>> to detect capabilities and this code. The above line seems to be the
>>> root problem - if I take your patch and remove 'smc' then libvirt works
>>> as expected and I can see ais in the domcapabilities.
>>>
>>> Looking at those wrappers David mentioned... I suspect you need this
>>> for the 'none' machine case. I tried a quick hack with the following:
>>>
>>> bool ais_allowed(void)
>>> {
>>> /* for "none" machine this results in true */
>>> return get_machine_class()->kvm_ais_allowed;
>>> }
>>>
>>> and
>>>
>>> if (ais_allowed() &&
>>> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>>> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>>> }
>>>
>>> This works and doesn't break libvirt compatibility detection.
>>
>> Oh, "none" machine fun again... I think you're on the right track, and
>> we really need a wrapper.
>
> D'oh, so this is the real reason for the wrappers ... ok, I'll respin my
> patch accordingly.
Can you add a comment to the wrappers?
>
> Thanks a lot for the testing, Matthew!
>
> Thomas
>
On 17/01/2020 12.05, Christian Borntraeger wrote:
>
>
> On 17.01.20 11:38, Thomas Huth wrote:
>> On 16/01/2020 21.26, Cornelia Huck wrote:
>>> On Thu, 16 Jan 2020 15:19:13 -0500
>>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>>
>>>> On 1/16/20 7:20 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.
>>>>>
>>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>>>> target/s390x/kvm.c | 11 ++++++++---
>>>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> (...)
>>>
>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>> index 15260aeb9a..4c1c8c0208 100644
>>>>> --- a/target/s390x/kvm.c
>>>>> +++ b/target/s390x/kvm.c
>>>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>>>>
>>>>> int kvm_arch_init(MachineState *ms, KVMState *s)
>>>>> {
>>>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
>>>>> +
>>>>
>>>> I still can't run a proper test due to unavailable hw but in the
>>>> meantime I tried to virsh define a libvirt guest pointed at qemu (master
>>>> + this patch). Regardless of machine type (s390-ccw-virtio-5.0 or
>>>> s390-ccw-virtio-4.2) I get:
>>>>
>>>> virsh define guest.xml
>>>> error: Failed to define domain from /path/to/guest.xml
>>>> error: invalid argument: could not find capabilities for arch=s390x
>>>> domaintype=kvm
>>>>
>>>> Similarly:
>>>>
>>>> virsh domcapabilities
>>>> error: failed to get emulator capabilities
>>>> error: invalid argument: unable to find any emulator to serve 's390x'
>>>> architecture
>>>>
>>>> Rolling back to qemu master, the define and domcapabilities work (with
>>>> no ais of course).
>>>>
>>>> So: there is some incompatibility between the way libvirt invokes qemu
>>>> to detect capabilities and this code. The above line seems to be the
>>>> root problem - if I take your patch and remove 'smc' then libvirt works
>>>> as expected and I can see ais in the domcapabilities.
>>>>
>>>> Looking at those wrappers David mentioned... I suspect you need this
>>>> for the 'none' machine case. I tried a quick hack with the following:
>>>>
>>>> bool ais_allowed(void)
>>>> {
>>>> /* for "none" machine this results in true */
>>>> return get_machine_class()->kvm_ais_allowed;
>>>> }
>>>>
>>>> and
>>>>
>>>> if (ais_allowed() &&
>>>> kvm_check_extension(s, KVM_CAP_S390_AIS_MIGRATION)) {
>>>> kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>>>> }
>>>>
>>>> This works and doesn't break libvirt compatibility detection.
>>>
>>> Oh, "none" machine fun again... I think you're on the right track, and
>>> we really need a wrapper.
>>
>> D'oh, so this is the real reason for the wrappers ... ok, I'll respin my
>> patch accordingly.
>
>
> Can you add a comment to the wrappers?
Yes, good idea, I'll do that!
Thomas
On 16.01.20 13:20, 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.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 4 ++++
> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
> target/s390x/kvm.c | 11 ++++++++---
> 3 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e7eadd14e8..6f43136396 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -456,6 +456,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;
> @@ -662,6 +663,9 @@ 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);
> +
> + s390mc->kvm_ais_allowed = false;
> ccw_machine_5_0_class_options(mc);
> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> }
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 8aa27199c9..f142d379c6 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -21,6 +21,9 @@
> #define S390_MACHINE_CLASS(klass) \
> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>
> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE)
> +
> typedef struct S390CcwMachineState {
> /*< private >*/
> MachineState parent_obj;
> @@ -40,6 +43,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 */
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 15260aeb9a..4c1c8c0208 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>
> int kvm_arch_init(MachineState *ms, KVMState *s)
> {
> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
> +
> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
> false, NULL);
>
> @@ -365,10 +367,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 enable this for newer
> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
> */
> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
> + if (smc->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;
>
We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed().
Care to create a similar wrapper?
apart from that
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
On 16/01/2020 13.23, David Hildenbrand wrote:
> On 16.01.20 13:20, 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.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>> target/s390x/kvm.c | 11 ++++++++---
>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e7eadd14e8..6f43136396 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -456,6 +456,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;
>> @@ -662,6 +663,9 @@ 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);
>> +
>> + s390mc->kvm_ais_allowed = false;
>> ccw_machine_5_0_class_options(mc);
>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>> }
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>> index 8aa27199c9..f142d379c6 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -21,6 +21,9 @@
>> #define S390_MACHINE_CLASS(klass) \
>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>>
>> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \
>> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE)
>> +
>> typedef struct S390CcwMachineState {
>> /*< private >*/
>> MachineState parent_obj;
>> @@ -40,6 +43,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 */
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 15260aeb9a..4c1c8c0208 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>
>> int kvm_arch_init(MachineState *ms, KVMState *s)
>> {
>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
>> +
>> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
>> false, NULL);
>>
>> @@ -365,10 +367,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 enable this for newer
>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
>> */
>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>> + if (smc->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;
>>
>
> We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed().
>
> Care to create a similar wrapper?
Honestly, why do we need these wrappers at all? They look cumbersome to
me. I'd rather remove them in case they are not urgently needed (so far
I don't see the point... could someone enlighten me why we have them?).
Thomas
On 16.01.20 13:26, Thomas Huth wrote:
> On 16/01/2020 13.23, David Hildenbrand wrote:
>> On 16.01.20 13:20, 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.
>>>
>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>> target/s390x/kvm.c | 11 ++++++++---
>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index e7eadd14e8..6f43136396 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -456,6 +456,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;
>>> @@ -662,6 +663,9 @@ 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);
>>> +
>>> + s390mc->kvm_ais_allowed = false;
>>> ccw_machine_5_0_class_options(mc);
>>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>>> }
>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>>> index 8aa27199c9..f142d379c6 100644
>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>> @@ -21,6 +21,9 @@
>>> #define S390_MACHINE_CLASS(klass) \
>>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>>>
>>> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \
>>> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE)
>>> +
>>> typedef struct S390CcwMachineState {
>>> /*< private >*/
>>> MachineState parent_obj;
>>> @@ -40,6 +43,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 */
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 15260aeb9a..4c1c8c0208 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>>
>>> int kvm_arch_init(MachineState *ms, KVMState *s)
>>> {
>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
>>> +
>>> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
>>> false, NULL);
>>>
>>> @@ -365,10 +367,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 enable this for newer
>>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
>>> */
>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>> + if (smc->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;
>>>
>>
>> We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed().
>>
>> Care to create a similar wrapper?
>
> Honestly, why do we need these wrappers at all? They look cumbersome to
> me. I'd rather remove them in case they are not urgently needed (so far
> I don't see the point... could someone enlighten me why we have them?).
I assume to minimize the number of places you have to lookup the
machine/machine class.
--
Thanks,
David / dhildenb
On 16/01/2020 13.28, David Hildenbrand wrote:
> On 16.01.20 13:26, Thomas Huth wrote:
>> On 16/01/2020 13.23, David Hildenbrand wrote:
>>> On 16.01.20 13:20, 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.
>>>>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1756946
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> hw/s390x/s390-virtio-ccw.c | 4 ++++
>>>> include/hw/s390x/s390-virtio-ccw.h | 4 ++++
>>>> target/s390x/kvm.c | 11 ++++++++---
>>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index e7eadd14e8..6f43136396 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -456,6 +456,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;
>>>> @@ -662,6 +663,9 @@ 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);
>>>> +
>>>> + s390mc->kvm_ais_allowed = false;
>>>> ccw_machine_5_0_class_options(mc);
>>>> compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>>>> }
>>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>>>> index 8aa27199c9..f142d379c6 100644
>>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>>> @@ -21,6 +21,9 @@
>>>> #define S390_MACHINE_CLASS(klass) \
>>>> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE)
>>>>
>>>> +#define S390_CCW_MACHINE_OBJ_GET_CLASS(obj) \
>>>> + OBJECT_GET_CLASS(S390CcwMachineClass, obj, TYPE_S390_CCW_MACHINE)
>>>> +
>>>> typedef struct S390CcwMachineState {
>>>> /*< private >*/
>>>> MachineState parent_obj;
>>>> @@ -40,6 +43,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 */
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index 15260aeb9a..4c1c8c0208 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -329,6 +329,8 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>>>
>>>> int kvm_arch_init(MachineState *ms, KVMState *s)
>>>> {
>>>> + S390CcwMachineClass *smc = S390_CCW_MACHINE_OBJ_GET_CLASS(ms);
>>>> +
>>>> object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
>>>> false, NULL);
>>>>
>>>> @@ -365,10 +367,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 enable this for newer
>>>> + * machine types and if KVM_CAP_S390_AIS_MIGRATION is available.
>>>> */
>>>> - /* kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0); */
>>>> + if (smc->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;
>>>>
>>>
>>> We have ri_allowed(), cpu_model_allowed(), hpage_1m_allowed().
>>>
>>> Care to create a similar wrapper?
>>
>> Honestly, why do we need these wrappers at all? They look cumbersome to
>> me. I'd rather remove them in case they are not urgently needed (so far
>> I don't see the point... could someone enlighten me why we have them?).
>
> I assume to minimize the number of places you have to lookup the
> machine/machine class.
I don't think that any of these functions is performance critical, since
they are only used during the initialization phase...
But looking more closely, cpu_model_allowed() and hpage_1m_allowed() are
used in functions where the current machine state / class is not
directly available, so the wrappers indeed make sense there.
We could remove the ri_allowed() wrapper, though, since this is also
only used in kvm_arch_init() where the machine state is easily available.
Thomas
© 2016 - 2024 Red Hat, Inc.