| Message ID | 20170926162058.30772-2-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: more zpci compat fun | expand |
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 */ >
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.
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)?
在 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.
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...
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.
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?
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?
* 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
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 :(
* 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
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...
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.
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.
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)
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.
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.
>> 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.
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 */
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(-)