Re: [PATCH qemu v2 1/3] hw/s390x: add CPI identifiers to QOM

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Mar 06, 2025 at 04:44:33PM +0100, Nina Schoetterl-Glausch wrote:
> On Thu, 2025-03-06 at 15:55 +0100, Thomas Huth wrote:
> > On 06/03/2025 13.23, shalini wrote:
> > > On 2025-03-05 16:56, Thomas Huth wrote:
> > > > On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
> > > > > Add Control-Program Identification (CPI) to the QEMU Object
> > > > > Model (QOM). The CPI identifiers provide information about
> > > > > the guest operating system. The CPI identifiers are:
> > > > > system type, system name, system level and sysplex name.
> > > > > 
> > > > > The system type provides the OS type of the guest (e.g. LINUX).
> > > > > The system name provides the name of the guest (e.g. TESTVM).
> > > > > The system level provides the distribution and kernel version
> > > > > of the guest OS (e.g. 0x50e00).
> > > > > The sysplex name provides the sysplex name of the guest
> > > > > (e.g. SYSPLEX).
> > > > > 
> > > > > Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> > > > > ---
> > > > >   hw/s390x/s390-virtio-ccw.c         | 29 +++++++++++++++++++++++++++++
> > > > >   include/hw/s390x/s390-virtio-ccw.h |  8 ++++++++
> > > > >   qapi/machine.json                  | 24 ++++++++++++++++++++++++
> > > > >   3 files changed, 61 insertions(+)
> > > > > 
> > > > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > > > index 51ae0c133d..13ea8db1b0 100644
> > > > > --- a/hw/s390x/s390-virtio-ccw.c
> > > > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > > > @@ -50,6 +50,7 @@
> 
> [...]
> 
> > > > > +##
> > > > > +{ 'struct': 'S390ControlProgramId', 'data': {
> > > > > +     'system-type': 'str',
> > > > > +     'system-name': 'str',
> > > > > +     'system-level': 'str',
> > > > 
> > > > Not sure, but would it make sense to use a number for the system-level
> > > > instead? At least it's a number in ControlProgramId, not a string.
> > > > 
> > > 
> > > The system-level, when interpreted as an int provides the output below
> > > 
> > > 'system-level': 74872343805430528
> > > 
> > > But the desired output below is obtained only when interpreted as a str. 
> > > please refer https://www.ibm.com/docs/en/linux-on-systems? 
> > > topic=identification-system-level for details on system-level. I will also 
> > > document this in the description of system-level as suggested by Daniel. 
> > > Thank you.
> > > 
> > > 'system-level': '0x10a000000060b00'
> > 
> > Well, the idea of QOM/QAPI is: It's an *API* for machines, not meant for 
> > direct consumption by the end user. So passing an integer as a string is not 
> > the right way here. For the user, you'd require some upper level instead 
> > that renders the integer in the right shape for the user. So please don't 
> > use a string for this at the QOM/QAPI level.
> 
> In a sense the system level is a bitfield.
> So this could become a struct
> 
> {
> 	'hypervisor-use' : true,
> 	'distribution-id': 3,	// or an enum?
> 	'distribution-version-major: 24,
> 	...
> }

Yes, if this is a mandatory format, breaking out the fields makes sense.

> Not sure how to handle the 3 undefined bits in the highest byte.

If they're always left zero, might as well just omit them until
such time as they have semantics defined (if ever)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|