[Qemu-devel] [PATCH RFC 0/1] s390x: pci compat handling

[Qemu-devel] [PATCH RFC 0/1] s390x: pci compat handling

[Cornelia Huck]

While playing around with compat machines a bit, I noticed that my
zpci detanglement patches broke migration from 2.7 to current master
(2.8 or newer are fine, which is why I did not notice that before.)

qemu 2.7 seems to create a savevm that a s390-next (or master) build
without the s390 phb chokes on:

qemu-system-s390x: Unknown savevm section or instance 'PCIBUS' 0

Creating the s390 phb for compat machines seems to cure this; still
RFC for the following reasons:

- I'm not sure what we're supposed to do on builds without pci. Fail
  creating the compat machines? Do we need a new set of _NOPCI compat
  machines for that?
- I don't understand why 2.7 fails, but 2.8 and later are fine. At
  least, I was not able to spot which commit changed the behaviour
  here... and I really want to understand this. Pointers welcome.
- I have not tested it extensively yet.

Patch is against s390-next.

Cornelia Huck (1):
  s390x/ccw: create s390 phb for compat reasons as well

 hw/s390x/s390-virtio-ccw.c         | 6 +++++-
 include/hw/s390x/s390-virtio-ccw.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.13.5

[Qemu-devel] [PATCH RFC 1/1] 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
some old machines.

Create the s390 phb if we need it for compat reasons, even if
we don't provide the zpci facility.

Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c         | 6 +++++-
 include/hw/s390x/s390-virtio-ccw.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 0471407187..7e3148eb4a 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;
@@ -288,7 +290,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()->pci_compat) {
         DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
         object_property_add_child(qdev_get_machine(),
                                   TYPE_S390_PCI_HOST_BRIDGE,
@@ -429,6 +431,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->pci_compat = false;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->hot_add_cpu = s390_hot_add_cpu;
@@ -784,6 +787,7 @@ static void ccw_machine_2_7_class_options(MachineClass *mc)
     S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
 
     s390mc->cpu_model_allowed = false;
+    s390mc->pci_compat = pci_available;
     ccw_machine_2_8_class_options(mc);
     SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_7);
 }
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index a9a90c2022..9978c89e90 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 pci_compat;
 } S390CcwMachineClass;
 
 /* runtime-instrumentation allowed by the machine */
-- 
2.13.5

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

[David Hildenbrand]

On 15.09.2017 12:14, 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
> some old machines.
> 
> Create the s390 phb if we need it for compat reasons, even if
> we don't provide the zpci facility.
> 
> Fixes: d32bd032d8 ("s390x/ccw: create s390 phb conditionally")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c         | 6 +++++-
>  include/hw/s390x/s390-virtio-ccw.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 0471407187..7e3148eb4a 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;
> @@ -288,7 +290,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()->pci_compat) {
>          DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
>          object_property_add_child(qdev_get_machine(),
>                                    TYPE_S390_PCI_HOST_BRIDGE,
> @@ -429,6 +431,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->pci_compat = false;
>      mc->init = ccw_init;
>      mc->reset = s390_machine_reset;
>      mc->hot_add_cpu = s390_hot_add_cpu;
> @@ -784,6 +787,7 @@ static void ccw_machine_2_7_class_options(MachineClass *mc)
>      S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
>  
>      s390mc->cpu_model_allowed = false;
> +    s390mc->pci_compat = pci_available;
>      ccw_machine_2_8_class_options(mc);
>      SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_7);
>  }
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index a9a90c2022..9978c89e90 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 pci_compat;
>  } S390CcwMachineClass;
>  
>  /* runtime-instrumentation allowed by the machine */
> 

As an alternative, simply

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 0f28ebd162..0f22efc3b6 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -195,6 +195,8 @@ bool s390_has_feat(S390Feat feat)
                 return true;
             }
         }
+        if (feat == S390_FEAT_ZPCI)
+            return true;
 #endif
         return 0;
     }


(this is also the way we handle other features without cpu model support
-especially if cpu model support is disabled for older machines)

-- 

Thanks,

David

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

[Cornelia Huck]

On Fri, 15 Sep 2017 13:57:40 +0200
David Hildenbrand <david@redhat.com> wrote:

> As an alternative, simply
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 0f28ebd162..0f22efc3b6 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -195,6 +195,8 @@ bool s390_has_feat(S390Feat feat)
>                  return true;
>              }
>          }
> +        if (feat == S390_FEAT_ZPCI)
> +            return true;

Move that out of the CONFIG_KVM #ifdef?

(Also, we still have the issue with pci support :/ - depend on
pci_available?)

>  #endif
>          return 0;
>      }
> 
> 
> (this is also the way we handle other features without cpu model support
> -especially if cpu model support is disabled for older machines)

Now that you mention cpu models (which are disabled on old machines),
the failure starts to look plausible :)

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

[David Hildenbrand]

On 15.09.2017 15:07, Cornelia Huck wrote:
> On Fri, 15 Sep 2017 13:57:40 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> As an alternative, simply
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 0f28ebd162..0f22efc3b6 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -195,6 +195,8 @@ bool s390_has_feat(S390Feat feat)
>>                  return true;
>>              }
>>          }
>> +        if (feat == S390_FEAT_ZPCI)
>> +            return true;
> 
> Move that out of the CONFIG_KVM #ifdef?
> 

Should not make a difference I think, cpu models are not shielded off
for TCG I think (maybe we should fix that, but doesn't look that easy -
they have no host model which would allow cpu->model = NULL).

> (Also, we still have the issue with pci support :/ - depend on
> pci_available?)

hm ....

> 
>>  #endif
>>          return 0;
>>      }
>>
>>
>> (this is also the way we handle other features without cpu model support
>> -especially if cpu model support is disabled for older machines)
> 
> Now that you mention cpu models (which are disabled on old machines),
> the failure starts to look plausible :)
> 


-- 

Thanks,

David

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

[Cornelia Huck]

On Fri, 15 Sep 2017 15:09:02 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 15.09.2017 15:07, Cornelia Huck wrote:
> > On Fri, 15 Sep 2017 13:57:40 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> As an alternative, simply
> >>
> >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> >> index 0f28ebd162..0f22efc3b6 100644
> >> --- a/target/s390x/cpu_models.c
> >> +++ b/target/s390x/cpu_models.c
> >> @@ -195,6 +195,8 @@ bool s390_has_feat(S390Feat feat)
> >>                  return true;
> >>              }
> >>          }
> >> +        if (feat == S390_FEAT_ZPCI)
> >> +            return true;  
> > 
> > Move that out of the CONFIG_KVM #ifdef?
> >   
> 
> Should not make a difference I think, cpu models are not shielded off
> for TCG I think (maybe we should fix that, but doesn't look that easy -
> they have no host model which would allow cpu->model = NULL).

It just looks odd to have it inside the define...

> 
> > (Also, we still have the issue with pci support :/ - depend on
> > pci_available?)  
> 
> hm ....

Should we maybe disallow compat machines if CONFIG_PCI is off, and
think about something for the 2.11 machine? Otherwise, I cannot see
that end well :/

But I'll just draw up a v2 for now.

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

[David Hildenbrand]

On 15.09.2017 15:22, Cornelia Huck wrote:
> On Fri, 15 Sep 2017 15:09:02 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 15.09.2017 15:07, Cornelia Huck wrote:
>>> On Fri, 15 Sep 2017 13:57:40 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> As an alternative, simply
>>>>
>>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>>> index 0f28ebd162..0f22efc3b6 100644
>>>> --- a/target/s390x/cpu_models.c
>>>> +++ b/target/s390x/cpu_models.c
>>>> @@ -195,6 +195,8 @@ bool s390_has_feat(S390Feat feat)
>>>>                  return true;
>>>>              }
>>>>          }
>>>> +        if (feat == S390_FEAT_ZPCI)
>>>> +            return true;  
>>>
>>> Move that out of the CONFIG_KVM #ifdef?
>>>   
>>
>> Should not make a difference I think, cpu models are not shielded off
>> for TCG I think (maybe we should fix that, but doesn't look that easy -
>> they have no host model which would allow cpu->model = NULL).
> 
> It just looks odd to have it inside the define...

sure, you can then also just move it into kvm_enable() next to the other
ones.

> 
>>
>>> (Also, we still have the issue with pci support :/ - depend on
>>> pci_available?)  
>>
>> hm ....
> 
> Should we maybe disallow compat machines if CONFIG_PCI is off, and
> think about something for the 2.11 machine? Otherwise, I cannot see
> that end well :/

If they required PCI, than that is the only way to do it.

> 
> But I'll just draw up a v2 for now.
> 


-- 

Thanks,

David

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

[Halil Pasic]

On 09/15/2017 03:37 PM, David Hildenbrand wrote:
> On 15.09.2017 15:22, Cornelia Huck wrote:
>> On Fri, 15 Sep 2017 15:09:02 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 15.09.2017 15:07, Cornelia Huck wrote:
>>>> On Fri, 15 Sep 2017 13:57:40 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>   
>>>>> As an alternative, simply
>>>>>
>>>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>>>> index 0f28ebd162..0f22efc3b6 100644
>>>>> --- a/target/s390x/cpu_models.c
>>>>> +++ b/target/s390x/cpu_models.c
>>>>> @@ -195,6 +195,8 @@ bool s390_has_feat(S390Feat feat)
>>>>>                  return true;
>>>>>              }
>>>>>          }
>>>>> +        if (feat == S390_FEAT_ZPCI)
>>>>> +            return true;  
>>>>
>>>> Move that out of the CONFIG_KVM #ifdef?
>>>>   
>>>
>>> Should not make a difference I think, cpu models are not shielded off
>>> for TCG I think (maybe we should fix that, but doesn't look that easy -
>>> they have no host model which would allow cpu->model = NULL).
>>
>> It just looks odd to have it inside the define...
> 
> sure, you can then also just move it into kvm_enable() next to the other
> ones.
> 
>>
>>>
>>>> (Also, we still have the issue with pci support :/ - depend on
>>>> pci_available?)  
>>>
>>> hm ....
>>
>> Should we maybe disallow compat machines if CONFIG_PCI is off, and
>> think about something for the 2.11 machine? Otherwise, I cannot see
>> that end well :/
> 
> If they required PCI, than that is the only way to do it.
> 
>>
>> But I'll just draw up a v2 for now.
>>

Sorry for not contributing anything here. I don't have the whole
thing in my head, so I can't quite follow without digging into this.

I don't think I will be able to join today -- maybe I could have a
look next week .

Halil 

> 
>