From: Tony Krowiak <akrowiak@linux.ibm.com>

Introduces the base object model for virtualizing AP devices.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 MAINTAINERS                  |   12 +++++++
 hw/s390x/Makefile.objs       |    2 +
 hw/s390x/ap-bridge.c         |   76 ++++++++++++++++++++++++++++++++++++++++++
 hw/s390x/ap-device.c         |   39 +++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c   |    4 ++
 include/hw/s390x/ap-bridge.h |   37 ++++++++++++++++++++
 include/hw/s390x/ap-device.h |   38 +++++++++++++++++++++
 7 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 hw/s390x/ap-bridge.c
 create mode 100644 hw/s390x/ap-device.c
 create mode 100644 include/hw/s390x/ap-bridge.h
 create mode 100644 include/hw/s390x/ap-device.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d12518c..97e8ed8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1199,6 +1199,18 @@ F: include/hw/s390x/s390-ccw.h
 T: git git://github.com/cohuck/qemu.git s390-next
 L: qemu-s390x@nongnu.org
 
+vfio-ap
+M: Christian Borntraeger <borntraeger@de.ibm.com>
+M: Tony Krowiak <akrowiak@linux.ibm.com>
+M: Halil Pasic <pasic@linux.ibm.com>
+M: Pierre Morel <pmorel@linux.ibm.com>
+S: Supported
+F: hw/s390x/ap-device.c
+F: hw/s390x/ap-bridge.c
+F: include/hw/s390x/ap-device.h
+F: include/hw/s390x/ap-bridge.h
+L: qemu-s390x@nongnu.org
+
 vhost
 M: Michael S. Tsirkin <mst@redhat.com>
 S: Supported
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 93282f7..add89b1 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -20,3 +20,5 @@ obj-$(CONFIG_TCG) += tod-qemu.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
 obj-y += s390-ccw.o
+obj-y += ap-device.o
+obj-y += ap-bridge.o
diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
new file mode 100644
index 0000000..4f10425
--- /dev/null
+++ b/hw/s390x/ap-bridge.c
@@ -0,0 +1,76 @@
+/*
+ * ap bridge
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Halil Pasic <pasic@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/sysbus.h"
+#include "qemu/bitops.h"
+#include "hw/s390x/ap-bridge.h"
+#include "cpu.h"
+
+static char *vfio_ap_bus_get_dev_path(DeviceState *dev)
+{
+    /* at most one */
+    return g_strdup_printf("/1");
+}
+
+static void vfio_ap_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+
+    k->get_dev_path = vfio_ap_bus_get_dev_path;
+    /* More than one vfio-ap device does not make sense */
+    k->max_dev = 1;
+}
+
+static const TypeInfo vfio_ap_bus_info = {
+    .name = TYPE_VFIO_AP_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(VFIOAPBus),
+    .class_init = vfio_ap_bus_class_init,
+};
+
+void s390_init_ap(void)
+{
+    DeviceState *dev;
+
+    /* Create bridge device */
+    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
+    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+
+    /* Create bus on bridge device */
+    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
+ }
+
+
+
+static void ap_bridge_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+}
+
+static const TypeInfo ap_bridge_info = {
+    .name          = TYPE_AP_BRIDGE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(APBridge),
+    .class_init    = ap_bridge_class_init,
+};
+
+static void ap_register(void)
+{
+    type_register_static(&ap_bridge_info);
+    type_register_static(&vfio_ap_bus_info);
+}
+
+type_init(ap_register)
diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
new file mode 100644
index 0000000..3cd4bae
--- /dev/null
+++ b/hw/s390x/ap-device.c
@@ -0,0 +1,39 @@
+/*
+ * Adjunct Processor (AP) matrix device
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+#include "hw/qdev.h"
+#include "hw/s390x/ap-device.h"
+
+static void ap_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "AP device class";
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo ap_device_info = {
+    .name = AP_DEVICE_TYPE,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(APDevice),
+    .class_size = sizeof(APDeviceClass),
+    .class_init = ap_class_init,
+    .abstract = true,
+};
+
+static void ap_device_register(void)
+{
+    type_register_static(&ap_device_info);
+}
+
+type_init(ap_device_register)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index f0f7fdc..3c100c2 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -32,6 +32,7 @@
 #include "ipl.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/css-bridge.h"
+#include "hw/s390x/ap-bridge.h"
 #include "migration/register.h"
 #include "cpu_models.h"
 #include "hw/nmi.h"
@@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
     /* init the SIGP facility */
     s390_init_sigp();
 
