Patchwork [10/12] config: Add header file for device config options

login
register
mail settings
Submitter Alexander Graf
Date Nov. 23, 2010, 2:34 p.m.
Message ID <1290522889-27559-11-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/72675/
State New
Headers show

Comments

Alexander Graf - Nov. 23, 2010, 2:34 p.m.
So far we have C preprocessor defines for target and host config
options, but we're lacking any information on which devices are
available.

We do need that information at times though, for example in the
ahci patch where we need to call a legacy init function depending
on whether we have support compiled in or not.

So this patch makes all config-devices options available as header
file. Please only include it in machine description code!

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v3 -> v4:

  - config: only include config-devices.h in machine description (blue swirl)
---
 Makefile.target |    5 ++++-
 hw/pc_piix.c    |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)
Blue Swirl - Nov. 23, 2010, 7:21 p.m.
On Tue, Nov 23, 2010 at 2:34 PM, Alexander Graf <agraf@suse.de> wrote:
> So far we have C preprocessor defines for target and host config
> options, but we're lacking any information on which devices are
> available.
>
> We do need that information at times though, for example in the
> ahci patch where we need to call a legacy init function depending
> on whether we have support compiled in or not.
>
> So this patch makes all config-devices options available as header
> file. Please only include it in machine description code!

How about including config-devices.h from hw/boards.h instead? On the
downside, all targets will be recompiled when config-devices.h change,
even if they don't have any device dependencies.
Alexander Graf - Nov. 23, 2010, 7:37 p.m.
On 23.11.2010, at 20:21, Blue Swirl wrote:

> On Tue, Nov 23, 2010 at 2:34 PM, Alexander Graf <agraf@suse.de> wrote:
>> So far we have C preprocessor defines for target and host config
>> options, but we're lacking any information on which devices are
>> available.
>> 
>> We do need that information at times though, for example in the
>> ahci patch where we need to call a legacy init function depending
>> on whether we have support compiled in or not.
>> 
>> So this patch makes all config-devices options available as header
>> file. Please only include it in machine description code!
> 
> How about including config-devices.h from hw/boards.h instead? On the
> downside, all targets will be recompiled when config-devices.h change,
> even if they don't have any device dependencies.

Not sure it makes sense to do this change this time around. If we can avoid dependency on device configuration, we should. So if we can limit the scope this is included in, it's good IMHO.

In fact, maybe the correct solution to the issue would be to move the legacy -drive instantiation to the -drive code, as it is already with virtio-blk. If used with -device and -drive ...,if=none the whole magic isn't necessary anyways.


Alex
Juan Quintela - Nov. 24, 2010, 2:22 a.m.
Alexander Graf <agraf@suse.de> wrote:
> So far we have C preprocessor defines for target and host config
> options, but we're lacking any information on which devices are
> available.
>
> We do need that information at times though, for example in the
> ahci patch where we need to call a legacy init function depending
> on whether we have support compiled in or not.
>
> So this patch makes all config-devices options available as header
> file. Please only include it in machine description code!
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

This already existed in the past,  Paul Brook reverted it.

commit a992fe3d0fc185112677286f7a02204d8245b61e
Author: Paul Brook <paul@codesourcery.com>
Date:   Sun Nov 22 16:25:30 2009 +0000

    Makefile dependencies for device configs

I did the 1st implementation, and I tried to get it back several times.
Paul Brook reasoning is that "if you need it, our device model is wrong.

If you show that you want to be able to compile out several devices, ...

Paul Brook:  "if you need it, our device model is wrong"

If you don't want to have the abomination of xen-stub.c kvm-stub.c,
etc....

Paul Brook:  "if you need it, our device model is wrong".

