Re: [Qemu-devel] [PATCH 1/2] Make pointer to fw_cfg device global.

[Anthony Liguori]

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