|
From: | Anthony Liguori |
Subject: | Re: [Qemu-devel] [PATCH 1/2] Make pointer to fw_cfg device global. |
Date: | Wed, 23 May 2012 08:46:45 -0500 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 |
On 05/23/2012 08:32 AM, Gleb Natapov wrote:
On Wed, May 23, 2012 at 08:25:35AM -0500, Anthony Liguori wrote:On 05/23/2012 07:37 AM, Gleb Natapov wrote:Ping.I don't understand why you're pinging.. The patch has just been on the list for a couple of days and is definitely not a 1.1 candidate. Am I missing something?It is definitely not 1.1 candidate. Only 1.1 patches are accepted now? When master will be opened for 1.2 commits?
After 1.1 is released (June 1st).
However...Well, I am pinging for review :)On Sun, May 20, 2012 at 12:02:40PM +0300, Gleb Natapov wrote:There can be only one fw_cfg device, so saving global reference to it removes the need to pass its pointer around. Signed-off-by: Gleb Natapov<address@hidden> --- hw/fw_cfg.c | 110 +++++++++++++++++++++++++++-------------------------- hw/fw_cfg.h | 15 +++---- hw/loader.c | 10 +---- hw/loader.h | 1 - hw/multiboot.c | 17 ++++---- hw/multiboot.h | 3 +- hw/pc.c | 63 ++++++++++++++---------------- hw/ppc_newworld.c | 43 ++++++++++----------- hw/ppc_oldworld.c | 43 ++++++++++----------- hw/sun4m.c | 93 +++++++++++++++++++++----------------------- hw/sun4u.c | 35 ++++++++--------- 11 files changed, 207 insertions(+), 226 deletions(-) diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c index 7b3b576..8c50473 100644 --- a/hw/fw_cfg.c +++ b/hw/fw_cfg.c @@ -48,7 +48,7 @@ typedef struct FWCfgEntry { FWCfgCallback callback; } FWCfgEntry; -struct FWCfgState { +static struct FWCfgState { SysBusDevice busdev; MemoryRegion ctl_iomem, data_iomem, comb_iomem; uint32_t ctl_iobase, data_iobase; @@ -57,7 +57,7 @@ struct FWCfgState { uint16_t cur_entry; uint32_t cur_offset; Notifier machine_ready; -}; +} *fw_cfg; #define JPG_FILE 0 #define BMP_FILE 1 @@ -113,7 +113,7 @@ error: return NULL; } -static void fw_cfg_bootsplash(FWCfgState *s)This is a step backwards IMHO. Relying on global state is generally a bad thing. Passing around pointers is a good thing because it forces you to think about the relationships between devices and makes it hard to do silly things (like have random interactions with fw_cfg in devices that have no business doing that).We are in a kind of agreement here, but fw_cfg is this rare thing that wants to be accessed by devices which, on a first glance, have no business doing that. We already saving fw_cfg globally for rom loaders to use, not nice either.
Rom loaders are an exception because they aren't being modeled as a device today. For devices, we want the interactions to be expressed via the QOM graph which for now means having a PTR property (although actually on qom-next, it should be possible to do a proper link property).
Since you're interacting with fw_cfg in a proper device, you really should do so through the device interface (in this case, FWcfgState).
Regards, Anthony Liguori
[Prev in Thread] | Current Thread | [Next in Thread] |