[PATCH v2 0/2] tpm: Return QMP error when TPM is disabled in build

[PATCH v2 0/2] tpm: Return QMP error when TPM is disabled in build

[Philippe Mathieu-Daudé]

Since v1:
- make the qapi schema conditional (Marc-André)

Philippe Mathieu-Daudé (2):
  qapi: Inline qmp_marshal_output() functions
  tpm: Return QMP error when TPM is disabled in build

 qapi/tpm.json            |  9 ++++++---
 monitor/hmp-cmds.c       |  4 ++++
 stubs/tpm.c              | 16 ----------------
 scripts/qapi/commands.py |  4 ++--
 4 files changed, 12 insertions(+), 21 deletions(-)

-- 
2.31.1

[RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Philippe Mathieu-Daudé]

In case we need to use QAPI types but no QAPI command / QAPI event
actually use them, the generated qmp_marshal_output() function will
trigger the compiler 'unused-function' warnings.
To prevent that, emit these functions inlined: the compiler will
ignore such unused functions.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC: No clue about QAPI...
Tested with GCC. If the compiler is picky we could use the 'unused'
function attribute.
---
 scripts/qapi/commands.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 0e13d510547..bbed776a909 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -91,8 +91,8 @@ def gen_call(name: str,
 def gen_marshal_output(ret_type: QAPISchemaType) -> str:
     return mcgen('''
 
-static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
-                                QObject **ret_out, Error **errp)
+static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
+                                        QObject **ret_out, Error **errp)
 {
     Visitor *v;
 
-- 
2.31.1

[PATCH v2 2/2] tpm: Return QMP error when TPM is disabled in build

[Philippe Mathieu-Daudé]

When the management layer queries a binary built using --disable-tpm
for TPM devices, it gets confused by getting empty responses:

  { "execute": "query-tpm" }
  {
      "return": [
      ]
  }
  { "execute": "query-tpm-types" }
  {
      "return": [
      ]
  }
  { "execute": "query-tpm-models" }
  {
      "return": [
      ]
  }

To make it clearer by returning an error:
- Make the TPM QAPI schema conditional
- Adapt the HMP command
- Remove stubs which became unnecessary

The management layer now gets a 'CommandNotFound' error:

  { "execute": "query-tpm" }
  {
      "error": {
          "class": "CommandNotFound",
          "desc": "The command query-tpm has not been found"
      }
  }

Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/tpm.json      |  9 ++++++---
 monitor/hmp-cmds.c |  4 ++++
 stubs/tpm.c        | 16 ----------------
 3 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/qapi/tpm.json b/qapi/tpm.json
index 6a10c9ed8d2..09332e6f996 100644
--- a/qapi/tpm.json
+++ b/qapi/tpm.json
@@ -33,7 +33,8 @@
 # <- { "return": [ "tpm-tis", "tpm-crb", "tpm-spapr" ] }
 #
 ##
-{ 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
+{ 'command': 'query-tpm-models', 'returns': ['TpmModel'],
+  'if': 'defined(CONFIG_TPM)' }
 
 ##
 # @TpmType:
@@ -63,7 +64,8 @@
 # <- { "return": [ "passthrough", "emulator" ] }
 #
 ##
-{ 'command': 'query-tpm-types', 'returns': ['TpmType'] }
+{ 'command': 'query-tpm-types', 'returns': ['TpmType'],
+  'if': 'defined(CONFIG_TPM)' }
 
 ##
 # @TPMPassthroughOptions:
@@ -152,4 +154,5 @@
 #    }
 #
 ##
-{ 'command': 'query-tpm', 'returns': ['TPMInfo'] }
+{ 'command': 'query-tpm', 'returns': ['TPMInfo'],
+  'if': 'defined(CONFIG_TPM)' }
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index d10ee141109..f6cadede40f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -901,6 +901,9 @@ void hmp_info_pci(Monitor *mon, const QDict *qdict)
 
 void hmp_info_tpm(Monitor *mon, const QDict *qdict)
 {
+#ifndef CONFIG_TPM
+    monitor_printf(mon, "TPM device not supported\n");
+#else
     TPMInfoList *info_list, *info;
     Error *err = NULL;
     unsigned int c = 0;
@@ -946,6 +949,7 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
         c++;
     }
     qapi_free_TPMInfoList(info_list);
+#endif /* CONFIG_TPM */
 }
 
 void hmp_quit(Monitor *mon, const QDict *qdict)
diff --git a/stubs/tpm.c b/stubs/tpm.c
index 9bded191d9d..b1dc6370a5e 100644
--- a/stubs/tpm.c
+++ b/stubs/tpm.c
@@ -6,7 +6,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qapi/qapi-commands-tpm.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
 
@@ -19,21 +18,6 @@ void tpm_cleanup(void)
 {
 }
 
-TPMInfoList *qmp_query_tpm(Error **errp)
-{
-    return NULL;
-}
-
-TpmTypeList *qmp_query_tpm_types(Error **errp)
-{
-    return NULL;
-}
-
-TpmModelList *qmp_query_tpm_models(Error **errp)
-{
-    return NULL;
-}
-
 void tpm_build_ppi_acpi(TPMIf *tpm, Aml *dev)
 {
 }
-- 
2.31.1

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Eric Blake]

On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote:
> In case we need to use QAPI types but no QAPI command / QAPI event
> actually use them, the generated qmp_marshal_output() function will
> trigger the compiler 'unused-function' warnings.
> To prevent that, emit these functions inlined: the compiler will
> ignore such unused functions.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC: No clue about QAPI...
> Tested with GCC. If the compiler is picky we could use the 'unused'
> function attribute.

And I have no clue if clang will warn about an unused inline function.
Going with the compiler attribute seems safer and just as easy to do
in the same two-line change (remember, the "unused" attribute merely
means "suppress warnings if I don't use this", and not "warn me if I
use it in spite of calling it unused").

> ---
>  scripts/qapi/commands.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 0e13d510547..bbed776a909 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -91,8 +91,8 @@ def gen_call(name: str,
>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>      return mcgen('''
>  
> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
> -                                QObject **ret_out, Error **errp)
> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
> +                                        QObject **ret_out, Error **errp)

On the other hand, the qapi generator is smart enough to only output
introspection data for qapi types that were actually used by a command
or event, so how is that working, and why is it not also being used to
elide the generation of unused qmp_marshal_output_FOO functions?  This
is where I'll have to defer to Markus.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/2] tpm: Return QMP error when TPM is disabled in build

[Eric Blake]

On Wed, Jun 09, 2021 at 08:49:55PM +0200, Philippe Mathieu-Daudé wrote:
> When the management layer queries a binary built using --disable-tpm
> for TPM devices, it gets confused by getting empty responses:
> 
...
> 
> To make it clearer by returning an error:
> - Make the TPM QAPI schema conditional
> - Adapt the HMP command
> - Remove stubs which became unnecessary
> 
> The management layer now gets a 'CommandNotFound' error:
> 
>   { "execute": "query-tpm" }
>   {
>       "error": {
>           "class": "CommandNotFound",
>           "desc": "The command query-tpm has not been found"
>       }
>   }
> 
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qapi/tpm.json      |  9 ++++++---
>  monitor/hmp-cmds.c |  4 ++++
>  stubs/tpm.c        | 16 ----------------
>  3 files changed, 10 insertions(+), 19 deletions(-)

Yes, looks nicer.

> 
> diff --git a/qapi/tpm.json b/qapi/tpm.json
> index 6a10c9ed8d2..09332e6f996 100644
> --- a/qapi/tpm.json
> +++ b/qapi/tpm.json
> @@ -33,7 +33,8 @@
>  # <- { "return": [ "tpm-tis", "tpm-crb", "tpm-spapr" ] }
>  #
>  ##
> -{ 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
> +{ 'command': 'query-tpm-models', 'returns': ['TpmModel'],
> +  'if': 'defined(CONFIG_TPM)' }

May need a rebase if the series to make 'if' language-agnostic lands
first (in fact, that would probably result in a build-time semantic
conflict rather than a patch-application-time merge conflict), but
that's not enough to prevent me from giving:

Reviewed-by: Eric Blake <eblake@redhat.com

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote:
>> In case we need to use QAPI types but no QAPI command / QAPI event
>> actually use them, the generated qmp_marshal_output() function will
>> trigger the compiler 'unused-function' warnings.
>> To prevent that, emit these functions inlined: the compiler will
>> ignore such unused functions.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> RFC: No clue about QAPI...
>> Tested with GCC. If the compiler is picky we could use the 'unused'
>> function attribute.
>
> And I have no clue if clang will warn about an unused inline function.
> Going with the compiler attribute seems safer and just as easy to do
> in the same two-line change (remember, the "unused" attribute merely
> means "suppress warnings if I don't use this", and not "warn me if I
> use it in spite of calling it unused").
>
>> ---
>>  scripts/qapi/commands.py | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>> index 0e13d510547..bbed776a909 100644
>> --- a/scripts/qapi/commands.py
>> +++ b/scripts/qapi/commands.py
>> @@ -91,8 +91,8 @@ def gen_call(name: str,
>>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>>      return mcgen('''
>>  
>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>> -                                QObject **ret_out, Error **errp)
>> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>> +                                        QObject **ret_out, Error **errp)
>
> On the other hand, the qapi generator is smart enough to only output
> introspection data for qapi types that were actually used by a command
> or event, so how is that working, and why is it not also being used to
> elide the generation of unused qmp_marshal_output_FOO functions?  This
> is where I'll have to defer to Markus.

This is a QAPI generator restriction.  Let me explain.

The qmp_marshal_output_T() are shared by all commands returning T.

The commands may be conditional.  The user is responsible for making T's
'if' the conjunction of the commands'.  See the FIXME in commands.py.

If I do this for tpm.json (appended), then tpm.h misses TpmModel when
CONFIG_TPM is off, and tpm_backend.h misses TpmType and TpmInfo.  I
suspect more TPM code needs to be guarded by CONFIG_TPM.



diff --git a/qapi/tpm.json b/qapi/tpm.json
index 09332e6f99..e74c881ea6 100644
--- a/qapi/tpm.json
+++ b/qapi/tpm.json
@@ -17,7 +17,9 @@
 #
 # Since: 1.5
 ##
-{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
+{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ],
+  'if': 'defined(CONFIG_TPM)' }
+
 ##
 # @query-tpm-models:
 #
@@ -47,7 +49,8 @@
 #
 # Since: 1.5
 ##
-{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ] }
+{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ],
+  'if': 'defined(CONFIG_TPM)' }
 
 ##
 # @query-tpm-types:
@@ -124,7 +127,8 @@
 { 'struct': 'TPMInfo',
   'data': {'id': 'str',
            'model': 'TpmModel',
-           'options': 'TpmTypeOptions' } }
+           'options': 'TpmTypeOptions' },
+  'if': 'defined(CONFIG_TPM)' }
 
 ##
 # @query-tpm:

Re: [PATCH v2 2/2] tpm: Return QMP error when TPM is disabled in build

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> When the management layer queries a binary built using --disable-tpm
> for TPM devices, it gets confused by getting empty responses:

What software exactly gets confused, and how?

>
>   { "execute": "query-tpm" }
>   {
>       "return": [
>       ]
>   }
>   { "execute": "query-tpm-types" }
>   {
>       "return": [
>       ]
>   }
>   { "execute": "query-tpm-models" }
>   {
>       "return": [
>       ]
>   }
>
> To make it clearer by returning an error:
> - Make the TPM QAPI schema conditional
> - Adapt the HMP command
> - Remove stubs which became unnecessary
>
> The management layer now gets a 'CommandNotFound' error:
>
>   { "execute": "query-tpm" }
>   {
>       "error": {
>           "class": "CommandNotFound",
>           "desc": "The command query-tpm has not been found"
>       }
>   }
>
> Suggested-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Yes, please.  But see my review of PATCH 1.

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Philippe Mathieu-Daudé]

On 6/10/21 11:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote:
>>> In case we need to use QAPI types but no QAPI command / QAPI event
>>> actually use them, the generated qmp_marshal_output() function will
>>> trigger the compiler 'unused-function' warnings.
>>> To prevent that, emit these functions inlined: the compiler will
>>> ignore such unused functions.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> RFC: No clue about QAPI...
>>> Tested with GCC. If the compiler is picky we could use the 'unused'
>>> function attribute.
>>
>> And I have no clue if clang will warn about an unused inline function.
>> Going with the compiler attribute seems safer and just as easy to do
>> in the same two-line change (remember, the "unused" attribute merely
>> means "suppress warnings if I don't use this", and not "warn me if I
>> use it in spite of calling it unused").
>>
>>> ---
>>>  scripts/qapi/commands.py | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>>> index 0e13d510547..bbed776a909 100644
>>> --- a/scripts/qapi/commands.py
>>> +++ b/scripts/qapi/commands.py
>>> @@ -91,8 +91,8 @@ def gen_call(name: str,
>>>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>>>      return mcgen('''
>>>  
>>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>> -                                QObject **ret_out, Error **errp)
>>> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>> +                                        QObject **ret_out, Error **errp)
>>
>> On the other hand, the qapi generator is smart enough to only output
>> introspection data for qapi types that were actually used by a command
>> or event, so how is that working, and why is it not also being used to
>> elide the generation of unused qmp_marshal_output_FOO functions?  This
>> is where I'll have to defer to Markus.
> 
> This is a QAPI generator restriction.  Let me explain.
> 
> The qmp_marshal_output_T() are shared by all commands returning T.
> 
> The commands may be conditional.  The user is responsible for making T's
> 'if' the conjunction of the commands'.  See the FIXME in commands.py.

Yes, I noticed the FIXME:

    # FIXME: If T is a user-defined type, the user is responsible
    # for making this work, i.e. to make T's condition the
    # conjunction of the T-returning commands' conditions.  If T
    # is a built-in type, this isn't possible: the
    # qmp_marshal_output_T() will be generated unconditionally.

Using inline / unused attributes don't invalidate this :)

> If I do this for tpm.json (appended), then tpm.h misses TpmModel when
> CONFIG_TPM is off, and tpm_backend.h misses TpmType and TpmInfo.  I
> suspect more TPM code needs to be guarded by CONFIG_TPM.

Yes, this is what I did first, use the code below and add #ifdef'ry,
but the code becomes ugly and harder to maintain because the enums
are used in middle of a QOM interface structure:

include/sysemu/tpm.h-37-struct TPMIfClass {
include/sysemu/tpm.h-38-    InterfaceClass parent_class;
include/sysemu/tpm.h-39-
include/sysemu/tpm.h:40:    enum TpmModel model;
include/sysemu/tpm.h-41-    void (*request_completed)(TPMIf *obj, int ret);
include/sysemu/tpm.h-42-    enum TPMVersion (*get_version)(TPMIf *obj);
include/sysemu/tpm.h-43-};
include/sysemu/tpm.h-44-

If you think using inline / unused attributes is not an option for
QAPI, then the #ifdef'ry isn't worth it and I'd prefer use v1 which
doesn't use conditional QAPI suggested by Marc-André.

> diff --git a/qapi/tpm.json b/qapi/tpm.json
> index 09332e6f99..e74c881ea6 100644
> --- a/qapi/tpm.json
> +++ b/qapi/tpm.json
> @@ -17,7 +17,9 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ],
> +  'if': 'defined(CONFIG_TPM)' }
> +
>  ##
>  # @query-tpm-models:
>  #
> @@ -47,7 +49,8 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ] }
> +{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ],
> +  'if': 'defined(CONFIG_TPM)' }
>  
>  ##
>  # @query-tpm-types:
> @@ -124,7 +127,8 @@
>  { 'struct': 'TPMInfo',
>    'data': {'id': 'str',
>             'model': 'TpmModel',
> -           'options': 'TpmTypeOptions' } }
> +           'options': 'TpmTypeOptions' },
> +  'if': 'defined(CONFIG_TPM)' }
>  
>  ##
>  # @query-tpm:
> 

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 6/10/21 11:33 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote:
>>>> In case we need to use QAPI types but no QAPI command / QAPI event
>>>> actually use them, the generated qmp_marshal_output() function will
>>>> trigger the compiler 'unused-function' warnings.
>>>> To prevent that, emit these functions inlined: the compiler will
>>>> ignore such unused functions.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> RFC: No clue about QAPI...
>>>> Tested with GCC. If the compiler is picky we could use the 'unused'
>>>> function attribute.
>>>
>>> And I have no clue if clang will warn about an unused inline function.
>>> Going with the compiler attribute seems safer and just as easy to do
>>> in the same two-line change (remember, the "unused" attribute merely
>>> means "suppress warnings if I don't use this", and not "warn me if I
>>> use it in spite of calling it unused").
>>>
>>>> ---
>>>>  scripts/qapi/commands.py | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>>>> index 0e13d510547..bbed776a909 100644
>>>> --- a/scripts/qapi/commands.py
>>>> +++ b/scripts/qapi/commands.py
>>>> @@ -91,8 +91,8 @@ def gen_call(name: str,
>>>>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>>>>      return mcgen('''
>>>>  
>>>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>>> -                                QObject **ret_out, Error **errp)
>>>> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>>> +                                        QObject **ret_out, Error **errp)
>>>
>>> On the other hand, the qapi generator is smart enough to only output
>>> introspection data for qapi types that were actually used by a command
>>> or event, so how is that working, and why is it not also being used to
>>> elide the generation of unused qmp_marshal_output_FOO functions?  This
>>> is where I'll have to defer to Markus.
>> 
>> This is a QAPI generator restriction.  Let me explain.
>> 
>> The qmp_marshal_output_T() are shared by all commands returning T.
>> 
>> The commands may be conditional.  The user is responsible for making T's
>> 'if' the conjunction of the commands'.  See the FIXME in commands.py.
>
> Yes, I noticed the FIXME:
>
>     # FIXME: If T is a user-defined type, the user is responsible
>     # for making this work, i.e. to make T's condition the
>     # conjunction of the T-returning commands' conditions.  If T
>     # is a built-in type, this isn't possible: the
>     # qmp_marshal_output_T() will be generated unconditionally.
>
> Using inline / unused attributes don't invalidate this :)

Generating the unused attribute lets us keep types unconditional even
when the commands returning them are conditional (also takes care of the
built-in case, where we cannot make the type conditional).

However, conditional commands returning an unconditional type is a bit
of a code smell.  In this particular case, the smell seems to lead to a
(minor) issue: too much TPM code is compiled even when CONFIG_TPM is
off.  With the attribute in place, we wouldn't have learned this.

We may still find non-smelly instances of this pattern.  Until then, I'm
a bit reluctant to generate the attribute.

>> If I do this for tpm.json (appended), then tpm.h misses TpmModel when
>> CONFIG_TPM is off, and tpm_backend.h misses TpmType and TpmInfo.  I
>> suspect more TPM code needs to be guarded by CONFIG_TPM.
>
> Yes, this is what I did first, use the code below and add #ifdef'ry,
> but the code becomes ugly and harder to maintain because the enums
> are used in middle of a QOM interface structure:
>
> include/sysemu/tpm.h-37-struct TPMIfClass {
> include/sysemu/tpm.h-38-    InterfaceClass parent_class;
> include/sysemu/tpm.h-39-
> include/sysemu/tpm.h:40:    enum TpmModel model;
> include/sysemu/tpm.h-41-    void (*request_completed)(TPMIf *obj, int ret);
> include/sysemu/tpm.h-42-    enum TPMVersion (*get_version)(TPMIf *obj);
> include/sysemu/tpm.h-43-};
> include/sysemu/tpm.h-44-
>
> If you think using inline / unused attributes is not an option for
> QAPI, then the #ifdef'ry isn't worth it and I'd prefer use v1 which
> doesn't use conditional QAPI suggested by Marc-André.

Ignorant question: why do we want to define QOM type "tpm-if" when
CONFIG_TPM is off?

[...]

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Philippe Mathieu-Daudé]

On 6/10/21 1:06 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 6/10/21 11:33 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> In case we need to use QAPI types but no QAPI command / QAPI event
>>>>> actually use them, the generated qmp_marshal_output() function will
>>>>> trigger the compiler 'unused-function' warnings.
>>>>> To prevent that, emit these functions inlined: the compiler will
>>>>> ignore such unused functions.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>> ---
>>>>> RFC: No clue about QAPI...
>>>>> Tested with GCC. If the compiler is picky we could use the 'unused'
>>>>> function attribute.
>>>>
>>>> And I have no clue if clang will warn about an unused inline function.
>>>> Going with the compiler attribute seems safer and just as easy to do
>>>> in the same two-line change (remember, the "unused" attribute merely
>>>> means "suppress warnings if I don't use this", and not "warn me if I
>>>> use it in spite of calling it unused").
>>>>
>>>>> ---
>>>>>  scripts/qapi/commands.py | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>>>>> index 0e13d510547..bbed776a909 100644
>>>>> --- a/scripts/qapi/commands.py
>>>>> +++ b/scripts/qapi/commands.py
>>>>> @@ -91,8 +91,8 @@ def gen_call(name: str,
>>>>>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>>>>>      return mcgen('''
>>>>>  
>>>>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>>>> -                                QObject **ret_out, Error **errp)
>>>>> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>>>> +                                        QObject **ret_out, Error **errp)
>>>>
>>>> On the other hand, the qapi generator is smart enough to only output
>>>> introspection data for qapi types that were actually used by a command
>>>> or event, so how is that working, and why is it not also being used to
>>>> elide the generation of unused qmp_marshal_output_FOO functions?  This
>>>> is where I'll have to defer to Markus.
>>>
>>> This is a QAPI generator restriction.  Let me explain.
>>>
>>> The qmp_marshal_output_T() are shared by all commands returning T.
>>>
>>> The commands may be conditional.  The user is responsible for making T's
>>> 'if' the conjunction of the commands'.  See the FIXME in commands.py.
>>
>> Yes, I noticed the FIXME:
>>
>>     # FIXME: If T is a user-defined type, the user is responsible
>>     # for making this work, i.e. to make T's condition the
>>     # conjunction of the T-returning commands' conditions.  If T
>>     # is a built-in type, this isn't possible: the
>>     # qmp_marshal_output_T() will be generated unconditionally.
>>
>> Using inline / unused attributes don't invalidate this :)
> 
> Generating the unused attribute lets us keep types unconditional even
> when the commands returning them are conditional (also takes care of the
> built-in case, where we cannot make the type conditional).
> 
> However, conditional commands returning an unconditional type is a bit
> of a code smell.  In this particular case, the smell seems to lead to a
> (minor) issue: too much TPM code is compiled even when CONFIG_TPM is
> off.  With the attribute in place, we wouldn't have learned this.
> 
> We may still find non-smelly instances of this pattern.  Until then, I'm
> a bit reluctant to generate the attribute.

I agree with your nose :)

>>> If I do this for tpm.json (appended), then tpm.h misses TpmModel when
>>> CONFIG_TPM is off, and tpm_backend.h misses TpmType and TpmInfo.  I
>>> suspect more TPM code needs to be guarded by CONFIG_TPM.
>>
>> Yes, this is what I did first, use the code below and add #ifdef'ry,
>> but the code becomes ugly and harder to maintain because the enums
>> are used in middle of a QOM interface structure:
>>
>> include/sysemu/tpm.h-37-struct TPMIfClass {
>> include/sysemu/tpm.h-38-    InterfaceClass parent_class;
>> include/sysemu/tpm.h-39-
>> include/sysemu/tpm.h:40:    enum TpmModel model;
>> include/sysemu/tpm.h-41-    void (*request_completed)(TPMIf *obj, int ret);
>> include/sysemu/tpm.h-42-    enum TPMVersion (*get_version)(TPMIf *obj);
>> include/sysemu/tpm.h-43-};
>> include/sysemu/tpm.h-44-
>>
>> If you think using inline / unused attributes is not an option for
>> QAPI, then the #ifdef'ry isn't worth it and I'd prefer use v1 which
>> doesn't use conditional QAPI suggested by Marc-André.
> 
> Ignorant question: why do we want to define QOM type "tpm-if" when
> CONFIG_TPM is off?

