s390x/ccw: create s390 phb for compat reasons as well

PATCH In review Cornelia Huck 3 participants 7 messages Sep 18, 2017

Share & tools

Cornelia Huck

author

2017-09-18 08:55 UTC #0 lore raw

PATCH v3 1/1 s390x/ccw: create s390 phb for compat reasons as well

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 <[email protected]>

Signed-off-by Cornelia Huck <[email protected]>

---

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(+)

target/s390x/cpu_models.c +3 -0

index c295e641e6..5169379db5 100644
@@ -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);

Signature · 1 line

2.13.5

Christian Borntraeger

2017-09-18 08:58 UTC #1 lore raw

Quoted 8 lines · 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).

Looks better now. Thanks

Quoted 6 lines

Fixes d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
Acked-by Christian Borntraeger <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---

Quoted 8 lines

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.

Quoted 20 lines

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

target/s390x/cpu_models.c +3 -0

index c295e641e6..5169379db5 100644
@@ -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);

Cornelia Huck

author

2017-09-18 09:07 UTC #2 lore raw

Quoted 22 lines · On Mon, 18 Sep 2017 10:58:47 +0200 Christian Borntraeger…

Quoted 8 lines · 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).
Looks better now. Thanks
Quoted 11 lines
Fixes d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
Acked-by Christian Borntraeger <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
v2->v3:
- no longer RFC (I tested a bit more)
- removed unrelated hunk
- more verbose patch description

Quoted 7 lines

Quoted 3 lines
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.

David Hildenbrand

2017-09-18 12:03 UTC #3 lore raw

In reply to Cornelia Huck

Quoted 33 lines · 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 <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
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(+)
target/s390x/cpu_models.c +0 -0
index c295e641e6..5169379db5 100644

target/s390x/cpu_models.c bool s390_has_feat(S390Feat feat) · line 204

Quoted 9 lines

target/s390x/cpu_models.c +3 -0

@@ -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" ?

Signature · 3 lines

Thanks,

David

Cornelia Huck

author

2017-09-18 12:11 UTC #4 lore raw

Quoted 39 lines · On Mon, 18 Sep 2017 14:03:20 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 38 lines · 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 <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
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(+)
target/s390x/cpu_models.c +3 -0
index c295e641e6..5169379db5 100644
@@ -196,6 +196,9 @@ bool s390_has_feat(S390Feat feat)
             }
         }
 #endif
+        if (feat == S390_FEAT_ZPCI) {
+            return true;
+        }

Quoted 8 lines

target/s390x/cpu_models.c bool s390_has_feat(S390Feat feat) · line 204
Quoted 4 lines
target/s390x/cpu_models.c +0 -0
         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.

Quoted 2 lines

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.

David Hildenbrand

2017-09-18 12:13 UTC #5 lore raw

Quoted 51 lines · Cornelia Huck

Quoted 39 lines · On Mon, 18 Sep 2017 14:03:20 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 38 lines · 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 <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
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(+)
target/s390x/cpu_models.c +3 -0
index c295e641e6..5169379db5 100644
@@ -196,6 +196,9 @@ bool s390_has_feat(S390Feat feat)
             }
         }
 #endif
+        if (feat == S390_FEAT_ZPCI) {
+            return true;
+        }
Quoted 8 lines
target/s390x/cpu_models.c bool s390_has_feat(S390Feat feat) · line 204
Quoted 4 lines
target/s390x/cpu_models.c +0 -0
         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.

Quoted 3 lines

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.

Signature · 3 lines

Thanks,

David

Cornelia Huck

author

2017-09-18 12:16 UTC #6 lore raw

Quoted 52 lines · On Mon, 18 Sep 2017 14:13:07 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 51 lines · Cornelia Huck
Quoted 47 lines · On Mon, 18 Sep 2017 14:03:20 +0200 David Hildenbrand <[email protected]> wrote:
Quoted 33 lines · 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 <[email protected]>
Signed-off-by Cornelia Huck <[email protected]>
---
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(+)
target/s390x/cpu_models.c +0 -0
index c295e641e6..5169379db5 100644
target/s390x/cpu_models.c bool s390_has_feat(S390Feat feat) · line 204
Quoted 9 lines
target/s390x/cpu_models.c +3 -0
@@ -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.

Quoted 7 lines

Quoted 3 lines
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.