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

[Thomas Huth <thuth@redhat.com>]

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