Re: [Qemu-devel] [PATCH 1/1] s390x: create a compat s390 phb for <=2.10

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 28 Sep 2017 12:41:54 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 09/28/2017 12:34 PM, Christian Borntraeger wrote:
> > 
> > 
> > On 09/27/2017 05:03 PM, Cornelia Huck wrote:  
> >> On Wed, 27 Sep 2017 15:49:27 +0100
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >>  
> >>> * Cornelia Huck (cohuck@redhat.com) wrote:  
> >>>> On Wed, 27 Sep 2017 15:28:38 +0100
> >>>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >>>>     
> >>>>> * David Hildenbrand (david@redhat.com) wrote:    
> >>>>>> On 27.09.2017 12:59, Christian Borntraeger wrote:      
> >>>>>>>
> >>>>>>>
> >>>>>>> On 09/27/2017 12:56 PM, Cornelia Huck wrote:      
> >>>>>>>> On Wed, 27 Sep 2017 18:25:00 +0800
> >>>>>>>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
> >>>>>>>>      
> >>>>>>>>> 在 2017/9/27 下午5:47, Cornelia Huck 写道:      
> >>>>>>>>>> On Tue, 26 Sep 2017 20:40:25 +0200
> >>>>>>>>>> David Hildenbrand <david@redhat.com> wrote:      
> >>>>>>>>      
> >>>>>>>>>>> I'd really really really (did I mention really?) favor something like a
> >>>>>>>>>>> dummy device, because we could easily handle the !CONFIG_PCI case then.
> >>>>>>>>>>>
> >>>>>>>>>>> All these compat options and conditions will kill us someday... we're
> >>>>>>>>>>> already patching around that whole stuff way too much.
> >>>>>>>>>>>
> >>>>>>>>>>> If we ever unconditionally created a device, we should keep doing so.        
> >>>>>>>>>> Yes, that whole thing is horrible, especially interaction with compat
> >>>>>>>>>> machines.
> >>>>>>>>>>
> >>>>>>>>>> Do you have an idea on how to create such a dummy device (without
> >>>>>>>>>> having to effectively copy a lot of configured-out code)?
> >>>>>>>>>>
> >>>>>>>>>>        
> >>>>>>>>> How about in s390_pcihost_hot_plug() we check s390_has_feat(zpci)?
> >>>>>>>>> If no zpci feature, we avoid plugging any pci device.
> >>>>>>>>> Then we could always create phb.
> >>>>>>>>> I think pcibus's vmstate is only data to migrate.      
> >>>>>>>>
> >>>>>>>> That's still problematic if CONFIG_PCI is off. I currently don't have a
> >>>>>>>> better idea than either disallowing compat machines on builds without
> >>>>>>>> pci, or using a dummy device...      
> >>>>>>>
> >>>>>>> For this particular case your initial patch might be less problematic than
> >>>>>>> a dummy device, because the code that does the migration is NOT contained
> >>>>>>> in s390 specific code but in common PCI code instead. We would need to keep
> >>>>>>> the dummy device always in a way that it will work with the common PCI
> >>>>>>> code.
> >>>>>>>       
> >>>>>>
> >>>>>> Interesting, so how is migration then handled for e.g. x86 or other
> >>>>>> architectures that can work without CONFIG_PCI? I assume their migration
> >>>>>> should also break?      
> >>>>>
> >>>>> It's tied to machine-type; the x86 i440fx and q35 machine types have
> >>>>> PCI; you can't disable PCI while still having those machine types.
> >>>>> (I don't know if you can disable PCI at all on x86)    
> >>>>
> >>>> Ugh, that sounds like we need two machine types on s390x as well
> >>>> (s390x-ccw-virtio and s390x-ccw-virtio-nopci or so), built
> >>>> conditionally. That whole zpci detanglement is looking worse and
> >>>> worse :(    
> >>>
> >>> Well fundamentally the migration expects to migrate something into
> >>> the same shaped hole on the destination;  if you've got a lump of PCI
> >>> config on the source there's got to be somewhere for it to fit on the
> >>> destination.
> >>> Now, if PCI is actually pretty rare; then you might be able to make
> >>> the host-pci bridge a normal device and not include it in any
> >>> machine type; that way those who want PCI can just instantiate
> >>> the host-pci bridge, and those who don't want it just stick with
> >>> the base machine type.  
> >>
> >> I fear that ship has already sailed; the s390-ccw-virtio machine type
> >> has been instantiating a phb for quite some time, which means we have
> >> to drag this on in the compat machines...  
> > 
> > In the end that means that you should revert
> > 
> > commit d32bd032d8fde41281aae34c16a4aa97e9acfeac
> > Author:     Cornelia Huck <cohuck@redhat.com>
> > AuthorDate: Thu Jul 6 17:21:52 2017 +0200
> > Commit:     Cornelia Huck <cohuck@redhat.com>
> > CommitDate: Wed Aug 30 18:23:25 2017 +0200
> > 
> >     s390x/ccw: create s390 phb conditionally
> > 
> > to keep s390-ccw-virtio clean proper.
> > 
> > If you want to have PCI disabled, you can do you in a machine like   
> too quick                                     ^that^					 
> > s390-rhelx.y.z or whatever.   
> 
> I think we really must revert this commit. 
> 

A set of non-pci machines looks more attractive to me. I currently have
the following (not yet tested):

From 5dd282bd6e12dca0ca8252019a4c4c58e729b2c5 Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cohuck@redhat.com>
Date: Thu, 28 Sep 2017 14:00:48 +0200
Subject: [PATCH] s390x: set of -nopci machines for non-pci builds

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1bcb7000ab..e032857b6e 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -266,7 +266,7 @@ static void ccw_init(MachineState *machine)
                       machine->initrd_filename, "s390-ccw.img",
                       "s390-netboot.img", true);
 
-    if (s390_has_feat(S390_FEAT_ZPCI)) {
+    if (pci_available) {
         DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
         object_property_add_child(qdev_get_machine(),
                                   TYPE_S390_PCI_HOST_BRIDGE,
@@ -596,9 +596,11 @@ bool css_migration_enabled(void)
     {                                                                         \
         MachineClass *mc = MACHINE_CLASS(oc);                                 \
         ccw_machine_##suffix##_class_options(mc);                             \
-        mc->desc = "VirtIO-ccw based S390 machine v" verstr;                  \
+        mc->desc = pci_available ? "VirtIO-ccw based S390 machine v" verstr : \
+            "VirtIO-ccw based S390 machine (no PCI) v" verstr;                \
         if (latest) {                                                         \
-            mc->alias = "s390-ccw-virtio";                                    \
+            mc->alias = pci_available ? "s390-ccw-virtio" :                   \
+                "s390-ccw-virtio-nopci";                                      \
             mc->is_default = 1;                                               \
         }                                                                     \
     }                                                                         \
@@ -609,14 +611,24 @@ bool css_migration_enabled(void)
         ccw_machine_##suffix##_instance_options(machine);                     \
     }                                                                         \
     static const TypeInfo ccw_machine_##suffix##_info = {                     \
-        .name = MACHINE_TYPE_NAME("s390-ccw-virtio-" verstr),                 \
+        .name = MACHINE_TYPE_NAME("s390-virtio-ccw-" verstr),                 \
+        .parent = TYPE_S390_CCW_MACHINE,                                      \
+        .class_init = ccw_machine_##suffix##_class_init,                      \
+        .instance_init = ccw_machine_##suffix##_instance_init,                \
+    };                                                                        \
+    static const TypeInfo ccw_machine_nopci_##suffix##_info = {               \
+        .name = MACHINE_TYPE_NAME("s390-virtio-ccw-nopci-" verstr),           \
         .parent = TYPE_S390_CCW_MACHINE,                                      \
         .class_init = ccw_machine_##suffix##_class_init,                      \
         .instance_init = ccw_machine_##suffix##_instance_init,                \
     };                                                                        \
     static void ccw_machine_register_##suffix(void)                           \
     {                                                                         \
-        type_register_static(&ccw_machine_##suffix##_info);                   \
+        if (pci_available) {                                                  \
+            type_register_static(&ccw_machine_##suffix##_info);               \
+        } else {                                                              \
+            type_register_static(&ccw_machine_nopci_##suffix##_info);         \
+        }                                                                     \
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
-- 
2.13.5