Last time I tried to get it back was to be able to compile out hpet (it
didn't worked well at the time).

I let you guess how it ended.

Later, Juan.
Alexander Graf - Nov. 24, 2010, 10:54 a.m.
On 24.11.2010, at 03:22, Juan Quintela wrote:

> Alexander Graf <agraf@suse.de> wrote:
>> So far we have C preprocessor defines for target and host config
>> options, but we're lacking any information on which devices are
>> available.
>> 
>> We do need that information at times though, for example in the
>> ahci patch where we need to call a legacy init function depending
>> on whether we have support compiled in or not.
>> 
>> So this patch makes all config-devices options available as header
>> file. Please only include it in machine description code!
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> This already existed in the past,  Paul Brook reverted it.
> 
> commit a992fe3d0fc185112677286f7a02204d8245b61e
> Author: Paul Brook <paul@codesourcery.com>
> Date:   Sun Nov 22 16:25:30 2009 +0000
> 
>    Makefile dependencies for device configs
> 
> I did the 1st implementation, and I tried to get it back several times.
> Paul Brook reasoning is that "if you need it, our device model is wrong.
> 
> If you show that you want to be able to compile out several devices, ...
> 
> Paul Brook:  "if you need it, our device model is wrong"
> 
> If you don't want to have the abomination of xen-stub.c kvm-stub.c,
> etc....
> 
> Paul Brook:  "if you need it, our device model is wrong".
> 
> Last time I tried to get it back was to be able to compile out hpet (it
> didn't worked well at the time).
> 
> I let you guess how it ended.

Sure, I only want to keep it in as a temporary solution in cases where overhauling the device model needs a lot of thorough thought and considerations.

As I stated for this particular issue, maybe the way forward is to move all the initialization code over to -drive instead of keeping it close to the device. But I'm not sure yet. Hence this very little intrusive patch.


Alex
Blue Swirl - Nov. 24, 2010, 8:45 p.m.
On Wed, Nov 24, 2010 at 10:54 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 24.11.2010, at 03:22, Juan Quintela wrote:
>
>> Alexander Graf <agraf@suse.de> wrote:
>>> So far we have C preprocessor defines for target and host config
>>> options, but we're lacking any information on which devices are
>>> available.
>>>
>>> We do need that information at times though, for example in the
>>> ahci patch where we need to call a legacy init function depending
>>> on whether we have support compiled in or not.
>>>
>>> So this patch makes all config-devices options available as header
>>> file. Please only include it in machine description code!
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> This already existed in the past,  Paul Brook reverted it.
>>
>> commit a992fe3d0fc185112677286f7a02204d8245b61e
>> Author: Paul Brook <paul@codesourcery.com>
>> Date:   Sun Nov 22 16:25:30 2009 +0000
>>
>>    Makefile dependencies for device configs
>>
>> I did the 1st implementation, and I tried to get it back several times.
>> Paul Brook reasoning is that "if you need it, our device model is wrong.
>>
>> If you show that you want to be able to compile out several devices, ...
>>
>> Paul Brook:  "if you need it, our device model is wrong"
>>
>> If you don't want to have the abomination of xen-stub.c kvm-stub.c,
>> etc....
>>
>> Paul Brook:  "if you need it, our device model is wrong".
>>
>> Last time I tried to get it back was to be able to compile out hpet (it
>> didn't worked well at the time).
>>
>> I let you guess how it ended.
>
> Sure, I only want to keep it in as a temporary solution in cases where overhauling the device model needs a lot of thorough thought and considerations.
>
> As I stated for this particular issue, maybe the way forward is to move all the initialization code over to -drive instead of keeping it close to the device. But I'm not sure yet. Hence this very little intrusive patch.

I read the config-devices.h thread and I agree that it's a bad idea.
So, I'm also NACKing this patch. Please adjust the code so that
CONFIG_AHCI is no longer needed.

Patch

diff --git a/Makefile.target b/Makefile.target
index 91e6e74..35862fd 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -1,6 +1,6 @@ 
 # -*- Mode: makefile -*-
 
-GENERATED_HEADERS = config-target.h
+GENERATED_HEADERS = config-target.h config-devices.h
 CONFIG_NO_KVM = $(if $(subst n,,$(CONFIG_KVM)),n,y)
 
 include ../config-host.mak
@@ -40,6 +40,9 @@  kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
 config-target.h: config-target.h-timestamp
 config-target.h-timestamp: config-target.mak
 
+config-devices.h: config-target.h-timestamp
+config-devices.h-timestamp: config-target.mak
+
 all: $(PROGS)
 
 # Dummy command so that make thinks it has done something
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 12359a7..2be25a6 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -35,6 +35,7 @@ 
 #include "sysemu.h"
 #include "sysbus.h"
 #include "blockdev.h"
+#include "config-devices.h"
 
 #define MAX_IDE_BUS 2