Re: [PATCH v4 4/4] hw/s390x: compat handling for backward migration

[Shalini Chellathurai Saroja <shalini@linux.ibm.com>]

On 2025-04-28 14:05, Nina Schoetterl-Glausch wrote:
> On Thu, 2025-04-10 at 17:09 +0200, Shalini Chellathurai Saroja wrote:
>> Add Control-Program Identification (CPI) device to QOM only when the 
>> virtual
>> machine supports CPI. CPI is supported from "s390-ccw-virtio-10.0" 
>> machine
>> and higher.
>> 
>> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c         | 10 +++++++++-
>>  include/hw/s390x/s390-virtio-ccw.h |  1 +
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 7f28cbd1de..81832ee638 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -274,6 +274,7 @@ static void s390_create_sclpcpi(SCLPDevice *sclp)
>>  static void ccw_init(MachineState *machine)
>>  {
>>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>>      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>>      int ret;
>>      VirtualCssBus *css_bus;
>> @@ -336,7 +337,10 @@ static void ccw_init(MachineState *machine)
>>      s390_init_tod();
>> 
>>      /* init SCLP event Control-Program Identification */
>> -    s390_create_sclpcpi(ms->sclp);
>> +    if (s390mc->use_cpi) {
>> +        s390_create_sclpcpi(ms->sclp);
>> +    }
> 
> Fixing this in a separate commit could be bad for bisecting.

Ok.

> You introduce use_cpi in an earlier commit set to false and
> then flipping it in the migration patch for new machines.
> This way there is no broken intermediate state.
> 
> I would also squash the compat migration changes into the previous
> patch.
> 

Hello Nina,

If use_cpi is set to false in patch 1, then the sclpcpi device will not 
be instantiated even for new machines at that point. The sclpcpi device 
will only be instantiated when the use_cpi is set to true in the 
migration patch.

I prefer to squash this entire patch to patch 1, then the sclpcpi device 
will only be instantiated for new machines with the code in patch 1 
itself and will not be dependent on the migration patch. I like this 
approach as the logic to add sclpcpi device is complete in patch 1.

What do you think?, thank you.

>>  }
>> 
>>  static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>> @@ -827,6 +831,7 @@ static void ccw_machine_class_init(ObjectClass 
>> *oc, void *data)
>> 
>>      s390mc->hpage_1m_allowed = true;
>>      s390mc->max_threads = 1;
>> +    s390mc->use_cpi = true;
>>      mc->reset = s390_machine_reset;
>>      mc->block_default_type = IF_VIRTIO;
>>      mc->no_cdrom = 1;
>> @@ -955,6 +960,9 @@ static void 
>> ccw_machine_9_2_class_options(MachineClass *mc)
>>          { TYPE_S390_PCI_DEVICE, "relaxed-translation", "off", },
>>      };
>> 
>> +    S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc);
>> +    s390mc->use_cpi = false;
>> +
>>      ccw_machine_10_0_class_options(mc);
>>      compat_props_add(mc->compat_props, hw_compat_9_2, 
>> hw_compat_9_2_len);
>>      compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
>> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
>> b/include/hw/s390x/s390-virtio-ccw.h
>> index 686d9497d2..fc4112fbf5 100644
>> --- a/include/hw/s390x/s390-virtio-ccw.h
>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>> @@ -55,6 +55,7 @@ struct S390CcwMachineClass {
>>      /*< public >*/
>>      bool hpage_1m_allowed;
>>      int max_threads;
>> +    bool use_cpi;
>>  };
>> 
>>  /* 1M huge page mappings allowed by the machine */

-- 
Mit freundlichen Grüßen / Kind regards
Shalini Chellathurai Saroja
Software Developer
Linux on IBM Z & KVM Development
IBM Deutschland Research & Development GmbH
Dept 1419, Schoenaicher Str. 220, 71032 Boeblingen
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht 
Stuttgart, HRB 243294