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

[Thomas Huth]

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