[Qemu-devel] [PATCH 0/1] s390x: more zpci compat fun

[Qemu-devel] [PATCH 0/1] s390x: more zpci compat fun

[Cornelia Huck]

Here's a (hopefully) less hackish variant of the fix in
<932ad4e2-a10c-77fc-fcb7-5dbbed0d2c7c@de.ibm.com>. Not really tested
(I had problems reproducing the problem on z12, but that might be
just me).

Please review & test, if possible. Patch against my s390-next branch
(but should fix on top of master as well, I think.)

Cornelia Huck (1):
  s390x: create a compat s390 phb for <=2.10

 hw/s390x/s390-virtio-ccw.c         | 8 +++++++-
 include/hw/s390x/s390-virtio-ccw.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.13.5

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

[Cornelia Huck]

d32bd032d8 ("s390x/ccw: create s390 phb conditionally") made
creating the s390 phb dependant on the zpci facility. This broke
migration from pre-cpu model machines which was fixed with
8ad9087c4a ("s390x/ccw: create s390 phb for compat reasons as well").
However, that is not enough: Migration from 2.10 with -cpu z13
breaks as well.

Let's create a phb for all pre-2.11 compat machines to fix this.
We leave the zpci facility off to avoid a guest-visible change
with cpu models on.

Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c         | 8 +++++++-
 include/hw/s390x/s390-virtio-ccw.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1bcb7000ab..981f1c4336 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -247,6 +247,8 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
     }
 }
 
