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

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 09/18/2017 10:07 AM, Cornelia Huck wrote:
> On Mon, 18 Sep 2017 09:47:00 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> 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 <cohuck@redhat.com>
>>>> ---
>>>>  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.

So with an improved patch description (explain the fix better)

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