+    /* create AP bridge and bus(es) */
+    s390_init_ap();
+
     /* get a BUS */
     css_bus = virtual_css_bus_init();
     s390_init_ipl_dev(machine->kernel_filename, machine->kernel_cmdline,
diff --git a/include/hw/s390x/ap-bridge.h b/include/hw/s390x/ap-bridge.h
new file mode 100644
index 0000000..7841ec2
--- /dev/null
+++ b/include/hw/s390x/ap-bridge.h
@@ -0,0 +1,37 @@
+/*
+ * ap bridge
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Halil Pasic <pasic@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef HW_S390X_AP_BRIDGE_H
+#define HW_S390X_AP_BRIDGE_H
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h"
+
+typedef struct APBridge {
+    SysBusDevice sysbus_dev;
+    bool css_dev_path;
+} APBridge;
+
+#define TYPE_AP_BRIDGE "ap-bridge"
+#define AP_BRIDGE(obj) \
+    OBJECT_CHECK(APBridge, (obj), TYPE_AP_BRIDGE)
+
+typedef struct VFIOAPBus {
+    BusState parent_obj;
+} VFIOAPBus;
+
+#define TYPE_VFIO_AP_BUS "vfio-ap-bus"
+#define VFIO_AP_BUS(obj) \
+     OBJECT_CHECK(VFIOAPBus, (obj), TYPE_VFIO_AP_BUS)
+
+void s390_init_ap(void);
+
+#endif
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
new file mode 100644
index 0000000..693df90
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE       "ap-device"
+
+typedef struct APDevice {
+    DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+    DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+    return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+    OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
+
+#endif /* HW_S390X_AP_DEVICE_H */
-- 
1.7.1

On 2018-09-12 22:08, Tony Krowiak wrote:
> From: Tony Krowiak <akrowiak@linux.ibm.com>
> 
> Introduces the base object model for virtualizing AP devices.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
[...]
> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
> new file mode 100644
> index 0000000..4f10425
> --- /dev/null
> +++ b/hw/s390x/ap-bridge.c
[...]
> +void s390_init_ap(void)
> +{
> +    DeviceState *dev;
> +
> +    /* Create bridge device */
> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
> +                              OBJECT(dev), NULL);
> +    qdev_init_nofail(dev);
> +
> +    /* Create bus on bridge device */
> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
> + }
[...]
> +type_init(ap_device_register)
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f0f7fdc..3c100c2 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -32,6 +32,7 @@
>  #include "ipl.h"
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "hw/s390x/css-bridge.h"
> +#include "hw/s390x/ap-bridge.h"
>  #include "migration/register.h"
>  #include "cpu_models.h"
>  #include "hw/nmi.h"
> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>      /* init the SIGP facility */
>      s390_init_sigp();
>  
> +    /* create AP bridge and bus(es) */
> +    s390_init_ap();
> +

Not sure since there is no explicit migration state involved here, but I
think you cannot simply create the ap-bridge device always
unconditionally, can you? Did you check whether you can ping-pong
migrate an guest that runs on an older version of QEMU to a QEMU that
contains this patch and back? If it does not work, it might be necessary
to restrict this ap-bridge device for new machine types only, I think.

 Thomas