+static S390CcwMachineClass *get_machine_class(void);
+
 static void ccw_init(MachineState *machine)
 {
     int ret;
@@ -266,7 +268,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 (s390_has_feat(S390_FEAT_ZPCI) || get_machine_class()->phb_compat) {
         DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
         object_property_add_child(qdev_get_machine(),
                                   TYPE_S390_PCI_HOST_BRIDGE,
@@ -407,6 +409,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->cpu_model_allowed = true;
     s390mc->css_migration_enabled = true;
     s390mc->gs_allowed = true;
+    s390mc->phb_compat = false;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->hot_add_cpu = s390_hot_add_cpu;
@@ -716,6 +719,9 @@ static void ccw_machine_2_10_instance_options(MachineState *machine)
 
 static void ccw_machine_2_10_class_options(MachineClass *mc)
 {
+    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
+
+    s390mc->phb_compat = pci_available;
     ccw_machine_2_11_class_options(mc);
     SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_10);
 }
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index a9a90c2022..fb717afe92 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -41,6 +41,7 @@ typedef struct S390CcwMachineClass {
     bool cpu_model_allowed;
     bool css_migration_enabled;
     bool gs_allowed;
+    bool phb_compat;
 } S390CcwMachineClass;
 
 /* runtime-instrumentation allowed by the machine */
-- 
2.13.5

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

[Christian Borntraeger]

On 09/26/2017 06:20 PM, Cornelia Huck wrote:
> d32bd032d8 ("s390x/ccw: create s390 phb conditionally") made
> creating the s390 phb dependant on the zpci facility. This broke
> migration from pre-cpu model machines which was fixed with
> 8ad9087c4a ("s390x/ccw: create s390 phb for compat reasons as well").
> However, that is not enough: Migration from 2.10 with -cpu z13
> breaks as well.
> 
> Let's create a phb for all pre-2.11 compat machines to fix this.
> We leave the zpci facility off to avoid a guest-visible change
> with cpu models on.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

seems to work fine. 
> ---
>  hw/s390x/s390-virtio-ccw.c         | 8 +++++++-
>  include/hw/s390x/s390-virtio-ccw.h | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1bcb7000ab..981f1c4336 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -247,6 +247,8 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>      }
>  }
> 
> +static S390CcwMachineClass *get_machine_class(void);
> +
>  static void ccw_init(MachineState *machine)
>  {
>      int ret;
> @@ -266,7 +268,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 (s390_has_feat(S390_FEAT_ZPCI) || get_machine_class()->phb_compat) {
>          DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
>          object_property_add_child(qdev_get_machine(),
>                                    TYPE_S390_PCI_HOST_BRIDGE,
> @@ -407,6 +409,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      s390mc->cpu_model_allowed = true;
>      s390mc->css_migration_enabled = true;
>      s390mc->gs_allowed = true;
> +    s390mc->phb_compat = false;
>      mc->init = ccw_init;
>      mc->reset = s390_machine_reset;
>      mc->hot_add_cpu = s390_hot_add_cpu;
> @@ -716,6 +719,9 @@ static void ccw_machine_2_10_instance_options(MachineState *machine)
> 
>  static void ccw_machine_2_10_class_options(MachineClass *mc)
>  {
> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> +
> +    s390mc->phb_compat = pci_available;
>      ccw_machine_2_11_class_options(mc);
>      SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_10);
>  }
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index a9a90c2022..fb717afe92 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -41,6 +41,7 @@ typedef struct S390CcwMachineClass {
>      bool cpu_model_allowed;
>      bool css_migration_enabled;
>      bool gs_allowed;
> +    bool phb_compat;
>  } S390CcwMachineClass;
> 
>  /* runtime-instrumentation allowed by the machine */
> 

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

[David Hildenbrand]

On 26.09.2017 18:20, Cornelia Huck wrote:
> d32bd032d8 ("s390x/ccw: create s390 phb conditionally") made
> creating the s390 phb dependant on the zpci facility. This broke
> migration from pre-cpu model machines which was fixed with
> 8ad9087c4a ("s390x/ccw: create s390 phb for compat reasons as well").
> However, that is not enough: Migration from 2.10 with -cpu z13
> breaks as well.
> 
> Let's create a phb for all pre-2.11 compat machines to fix this.
> We leave the zpci facility off to avoid a guest-visible change
> with cpu models on.
> 
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c         | 8 +++++++-
>  include/hw/s390x/s390-virtio-ccw.h | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 1bcb7000ab..981f1c4336 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -247,6 +247,8 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>      }
>  }
>  
> +static S390CcwMachineClass *get_machine_class(void);
> +
>  static void ccw_init(MachineState *machine)
>  {
>      int ret;
> @@ -266,7 +268,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 (s390_has_feat(S390_FEAT_ZPCI) || get_machine_class()->phb_compat) {
>          DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
>          object_property_add_child(qdev_get_machine(),
>                                    TYPE_S390_PCI_HOST_BRIDGE,
> @@ -407,6 +409,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      s390mc->cpu_model_allowed = true;
>      s390mc->css_migration_enabled = true;
>      s390mc->gs_allowed = true;
> +    s390mc->phb_compat = false;
>      mc->init = ccw_init;
>      mc->reset = s390_machine_reset;
>      mc->hot_add_cpu = s390_hot_add_cpu;
> @@ -716,6 +719,9 @@ static void ccw_machine_2_10_instance_options(MachineState *machine)
>  
>  static void ccw_machine_2_10_class_options(MachineClass *mc)
>  {
> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> +
> +    s390mc->phb_compat = pci_available;
>      ccw_machine_2_11_class_options(mc);
>      SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_10);
>  }
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index a9a90c2022..fb717afe92 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -41,6 +41,7 @@ typedef struct S390CcwMachineClass {
>      bool cpu_model_allowed;
>      bool css_migration_enabled;
>      bool gs_allowed;
> +    bool phb_compat;
>  } S390CcwMachineClass;
>  
>  /* runtime-instrumentation allowed by the machine */
> 

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.

-- 

Thanks,

David

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

[Cornelia Huck]

On Tue, 26 Sep 2017 20:40:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 26.09.2017 18:20, Cornelia Huck wrote:
> > d32bd032d8 ("s390x/ccw: create s390 phb conditionally") made
> > creating the s390 phb dependant on the zpci facility. This broke
> > migration from pre-cpu model machines which was fixed with
> > 8ad9087c4a ("s390x/ccw: create s390 phb for compat reasons as well").
> > However, that is not enough: Migration from 2.10 with -cpu z13
> > breaks as well.
> > 
> > Let's create a phb for all pre-2.11 compat machines to fix this.
> > We leave the zpci facility off to avoid a guest-visible change
> > with cpu models on.
> > 
> > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c         | 8 +++++++-
> >  include/hw/s390x/s390-virtio-ccw.h | 1 +
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 1bcb7000ab..981f1c4336 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -247,6 +247,8 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
> >      }
> >  }
> >  
> > +static S390CcwMachineClass *get_machine_class(void);
> > +
> >  static void ccw_init(MachineState *machine)
> >  {
> >      int ret;
> > @@ -266,7 +268,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 (s390_has_feat(S390_FEAT_ZPCI) || get_machine_class()->phb_compat) {
> >          DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
> >          object_property_add_child(qdev_get_machine(),
> >                                    TYPE_S390_PCI_HOST_BRIDGE,
> > @@ -407,6 +409,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
> >      s390mc->cpu_model_allowed = true;
> >      s390mc->css_migration_enabled = true;
> >      s390mc->gs_allowed = true;
> > +    s390mc->phb_compat = false;
> >      mc->init = ccw_init;
> >      mc->reset = s390_machine_reset;
> >      mc->hot_add_cpu = s390_hot_add_cpu;
> > @@ -716,6 +719,9 @@ static void ccw_machine_2_10_instance_options(MachineState *machine)
> >  
> >  static void ccw_machine_2_10_class_options(MachineClass *mc)
> >  {
> > +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
> > +
> > +    s390mc->phb_compat = pci_available;
> >      ccw_machine_2_11_class_options(mc);
> >      SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_10);
> >  }
> > diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> > index a9a90c2022..fb717afe92 100644
> > --- a/include/hw/s390x/s390-virtio-ccw.h
> > +++ b/include/hw/s390x/s390-virtio-ccw.h
> > @@ -41,6 +41,7 @@ typedef struct S390CcwMachineClass {
> >      bool cpu_model_allowed;
> >      bool css_migration_enabled;
> >      bool gs_allowed;
> > +    bool phb_compat;
> >  } S390CcwMachineClass;
> >  
> >  /* runtime-instrumentation allowed by the machine */
> >   
> 
> 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)?

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

[Yi Min Zhao]

在 2017/9/27 下午5:47, Cornelia Huck 写道:
> On Tue, 26 Sep 2017 20:40:25 +0200
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 26.09.2017 18:20, Cornelia Huck wrote:
>>> d32bd032d8 ("s390x/ccw: create s390 phb conditionally") made
>>> creating the s390 phb dependant on the zpci facility. This broke
>>> migration from pre-cpu model machines which was fixed with
>>> 8ad9087c4a ("s390x/ccw: create s390 phb for compat reasons as well").
>>> However, that is not enough: Migration from 2.10 with -cpu z13
>>> breaks as well.
>>>
>>> Let's create a phb for all pre-2.11 compat machines to fix this.
>>> We leave the zpci facility off to avoid a guest-visible change
>>> with cpu models on.
>>>
>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c         | 8 +++++++-
>>>   include/hw/s390x/s390-virtio-ccw.h | 1 +
>>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 1bcb7000ab..981f1c4336 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -247,6 +247,8 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>>>       }
>>>   }
>>>   
>>> +static S390CcwMachineClass *get_machine_class(void);
>>> +
>>>   static void ccw_init(MachineState *machine)
>>>   {
>>>       int ret;
>>> @@ -266,7 +268,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 (s390_has_feat(S390_FEAT_ZPCI) || get_machine_class()->phb_compat) {
>>>           DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
>>>           object_property_add_child(qdev_get_machine(),
>>>                                     TYPE_S390_PCI_HOST_BRIDGE,
>>> @@ -407,6 +409,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>>>       s390mc->cpu_model_allowed = true;
>>>       s390mc->css_migration_enabled = true;
>>>       s390mc->gs_allowed = true;
>>> +    s390mc->phb_compat = false;
>>>       mc->init = ccw_init;
>>>       mc->reset = s390_machine_reset;
>>>       mc->hot_add_cpu = s390_hot_add_cpu;
>>> @@ -716,6 +719,9 @@ static void ccw_machine_2_10_instance_options(MachineState *machine)
>>>   
>>>   static void ccw_machine_2_10_class_options(MachineClass *mc)
>>>   {
>>> +    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
>>> +
>>> +    s390mc->phb_compat = pci_available;
>>>       ccw_machine_2_11_class_options(mc);
>>>       SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_10);
>>>   }
>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>>> index a9a90c2022..fb717afe92 100644
>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>> @@ -41,6 +41,7 @@ typedef struct S390CcwMachineClass {
>>>       bool cpu_model_allowed;
>>>       bool css_migration_enabled;
>>>       bool gs_allowed;
>>> +    bool phb_compat;
>>>   } S390CcwMachineClass;
>>>   
>>>   /* runtime-instrumentation allowed by the machine */
>>>    
>> 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.

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

[Cornelia Huck]

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...

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

[Christian Borntraeger]

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.

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

[David Hildenbrand]

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?

-- 

Thanks,

David

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

[Christian Borntraeger]

On 09/27/2017 02:21 PM, David Hildenbrand 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?

If you migrate from a QEMU with CONFIG_PCI to a QEMU without CONFIG_PCI 
I assume it will break. But maybe there is really some clever way to
do the right thing. 

PS: Is really anybody disabling CONFIG_PCI and why?

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

[Dr. David Alan Gilbert]

* 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)

Dave

> -- 
> 
> Thanks,
> 
> David
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Cornelia Huck]

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 :(

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

[Dr. David Alan Gilbert]

* 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.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Cornelia Huck]

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...

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