Good question. I suppose for historical reasons? Copy/pasting of
another older include/sysemu/ files? Recently I saw these headers
received more love, such better #ifdef'ry to allow code elision.

I'll defer that to Stefan.

Thanks for the review,

Phil.

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 6/10/21 1:06 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> On 6/10/21 11:33 AM, Markus Armbruster wrote:
>>>> Eric Blake <eblake@redhat.com> writes:
>>>>
>>>>> On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>> In case we need to use QAPI types but no QAPI command / QAPI event
>>>>>> actually use them, the generated qmp_marshal_output() function will
>>>>>> trigger the compiler 'unused-function' warnings.
>>>>>> To prevent that, emit these functions inlined: the compiler will
>>>>>> ignore such unused functions.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>> ---
>>>>>> RFC: No clue about QAPI...
>>>>>> Tested with GCC. If the compiler is picky we could use the 'unused'
>>>>>> function attribute.
>>>>>
>>>>> And I have no clue if clang will warn about an unused inline function.
>>>>> Going with the compiler attribute seems safer and just as easy to do
>>>>> in the same two-line change (remember, the "unused" attribute merely
>>>>> means "suppress warnings if I don't use this", and not "warn me if I
>>>>> use it in spite of calling it unused").
>>>>>
>>>>>> ---
>>>>>>  scripts/qapi/commands.py | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
>>>>>> index 0e13d510547..bbed776a909 100644
>>>>>> --- a/scripts/qapi/commands.py
>>>>>> +++ b/scripts/qapi/commands.py
>>>>>> @@ -91,8 +91,8 @@ def gen_call(name: str,
>>>>>>  def gen_marshal_output(ret_type: QAPISchemaType) -> str:
>>>>>>      return mcgen('''
>>>>>>  
>>>>>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>>>>> -                                QObject **ret_out, Error **errp)
>>>>>> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
>>>>>> +                                        QObject **ret_out, Error **errp)
>>>>>
>>>>> On the other hand, the qapi generator is smart enough to only output
>>>>> introspection data for qapi types that were actually used by a command
>>>>> or event, so how is that working, and why is it not also being used to
>>>>> elide the generation of unused qmp_marshal_output_FOO functions?  This
>>>>> is where I'll have to defer to Markus.
>>>>
>>>> This is a QAPI generator restriction.  Let me explain.
>>>>
>>>> The qmp_marshal_output_T() are shared by all commands returning T.
>>>>
>>>> The commands may be conditional.  The user is responsible for making T's
>>>> 'if' the conjunction of the commands'.  See the FIXME in commands.py.
>>>
>>> Yes, I noticed the FIXME:
>>>
>>>     # FIXME: If T is a user-defined type, the user is responsible
>>>     # for making this work, i.e. to make T's condition the
>>>     # conjunction of the T-returning commands' conditions.  If T
>>>     # is a built-in type, this isn't possible: the
>>>     # qmp_marshal_output_T() will be generated unconditionally.
>>>
>>> Using inline / unused attributes don't invalidate this :)
>> 
>> Generating the unused attribute lets us keep types unconditional even
>> when the commands returning them are conditional (also takes care of the
>> built-in case, where we cannot make the type conditional).
>> 
>> However, conditional commands returning an unconditional type is a bit
>> of a code smell.  In this particular case, the smell seems to lead to a
>> (minor) issue: too much TPM code is compiled even when CONFIG_TPM is
>> off.  With the attribute in place, we wouldn't have learned this.
>> 
>> We may still find non-smelly instances of this pattern.  Until then, I'm
>> a bit reluctant to generate the attribute.
>
> I agree with your nose :)
>
>>>> If I do this for tpm.json (appended), then tpm.h misses TpmModel when
>>>> CONFIG_TPM is off, and tpm_backend.h misses TpmType and TpmInfo.  I
>>>> suspect more TPM code needs to be guarded by CONFIG_TPM.
>>>
>>> Yes, this is what I did first, use the code below and add #ifdef'ry,
>>> but the code becomes ugly and harder to maintain because the enums
>>> are used in middle of a QOM interface structure:
>>>
>>> include/sysemu/tpm.h-37-struct TPMIfClass {
>>> include/sysemu/tpm.h-38-    InterfaceClass parent_class;
>>> include/sysemu/tpm.h-39-
>>> include/sysemu/tpm.h:40:    enum TpmModel model;
>>> include/sysemu/tpm.h-41-    void (*request_completed)(TPMIf *obj, int ret);
>>> include/sysemu/tpm.h-42-    enum TPMVersion (*get_version)(TPMIf *obj);
>>> include/sysemu/tpm.h-43-};
>>> include/sysemu/tpm.h-44-
>>>
>>> If you think using inline / unused attributes is not an option for
>>> QAPI, then the #ifdef'ry isn't worth it and I'd prefer use v1 which
>>> doesn't use conditional QAPI suggested by Marc-André.
>> 
>> Ignorant question: why do we want to define QOM type "tpm-if" when
>> CONFIG_TPM is off?
>
> Good question. I suppose for historical reasons? Copy/pasting of
> another older include/sysemu/ files? Recently I saw these headers
> received more love, such better #ifdef'ry to allow code elision.
>
> I'll defer that to Stefan.

Stefan, would you be willing to look into this?

Re: [RFC PATCH v2 1/2] qapi: Inline qmp_marshal_output() functions

[Stefan Berger]

On 6/11/21 4:02 AM, Markus Armbruster wrote:
> Stefan, would you be willing to look into this?
>
Have a look at the 3 topmost patches: 
https://github.com/stefanberger/qemu-tpm/commits/tpm-eliminate-if-not-config-tpm