Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 26/03/21 11:57, Markus Armbruster wrote:
>>>         'rng-random':                 'RngRandomProperties',
>>>         'secret':                     'SecretProperties',
>>>         'secret_keyring':             'SecretKeyringProperties',
>>> -      'sev-guest':                  { 'type': 'SevGuestProperties',
>>> -                                      'if': 'defined(CONFIG_SEV)' },
>>> +      'sev-guest':                  'SevGuestProperties',
>>>         'throttle-group':             'ThrottleGroupProperties',
>>>         'tls-creds-anon':             'TlsCredsAnonProperties',
>>>         'tls-creds-psk':              'TlsCredsPskProperties',
>> No branch for tag value 'pef-guest', i.e. no tag-specific members.
>> There are two more: can_bus, s390_pv_guest.  I assume this is
>> intentional.
>
> Yes, they have no properties.
>
>> Links a bit of dead code into the other qemu-system-FOO, but that's
>> okay.
>> If we genuinely needed (or wanted) target-dependent -object, we'd
>> split
>> qom-target.json off qom.json, and put the target-dependent parts there,
>> including the enum and the union, with the obvious ripple effects.  Not
>> now, maybe not ever.
>> Would adding "only for CONFIG_MUMBLE" to the doc comments make
>> sense?
>> It's what we did before we had 'if'.
>
> In this specific case we had not documentation at all for objects.

... until Kevin added some when he QAPIfied.  Looks like this (copied
from qemu-qmp-ref.7)[*]:

   SevGuestProperties (Object)
       Properties for sev-guest objects.

   Members
       sev-device: string (optional)
              SEV device to use (default: "/dev/sev")

       dh-cert-file: string (optional)
              guest owners DH certificate (encoded with base64)

       session-file: string (optional)
              guest owners session parameters (encoded with base64)

       policy: int (optional)
              SEV policy value (default: 0x1)

       handle: int (optional)
              SEV firmware handle (default: 0)

       cbitpos: int (optional)
              C-bit location in page table entry (default: 0)

       reduced-phys-bits: int
              number  of  bits  in  physical addresses that become unavailable
              when SEV is enabled

   Since
       2.12

   If
       defined(CONFIG_SEV)

Your patch drops the last three lines, without a replacement.

>                                                                     We
> can add the information on relevant targets in the documentation for
> the *Properties types.

Yes, please.

Preferably with that squashed in:
Reviewed-by: Markus Armbruster <armbru@redhat.com>


[*] I lied.  It actually looks like

   If
       defined(CONFIG_SEV).SS ObjectType (Enum)

The running together of "defined(CONFIG_SEV)" and ".SS ObjectType
(Enum)" is a bug.  I'll investigate.