[Christian Borntraeger]

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 
s390-rhelx.y.z or whatever. 

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

[Christian Borntraeger]

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. 

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

[Cornelia Huck]

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

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

[Christian Borntraeger]

On 09/28/2017 02:07 PM, Cornelia Huck wrote:
> 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):


I see no point if PCI disable machines. There is no non-PCI x86 machine
besides the isapc. But this has no version whatsoever so it certainly
is not made for being migrated.
As far as I can see and we would make things much more complex.

non-PCI will trigger other issues. e.g. zpci and aen is part of the z14 cpu model
since 2.10. Really this is not helping, its making things worse.  

If you really want to avoid PCI for whatever redhat is doing, can't you just hide
it in a rhel specific machine (which you seem to have anyway for x86 and power)?

So unless you convince me otherwise consider nopci machine for the upstream
machines NACKed by me.

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

[Cornelia Huck]

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

> I see no point if PCI disable machines. There is no non-PCI x86 machine
> besides the isapc. But this has no version whatsoever so it certainly
> is not made for being migrated.
> As far as I can see and we would make things much more complex.
> 
> non-PCI will trigger other issues. e.g. zpci and aen is part of the z14 cpu model
> since 2.10. Really this is not helping, its making things worse.  

What about creating the phb depending upon pci_available? Has no impact
for normal builds unless you muck around manually...

> If you really want to avoid PCI for whatever redhat is doing, can't you just hide
> it in a rhel specific machine (which you seem to have anyway for x86 and power)?

...but makes it easier to disable it correctly, if wanted.

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

[David Hildenbrand]

>> 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. 
> 

I tend to agree.

-- 

Thanks,

David