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

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

On 2025-04-09 07:49, Thomas Huth wrote:
> On 03/04/2025 14.49, Shalini Chellathurai Saroja wrote:
>> On 2025-04-02 09:52, Thomas Huth wrote:
>>> On 31/03/2025 16.00, 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/event-facility.c  | 27 ++++++++++++++++++++++-----
>>>>   hw/s390x/s390-virtio-ccw.c |  1 +
>>>>   2 files changed, 23 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>>>> index c0fb6e098c..cb23bbc54b 100644
>>>> --- a/hw/s390x/event-facility.c
>>>> +++ b/hw/s390x/event-facility.c
>>>> @@ -22,6 +22,7 @@
>>>>   #include "hw/s390x/sclp.h"
>>>>   #include "migration/vmstate.h"
>>>>   #include "hw/s390x/event-facility.h"
>>>> +#include "hw/qdev-properties.h"
>>>>     typedef struct SCLPEventsBus {
>>>>       BusState qbus;
>>>> @@ -54,6 +55,7 @@ struct SCLPEventFacility {
>>>>       bool allow_all_mask_sizes;
>>>>       /* length of the receive mask */
>>>>       uint16_t mask_length;
>>>> +    bool use_cpi;
>>>>   };
>>>>     /* return true if any child has event pending set */
>>>> @@ -455,11 +457,20 @@ static void realize_event_facility(DeviceState 
>>>> *dev, Error **errp)
>>>>           qdev_unrealize(DEVICE(&event_facility->quiesce));
>>>>           return;
>>>>       }
>>>> -    if (!qdev_realize(DEVICE(&event_facility->cpi),
>>>> -                      BUS(&event_facility->sbus), errp)) {
>>>> -        qdev_unrealize(DEVICE(&event_facility->quiesce));
>>>> -        qdev_unrealize(DEVICE(&event_facility->cpu_hotplug));
>>>> -        return;
>>>> +    /*
>>>> +     * Add sclpcpi device to QOM only when the virtual machine 
>>>> supports
>>>> +     * Control-Program Identification. It is supported by 
>>>> "s390-ccw- virtio-10.0"
>>>> +     * machine and higher.
>>>> +     */
>>>> +    if (!event_facility->use_cpi) {
>>>> +        object_unparent(OBJECT(&event_facility->cpi));
>>>> +    } else {
>>>> +        if (!qdev_realize(DEVICE(&event_facility->cpi),
>>>> +                          BUS(&event_facility->sbus), errp)) {
>>>> +            qdev_unrealize(DEVICE(&event_facility->quiesce));
>>>> +            qdev_unrealize(DEVICE(&event_facility->cpu_hotplug));
>>>> +            return;
>>>> +        }
>>> 
>>> Hmm, first doing object_initialize_child() in init_event_facility()
>>> and then unparenting it here again in case we are running with an
>>> older machine type is a little bit ugly. I wonder whether it would be
>>> nicer to add the QOM object from ccw_init() init instead, similar to
>>> what we do with the SCLP-console in s390_create_sclpconsole() ? If
>>> you've got some spare minutes, could you please give it a try whether
>>> that looks nicer?
>>> 
>> 
>> Hello Thomas,
>> 
>> Sure. Did you mean like the code below?, if yes, the use_cpi is always 
>> true when adding the sclpcpi device from ccw_init(). The use_cpi is 
>> set to false at a later point, when the machine type is 9.2 or older. 
>> This means the sclpcpi device is always added, the output and the code 
>> are provided below. Please let me know how to proceed, thank you very 
>> much.
> ...
>> @@ -499,6 +485,7 @@ static void init_event_facility_class(ObjectClass 
>> *klass, void *data)
>>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>       k->command_handler = command_handler;
>>       k->event_pending = event_pending;
>> +    k->use_cpi = use_cpi;
>>   }
> ...
>> diff --git a/include/hw/s390x/event-facility.h 
>> b/include/hw/s390x/event- facility.h
>> index f445d2f9f5..ba20161023 100644
>> --- a/include/hw/s390x/event-facility.h
>> +++ b/include/hw/s390x/event-facility.h
>> @@ -204,6 +204,7 @@ struct SCLPEventFacilityClass {
>>       SysBusDeviceClass parent_class;
>>       void (*command_handler)(SCLPEventFacility *ef, SCCB *sccb, 
>> uint64_t code);
>>       bool (*event_pending)(SCLPEventFacility *ef);
>> +    bool (*use_cpi)(SCLPEventFacility *ef);
>>   };
> 
>  Hi,
> 
> you certainly don't need the (*use_cpi) callback here.
> 
> I'd suggest to:
> 
> 1) Add a boolean flag to S390CcwMachineClass in s390-virtio-ccw.h
> called "use_cpi", "cpi_allowed", "has_cpi" or whatever.
> 
> 2) Set that flag to true in ccw_machine_class_init() (similar to that
> hpage_1m_allowed flag)
> 
> 3) Set that flag to false in ccw_machine_9_2_class_options() so that
> it gets disabled for older machine type classes. Important: use the
> class_options() function here, not the instance_options()! Also not
> that this should go into the ccw_machine_10_0_class_options()
> functions instead once v10.0 has been released.
> 
> 4) In ccw_init() you should now be able to use "S390CcwMachineClass
> *s390mc = S390_CCW_MACHINE_CLASS(mc)" to query the flag from the
> machine class.
> 
>  HTH,
>   Thomas

Hi Thomas,

I will do this, Thank you.

-- 
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