[Qemu-devel] [PATCH v3] s390x/ccw: create s390 phb for compat reasons as well

[Qemu-devel] [PATCH v3] s390x/ccw: create s390 phb for compat reasons as well

[Cornelia Huck]

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: We can
tweak s390_has_feat() to always indicate the zpci facility bit
when no cpu model is available (on 2.7 and previous compat machines).

Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

v2->v3:
- no longer RFC (I tested a bit more)
- removed unrelated hunk
- more verbose patch description

I'll wait a bit for more acks/reviews and will probably send a pull
request for s390x tomorrow or so before the amount of queued patches
gets out of hand...

---
 target/s390x/cpu_models.c | 3 +++
 1 file changed, 3 insertions(+)

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;
+        }
         return 0;
     }
     return test_bit(feat, cpu->model->features);
-- 
2.13.5

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

[Christian Borntraeger]

On 09/18/2017 10:55 AM, 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: We can
> tweak s390_has_feat() to always indicate the zpci facility bit
> when no cpu model is available (on 2.7 and previous compat machines).

Looks better now. Thanks
> 
> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v2->v3:
> - no longer RFC (I tested a bit more)
> - removed unrelated hunk
> - more verbose patch description
> 
> I'll wait a bit for more acks/reviews and will probably send a pull
> request for s390x tomorrow or so before the amount of queued patches
> gets out of hand...

I have a small amount of patches pending, but then I got sick last week
so I still have to sort out things. Lets do the pull request and we can
then queue the other patches after they have received proper review.

> 
> ---
>  target/s390x/cpu_models.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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;
> +        }
>          return 0;
>      }
>      return test_bit(feat, cpu->model->features);
> 

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

[Cornelia Huck]

On Mon, 18 Sep 2017 10:58:47 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 09/18/2017 10:55 AM, 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: We can
> > tweak s390_has_feat() to always indicate the zpci facility bit
> > when no cpu model is available (on 2.7 and previous compat machines).  
> 
> Looks better now. Thanks
> > 
> > Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > v2->v3:
> > - no longer RFC (I tested a bit more)
> > - removed unrelated hunk
> > - more verbose patch description
> > 
> > I'll wait a bit for more acks/reviews and will probably send a pull
> > request for s390x tomorrow or so before the amount of queued patches
> > gets out of hand...  
> 
> I have a small amount of patches pending, but then I got sick last week
> so I still have to sort out things. Lets do the pull request and we can
> then queue the other patches after they have received proper review.

Sounds like a plan.

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

[David Hildenbrand]

On 18.09.2017 10:55, 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: We can
> tweak s390_has_feat() to always indicate the zpci facility bit
> when no cpu model is available (on 2.7 and previous compat machines).
> 
> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v2->v3:
> - no longer RFC (I tested a bit more)
> - removed unrelated hunk
> - more verbose patch description
> 
> I'll wait a bit for more acks/reviews and will probably send a pull
> request for s390x tomorrow or so before the amount of queued patches
> gets out of hand...
> 
> ---
>  target/s390x/cpu_models.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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;
> +        }
>          return 0;
>      }
>      return test_bit(feat, cpu->model->features);
> 

1. cpu->model will always be set for QEMU, so you can move that into the
ifdef, next do the other checks. You can even send a cleanup to remove
the if (kvm_enabled()) check.

2. "return pci_available" ?

-- 

Thanks,

David

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

[Cornelia Huck]

On Mon, 18 Sep 2017 14:03:20 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 18.09.2017 10:55, 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: We can
> > tweak s390_has_feat() to always indicate the zpci facility bit
> > when no cpu model is available (on 2.7 and previous compat machines).
> > 
> > Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > v2->v3:
> > - no longer RFC (I tested a bit more)
> > - removed unrelated hunk
> > - more verbose patch description
> > 
> > I'll wait a bit for more acks/reviews and will probably send a pull
> > request for s390x tomorrow or so before the amount of queued patches
> > gets out of hand...
> > 
> > ---
> >  target/s390x/cpu_models.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > 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;
> > +        }
> >          return 0;
> >      }
> >      return test_bit(feat, cpu->model->features);
> >   
> 
> 1. cpu->model will always be set for QEMU, so you can move that into the
> ifdef, next do the other checks. You can even send a cleanup to remove
> the if (kvm_enabled()) check.

I prefer it the way it is now. There's nothing kvm specific about that
bit, and cpu->model always being set is not really obvious.

> 
> 2. "return pci_available" ?

I thought about that as well. I think, however, that a better way is
just to disable compat machines that rely on pci being available.

As you can turn off pci only manually, I'd like to defer this and first
get this out of the door, as it fixes an existing problem.

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

[David Hildenbrand]

On 18.09.2017 14:11, Cornelia Huck wrote:
> On Mon, 18 Sep 2017 14:03:20 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 18.09.2017 10:55, 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: We can
>>> tweak s390_has_feat() to always indicate the zpci facility bit
>>> when no cpu model is available (on 2.7 and previous compat machines).
>>>
>>> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> v2->v3:
>>> - no longer RFC (I tested a bit more)
>>> - removed unrelated hunk
>>> - more verbose patch description
>>>
>>> I'll wait a bit for more acks/reviews and will probably send a pull
>>> request for s390x tomorrow or so before the amount of queued patches
>>> gets out of hand...
>>>
>>> ---
>>>  target/s390x/cpu_models.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> 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;
>>> +        }
>>>          return 0;
>>>      }
>>>      return test_bit(feat, cpu->model->features);
>>>   
>>
>> 1. cpu->model will always be set for QEMU, so you can move that into the
>> ifdef, next do the other checks. You can even send a cleanup to remove
>> the if (kvm_enabled()) check.
> 
> I prefer it the way it is now. There's nothing kvm specific about that
> bit, and cpu->model always being set is not really obvious.
> 

I's already kvm specific as this can never happen with TCG :)

But whatever you prefer. This is good enough to fix the problem.


-- 

Thanks,

David

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

[Cornelia Huck]

On Mon, 18 Sep 2017 14:13:07 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 18.09.2017 14:11, Cornelia Huck wrote:
> > On Mon, 18 Sep 2017 14:03:20 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 18.09.2017 10:55, 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: We can
> >>> tweak s390_has_feat() to always indicate the zpci facility bit
> >>> when no cpu model is available (on 2.7 and previous compat machines).
> >>>
> >>> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> >>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>
> >>> v2->v3:
> >>> - no longer RFC (I tested a bit more)
> >>> - removed unrelated hunk
> >>> - more verbose patch description
> >>>
> >>> I'll wait a bit for more acks/reviews and will probably send a pull
> >>> request for s390x tomorrow or so before the amount of queued patches
> >>> gets out of hand...
> >>>
> >>> ---
> >>>  target/s390x/cpu_models.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> 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;
> >>> +        }
> >>>          return 0;
> >>>      }
> >>>      return test_bit(feat, cpu->model->features);
> >>>     
> >>
> >> 1. cpu->model will always be set for QEMU, so you can move that into the
> >> ifdef, next do the other checks. You can even send a cleanup to remove
> >> the if (kvm_enabled()) check.  
> > 
> > I prefer it the way it is now. There's nothing kvm specific about that
> > bit, and cpu->model always being set is not really obvious.
> >   
> 
> I's already kvm specific as this can never happen with TCG :)
> 
> But whatever you prefer. This is good enough to fix the problem.

Sure, we can revisit that one later. I plan to send a pull request
tomorrow.