On 09/13/2018 07:48 AM, Thomas Huth wrote:
> On 2018-09-12 22:08, Tony Krowiak wrote:
>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>
>> Introduces the base object model for virtualizing AP devices.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
> [...]
>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>> new file mode 100644
>> index 0000000..4f10425
>> --- /dev/null
>> +++ b/hw/s390x/ap-bridge.c
> [...]
>> +void s390_init_ap(void)
>> +{
>> +    DeviceState *dev;
>> +
>> +    /* Create bridge device */
>> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
>> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
>> +                              OBJECT(dev), NULL);
>> +    qdev_init_nofail(dev);
>> +
>> +    /* Create bus on bridge device */
>> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
>> + }
> [...]
>> +type_init(ap_device_register)
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index f0f7fdc..3c100c2 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -32,6 +32,7 @@
>>  #include "ipl.h"
>>  #include "hw/s390x/s390-virtio-ccw.h"
>>  #include "hw/s390x/css-bridge.h"
>> +#include "hw/s390x/ap-bridge.h"
>>  #include "migration/register.h"
>>  #include "cpu_models.h"
>>  #include "hw/nmi.h"
>> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>>      /* init the SIGP facility */
>>      s390_init_sigp();
>>  
>> +    /* create AP bridge and bus(es) */
>> +    s390_init_ap();
>> +
> 
> Not sure since there is no explicit migration state involved here, but I
> think you cannot simply create the ap-bridge device always
> unconditionally, can you? Did you check whether you can ping-pong
> migrate an guest that runs on an older version of QEMU to a QEMU that
> contains this patch and back? If it does not work, it might be necessary
> to restrict this ap-bridge device for new machine types only, I think.

We have already the cpu features at this point in time. So simply doing
s390_has_feat(S390_FEAT_AP) in s390_init_ap ?

On 09/13/2018 08:29 AM, Christian Borntraeger wrote:
>>> +++ b/hw/s390x/ap-bridge.c
>> [...]
>>> +void s390_init_ap(void)
>>> +{
>>> +    DeviceState *dev;
>>> +
>>> +    /* Create bridge device */
>>> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
>>> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
>>> +                              OBJECT(dev), NULL);
>>> +    qdev_init_nofail(dev);
>>> +
>>> +    /* Create bus on bridge device */
>>> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
>>> + }
>> [...]
>>> +type_init(ap_device_register)
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index f0f7fdc..3c100c2 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -32,6 +32,7 @@
>>>  #include "ipl.h"
>>>  #include "hw/s390x/s390-virtio-ccw.h"
>>>  #include "hw/s390x/css-bridge.h"
>>> +#include "hw/s390x/ap-bridge.h"
>>>  #include "migration/register.h"
>>>  #include "cpu_models.h"
>>>  #include "hw/nmi.h"
>>> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>>>      /* init the SIGP facility */
>>>      s390_init_sigp();
>>>  
>>> +    /* create AP bridge and bus(es) */
>>> +    s390_init_ap();
>>> +
>>
>> Not sure since there is no explicit migration state involved here, but I
>> think you cannot simply create the ap-bridge device always
>> unconditionally, can you? Did you check whether you can ping-pong
>> migrate an guest that runs on an older version of QEMU to a QEMU that
>> contains this patch and back? If it does not work, it might be necessary
>> to restrict this ap-bridge device for new machine types only, I think.
> 
> We have already the cpu features at this point in time. So simply doing

s/doing/checking/

> s390_has_feat(S390_FEAT_AP) in s390_init_ap ?
> 

