Re: [PATCH v11 01/11] s390x: Register TYPE_S390_CCW_MACHINE properties as class properties

[Pierre Morel <pmorel@linux.ibm.com>]

On 11/6/22 12:37, Thomas Huth wrote:
> On 04/11/2022 15.57, Pierre Morel wrote:
>>
>>
>> On 11/4/22 15:29, Thomas Huth wrote:
>>> On 04/11/2022 11.53, Cédric Le Goater wrote:
>>>> On 11/4/22 11:16, Pierre Morel wrote:
>>>>>
>>>>>
>>>>> On 11/4/22 07:32, Thomas Huth wrote:
>>>>>> On 03/11/2022 18.01, Pierre Morel wrote:
>>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>>> ---
>>>>>>>   hw/s390x/s390-virtio-ccw.c | 127 
>>>>>>> +++++++++++++++++++++----------------
>>>>>>>   1 file changed, 72 insertions(+), 55 deletions(-)
>>>>>>
>>>>>> -EMISSINGPATCHDESCRIPTION
>>>>>>
>>>>>> ... please add some words *why* this is a good idea / necessary.
>>>>>
>>>>> I saw that the i386 patch had no description for the same patch so...
>>>>>
>>>>> To be honest I do not know why it is necessary.
>>>>> The only reason I see is to be in sync with the PC implementation.
>>>>>
>>>>> So what about:
>>>>> "
>>>>> Register TYPE_S390_CCW_MACHINE properties as class properties
>>>>> to be conform with the X architectures
>>>>> "
>>>>> ?
>>>>>
>>>>> @Cédric , any official recommendation for doing that?
>>>>
>>>> There was a bunch of commits related to QOM in this series :
>>>>
>>>>    91def7b83 arm/virt: Register most properties as class properties
>>>>    f5730c69f0 i386: Register feature bit properties as class properties
>>>>
>>>> which moved property definitions at the class level.
>>>>
>>>> Then,
>>>>
>>>>    commit d8fb7d0969 ("vl: switch -M parsing to keyval")
>>>>
>>>> changed machine_help_func() to use a machine class and not machine
>>>> instance anymore.
>>>>
>>>> I would use the same kind of commit log and add a Fixes tag to get it
>>>> merged in 7.2
>>>
>>> Ah, so this fixes the problem that running QEMU with " -M 
>>> s390-ccw-virtio,help" does not show the s390x-specific properties 
>>> anymore? ... that's certainly somethings that should be mentioned in 
>>> the commit message! What about something like this:
>>>
>>> "Currently, when running 'qemu-system-s390x -M -M 
>>> s390-ccw-virtio,help' the s390x-specific properties are not listed 
>>> anymore. This happens because since commit d8fb7d0969 ("vl: switch -M 
>>> parsing to keyval") the properties have to be defined at the class 
>>> level and not at the instance level anymore. Fix it on s390x now, 
>>> too, by moving the registration of the properties to the class level"
>>>
>>> Fixes: d8fb7d0969 ("vl: switch -M parsing to keyval")
>>>
>>> ?
>>>
>>>   Thomas
>>>
>>
>> That seems really good :)
> 
> All right, I've queued this patch (with the updated commit description) 
> and the next one on my s390x-branch for QEMU 7.2:
> 
>   https://gitlab.com/thuth/qemu/-/commits/s390x-next/
> 
>   Thomas
> 
> 

Thank you!

Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen