All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	qemu-devel@nongnu.org, "Eric Blake" <eblake@redhat.com>,
	qemu-arm@nongnu.org, "Thomas Huth" <thuth@redhat.com>
Subject: Re: [RFC PATCH-for-9.1? 2/2] hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in
Date: Thu, 08 Aug 2024 07:35:21 +0200	[thread overview]
Message-ID: <871q2zshk6.fsf@pond.sub.org> (raw)
In-Reply-To: <CAFEAcA_2pJA47K72qJQX9bc8sBcA+0wJGaf3KAaYJaRurjQD7w@mail.gmail.com> (Peter Maydell's message of "Wed, 7 Aug 2024 17:30:50 +0100")

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 7 Aug 2024 at 12:10, Markus Armbruster <armbru@redhat.com> wrote:
>> Having to manually include a configuration header like CONFIG_DEVICES
>> wherever you use configuration symbols strikes me as unadvisable when
>> uses include checking for definedness, such as #ifdef: silent miscompile
>> when you forget to include.
>>
>> This is why Autoconf wants you to include config.h first in any .c: it
>> makes #ifdef & friends safe.
>>
>> qemu/osdep.h does include some configuration headers:
>>
>>     #include "config-host.h"
>>     #ifdef COMPILING_PER_TARGET
>>     #include CONFIG_TARGET
>>     #else
>>     #include "exec/poison.h"
>>     #endif
>>
>> Why not CONFIG_DEVICES?
>
> The stuff in CONFIG_DEVICES is target-specific, so wanting
> to include it should be rare (currently we include it in
> only about 25 files). Any file that includes it has to be
> a compile-per-target file, and generally we'd rather avoid that.

Since all the macros defined in CONFIG_DEVICES are poisoned by
exec/poison.h, which *is* included by qemu/osdep.h, target-independent
files never need to include CONFIG_DEVICES.  My question was strictly
about target-dependent files, i.e. exactly the ones that include
CONFIG_TARGET.

> Plus it's a bit odd to need to change code based on whether
> some other device was configured into the system,

I agree it's a bit odd for device code to check whether some other
device is also selected for the current target.

But is it odd for the QAPI schema to define a device-specific command
only when the device is selected?

>                                                   so I think
> that's something worth restricting to only files that effectively
> opt in to it.

Is accidental use of a macro from CONFIG_DEVICES a risk?  Is the risk
mitigated to some useful degree by having to include CONFIG_DEVICES?

I consider the combination of testing configuration symbols with #ifdef
and requiring a manual #include to actually get the symbols (and make
the #ifdef work) bad practice.

Options:

1. Approximate symbols from CONFIG_DEVICES with symbols from
   CONFIG_TARGET.  This is what we do now.

2. Use symbols from CONFIG_DEVICES.  Generated headers are no longer
   self-contained.  Strong dislike.

3. Define device-specific stuff unconditionally.  We get to fool around
   with stubs, binaries carry more useless code, and introspection
   becomes less useful.  Meh.

Thoughts?


  reply	other threads:[~2024-08-08  5:35 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-06 14:19 [RFC PATCH-for-9.1? 0/2] hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in Philippe Mathieu-Daudé
2024-08-06 14:19 ` [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/ Philippe Mathieu-Daudé
2024-08-07  3:46   ` Richard Henderson
2024-08-07  7:31     ` Philippe Mathieu-Daudé
2024-08-07 16:17     ` Peter Maydell
2024-08-08  4:32       ` Markus Armbruster
2024-08-08  8:44         ` Peter Maydell
2024-08-08  9:02           ` Markus Armbruster
2024-08-08 10:15             ` Peter Maydell
2024-08-08 11:05               ` Markus Armbruster
2024-08-08 11:23               ` Daniel P. Berrangé
2024-08-08 11:32                 ` Peter Maydell
2024-08-08 11:48                   ` Daniel P. Berrangé
2024-08-08 11:56                     ` Daniel P. Berrangé
2024-08-08 12:04                     ` Peter Maydell
2024-08-07  5:12   ` Markus Armbruster
2024-08-07  7:28     ` Philippe Mathieu-Daudé
2024-08-07  8:16       ` Markus Armbruster
2024-08-06 14:19 ` [RFC PATCH-for-9.1? 2/2] hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in Philippe Mathieu-Daudé
2024-08-07  3:47   ` Richard Henderson
2024-08-07  8:18   ` Markus Armbruster
2024-08-07  9:03     ` Philippe Mathieu-Daudé
2024-08-07 11:10       ` Markus Armbruster
2024-08-07 16:30         ` Peter Maydell
2024-08-08  5:35           ` Markus Armbruster [this message]
2024-08-08  8:48             ` Peter Maydell
2025-03-11 11:39 ` [RFC PATCH-for-9.1? 0/2] " Philippe Mathieu-Daudé

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871q2zshk6.fsf@pond.sub.org \
    --to=armbru@redhat.com \
    --cc=eblake@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.