On 09/13/2018 02:29 AM, Christian Borntraeger wrote:
>
> On 09/13/2018 07:48 AM, Thomas Huth wrote:
>> On 2018-09-12 22:08, Tony Krowiak wrote:
>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>
>>> Introduces the base object model for virtualizing AP devices.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> ---
>> [...]
>>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>>> new file mode 100644
>>> index 0000000..4f10425
>>> --- /dev/null
>>> +++ b/hw/s390x/ap-bridge.c
>> [...]
>>> +void s390_init_ap(void)
>>> +{
>>> +    DeviceState *dev;
>>> +
>>> +    /* Create bridge device */
>>> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
>>> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
>>> +                              OBJECT(dev), NULL);
>>> +    qdev_init_nofail(dev);
>>> +
>>> +    /* Create bus on bridge device */
>>> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
>>> + }
>> [...]
>>> +type_init(ap_device_register)
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index f0f7fdc..3c100c2 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -32,6 +32,7 @@
>>>   #include "ipl.h"
>>>   #include "hw/s390x/s390-virtio-ccw.h"
>>>   #include "hw/s390x/css-bridge.h"
>>> +#include "hw/s390x/ap-bridge.h"
>>>   #include "migration/register.h"
>>>   #include "cpu_models.h"
>>>   #include "hw/nmi.h"
>>> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>>>       /* init the SIGP facility */
>>>       s390_init_sigp();
>>>   
>>> +    /* create AP bridge and bus(es) */
>>> +    s390_init_ap();
>>> +
>> Not sure since there is no explicit migration state involved here, but I
>> think you cannot simply create the ap-bridge device always
>> unconditionally, can you? Did you check whether you can ping-pong
>> migrate an guest that runs on an older version of QEMU to a QEMU that
>> contains this patch and back? If it does not work, it might be necessary
>> to restrict this ap-bridge device for new machine types only, I think.
> We have already the cpu features at this point in time. So simply doing
> s390_has_feat(S390_FEAT_AP) in s390_init_ap ?

This seems like a reasonable solution to me. If the S390_FEAT_AP is not 
switched
on for the guest, then the vfio-ap device will not be realized; therefore,
there is no need for a bridge device. I will fix the s390_init_ap() 
function such
that the AP bridge device is created only if the S390_FEAT_AP is 
switched on for
the guest. That ought to solve the migration issue David pointed out. 
Thanks
David.

On 09/13/2018 01:02 PM, Tony Krowiak wrote:
> On 09/13/2018 02:29 AM, Christian Borntraeger wrote:
>>
>> On 09/13/2018 07:48 AM, Thomas Huth wrote:
>>> On 2018-09-12 22:08, Tony Krowiak wrote:
>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>
>>>> Introduces the base object model for virtualizing AP devices.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>> [...]
>>>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>>>> new file mode 100644
>>>> index 0000000..4f10425
>>>> --- /dev/null
>>>> +++ b/hw/s390x/ap-bridge.c
>>> [...]
>>>> +void s390_init_ap(void)
>>>> +{
>>>> +    DeviceState *dev;
>>>> +
>>>> +    /* Create bridge device */
>>>> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
>>>> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
>>>> +                              OBJECT(dev), NULL);
>>>> +    qdev_init_nofail(dev);
>>>> +
>>>> +    /* Create bus on bridge device */
>>>> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
>>>> + }
>>> [...]
>>>> +type_init(ap_device_register)
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index f0f7fdc..3c100c2 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -32,6 +32,7 @@
>>>>   #include "ipl.h"
>>>>   #include "hw/s390x/s390-virtio-ccw.h"
>>>>   #include "hw/s390x/css-bridge.h"
>>>> +#include "hw/s390x/ap-bridge.h"
>>>>   #include "migration/register.h"
>>>>   #include "cpu_models.h"
>>>>   #include "hw/nmi.h"
>>>> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>>>>       /* init the SIGP facility */
>>>>       s390_init_sigp();
>>>>   +    /* create AP bridge and bus(es) */
>>>> +    s390_init_ap();
>>>> +
>>> Not sure since there is no explicit migration state involved here, 
>>> but I
>>> think you cannot simply create the ap-bridge device always
>>> unconditionally, can you? Did you check whether you can ping-pong
>>> migrate an guest that runs on an older version of QEMU to a QEMU that
>>> contains this patch and back? If it does not work, it might be 
>>> necessary
>>> to restrict this ap-bridge device for new machine types only, I think.
>> We have already the cpu features at this point in time. So simply doing
>> s390_has_feat(S390_FEAT_AP) in s390_init_ap ?
>
> This seems like a reasonable solution to me. If the S390_FEAT_AP is 
> not switched
> on for the guest, then the vfio-ap device will not be realized; 
> therefore,
> there is no need for a bridge device. I will fix the s390_init_ap() 
> function such
> that the AP bridge device is created only if the S390_FEAT_AP is 
> switched on for
> the guest. That ought to solve the migration issue David pointed out. 
> Thanks
> David.

