From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXBtx-00066Q-I4 for qemu-devel@nongnu.org; Wed, 23 May 2012 09:47:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXBtq-00056L-HG for qemu-devel@nongnu.org; Wed, 23 May 2012 09:46:57 -0400 Received: from mail-qa0-f52.google.com ([209.85.216.52]:52990) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXBtq-00056H-Cw for qemu-devel@nongnu.org; Wed, 23 May 2012 09:46:50 -0400 Received: by qabj34 with SMTP id j34so4301686qab.11 for ; Wed, 23 May 2012 06:46:48 -0700 (PDT) Message-ID: <4FBCEA45.3060108@codemonkey.ws> Date: Wed, 23 May 2012 08:46:45 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1337504561-20297-1-git-send-email-gleb@redhat.com> <20120523123731.GF10209@redhat.com> <4FBCE54F.9070603@codemonkey.ws> <20120523133226.GI10209@redhat.com> In-Reply-To: <20120523133226.GI10209@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] Make pointer to fw_cfg device global. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov Cc: blauwirbel@gmail.com, qemu-devel@nongnu.org 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 >>>> --- >>>> 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