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<g...@redhat.com>
---
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