Sorry, meant Thomas.

>
>

On 09/13/2018 07:15 PM, Tony Krowiak wrote:
> On 09/13/2018 01:02 PM, Tony Krowiak wrote:
>> On 09/13/2018 02:29 AM, Christian Borntraeger wrote:
>>>
>>> On 09/13/2018 07:48 AM, Thomas Huth wrote:
>>>> On 2018-09-12 22:08, Tony Krowiak wrote:
>>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>>
>>>>> Introduces the base object model for virtualizing AP devices.
>>>>>
>>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>> ---
>>>> [...]
>>>>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>>>>> new file mode 100644
>>>>> index 0000000..4f10425
>>>>> --- /dev/null
>>>>> +++ b/hw/s390x/ap-bridge.c
>>>> [...]
>>>>> +void s390_init_ap(void)
>>>>> +{
>>>>> +    DeviceState *dev;
>>>>> +
>>>>> +    /* Create bridge device */
>>>>> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
>>>>> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
>>>>> +                              OBJECT(dev), NULL);
>>>>> +    qdev_init_nofail(dev);
>>>>> +
>>>>> +    /* Create bus on bridge device */
>>>>> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
>>>>> + }
>>>> [...]
>>>>> +type_init(ap_device_register)
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index f0f7fdc..3c100c2 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -32,6 +32,7 @@
>>>>>   #include "ipl.h"
>>>>>   #include "hw/s390x/s390-virtio-ccw.h"
>>>>>   #include "hw/s390x/css-bridge.h"
>>>>> +#include "hw/s390x/ap-bridge.h"
>>>>>   #include "migration/register.h"
>>>>>   #include "cpu_models.h"
>>>>>   #include "hw/nmi.h"
>>>>> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>>>>>       /* init the SIGP facility */
>>>>>       s390_init_sigp();
>>>>>   +    /* create AP bridge and bus(es) */
>>>>> +    s390_init_ap();
>>>>> +
>>>> Not sure since there is no explicit migration state involved here, but I
>>>> think you cannot simply create the ap-bridge device always
>>>> unconditionally, can you? Did you check whether you can ping-pong
>>>> migrate an guest that runs on an older version of QEMU to a QEMU that
>>>> contains this patch and back? If it does not work, it might be necessary
>>>> to restrict this ap-bridge device for new machine types only, I think.

@Thomas
I re-checked the back and forth migration using libvirt's save/restore. It
works, because as you said, there in no migration state. So there are no
missing/unexpected subsections in the migration stream/file.

>>> We have already the cpu features at this point in time. So simply doing
>>> s390_has_feat(S390_FEAT_AP) in s390_init_ap ?
>>
>> This seems like a reasonable solution to me. If the S390_FEAT_AP is not switched
>> on for the guest, then the vfio-ap device will not be realized; therefore,
>> there is no need for a bridge device. I will fix the s390_init_ap() function such
>> that the AP bridge device is created only if the S390_FEAT_AP is switched on for
>> the guest. That ought to solve the migration issue David pointed out. Thanks
>> David.

With that said, creating the ap-bridge only under the condition
s390_has_feat(S390_FEAT_AP) could still be the better option. We
could probably drop the check in vfio_ap_realize(). I'm not sure, so
I'm fine either way.

Regards,
Halil


>>
>>
> 
> 

