Re: [Qemu-devel] [PATCH RFC v2 1/1] s390x/ccw: create s390 phb for compat reasons as well

[Cornelia Huck]

On Mon, 18 Sep 2017 09:47:00 +0200
Christian Borntraeger <address@hidden> wrote:

> On 09/18/2017 09:33 AM, Christian Borntraeger wrote:
> > 
> > 
> > On 09/15/2017 06:39 PM, Cornelia Huck wrote:  
> >> d32bd032d8 ("s390x/ccw: create s390 phb conditionally") made
> >> registering the s390 pci host bridge conditional on presense
> >> of the zpci facility bit. Sadly, that breaks migration from
> >> machines that did not use the cpu model (2.7 and previous).
> >>
> >> Create the s390 phb for pre-cpu model machines as well.
> >>
> >> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> >> Signed-off-by: Cornelia Huck <address@hidden>
> >> ---
> >>  hw/s390x/s390-virtio-ccw.c | 2 ++
> >>  target/s390x/cpu_models.c  | 3 +++
> >>  2 files changed, 5 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 0471407187..907abc7a32 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -269,6 +269,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;

This hunk is from the previous version and should not be in there...

> >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> >> index c295e641e6..5169379db5 100644
> >> --- a/target/s390x/cpu_models.c
> >> +++ b/target/s390x/cpu_models.c
> >> @@ -196,6 +196,9 @@ bool s390_has_feat(S390Feat feat)
> >>              }
> >>          }
> >>  #endif
> >> +        if (feat == S390_FEAT_ZPCI) {
> >> +            return true;
> >> +        }  
> > 
> > Shouldnt that be depend on the machine being 2.7? I mean unless I misread 
> > the context of this patch, you hard enable the PCI facility bit for all 
> > machines and make all s390_has_feat(S390_FEAT_ZPCI) useless?
> >   
> 
> Sorry, I had to lookup again the original code. So this is inside 
> !cpu->model. 
> Yes makes sense then.

Yes, this is hard to figure out from the context alone.