On 09/13/2018 02:24 PM, Halil Pasic wrote:
>
>
> On 09/13/2018 07:15 PM, Tony Krowiak wrote:
>> On 09/13/2018 01:02 PM, Tony Krowiak wrote:
>>> On 09/13/2018 02:29 AM, Christian Borntraeger wrote:
>>>>
>>>> On 09/13/2018 07:48 AM, Thomas Huth wrote:
>>>>> On 2018-09-12 22:08, Tony Krowiak wrote:
>>>>>> From: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>>>
>>>>>> Introduces the base object model for virtualizing AP devices.
>>>>>>
>>>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>>>> ---
>>>>> [...]
>>>>>> diff --git a/hw/s390x/ap-bridge.c b/hw/s390x/ap-bridge.c
>>>>>> new file mode 100644
>>>>>> index 0000000..4f10425
>>>>>> --- /dev/null
>>>>>> +++ b/hw/s390x/ap-bridge.c
>>>>> [...]
>>>>>> +void s390_init_ap(void)
>>>>>> +{
>>>>>> +    DeviceState *dev;
>>>>>> +
>>>>>> +    /* Create bridge device */
>>>>>> +    dev = qdev_create(NULL, TYPE_AP_BRIDGE);
>>>>>> +    object_property_add_child(qdev_get_machine(), TYPE_AP_BRIDGE,
>>>>>> +                              OBJECT(dev), NULL);
>>>>>> +    qdev_init_nofail(dev);
>>>>>> +
>>>>>> +    /* Create bus on bridge device */
>>>>>> +    qbus_create(TYPE_VFIO_AP_BUS, dev, TYPE_VFIO_AP_BUS);
>>>>>> + }
>>>>> [...]
>>>>>> +type_init(ap_device_register)
>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>> index f0f7fdc..3c100c2 100644
>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>> @@ -32,6 +32,7 @@
>>>>>>   #include "ipl.h"
>>>>>>   #include "hw/s390x/s390-virtio-ccw.h"
>>>>>>   #include "hw/s390x/css-bridge.h"
>>>>>> +#include "hw/s390x/ap-bridge.h"
>>>>>>   #include "migration/register.h"
>>>>>>   #include "cpu_models.h"
>>>>>>   #include "hw/nmi.h"
>>>>>> @@ -263,6 +264,9 @@ static void ccw_init(MachineState *machine)
>>>>>>       /* init the SIGP facility */
>>>>>>       s390_init_sigp();
>>>>>>   +    /* create AP bridge and bus(es) */
>>>>>> +    s390_init_ap();
>>>>>> +
>>>>> Not sure since there is no explicit migration state involved here, 
>>>>> but I
>>>>> think you cannot simply create the ap-bridge device always
>>>>> unconditionally, can you? Did you check whether you can ping-pong
>>>>> migrate an guest that runs on an older version of QEMU to a QEMU that
>>>>> contains this patch and back? If it does not work, it might be 
>>>>> necessary
>>>>> to restrict this ap-bridge device for new machine types only, I 
>>>>> think.
>
> @Thomas
> I re-checked the back and forth migration using libvirt's 
> save/restore. It
> works, because as you said, there in no migration state. So there are no
> missing/unexpected subsections in the migration stream/file.
>
>>>> We have already the cpu features at this point in time. So simply 
>>>> doing
>>>> s390_has_feat(S390_FEAT_AP) in s390_init_ap ?
>>>
>>> This seems like a reasonable solution to me. If the S390_FEAT_AP is 
>>> not switched
>>> on for the guest, then the vfio-ap device will not be realized; 
>>> therefore,
>>> there is no need for a bridge device. I will fix the s390_init_ap() 
>>> function such
>>> that the AP bridge device is created only if the S390_FEAT_AP is 
>>> switched on for
>>> the guest. That ought to solve the migration issue David pointed 
>>> out. Thanks
>>> David.
>
> With that said, creating the ap-bridge only under the condition
> s390_has_feat(S390_FEAT_AP) could still be the better option. We
> could probably drop the check in vfio_ap_realize(). I'm not sure, so
> I'm fine either way.

I am including the check for S390_FEAT_AP in the s390_ap_init() function
as recommended by Christian, and removing it from the vfio_ap_realize()
function because if there is no AP bridge, QEMU terminates with an
error when trying to add a vfio-ap device.

>
>
> Regards,
> Halil
>
>
>>>
>>>
>>
>>