From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MBkrB-0006SQ-Dd for qemu-devel@nongnu.org; Wed, 03 Jun 2009 03:25:53 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MBkr6-0006Pj-Ly for qemu-devel@nongnu.org; Wed, 03 Jun 2009 03:25:52 -0400 Received: from [199.232.76.173] (port=36967 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MBkr6-0006Pg-Gj for qemu-devel@nongnu.org; Wed, 03 Jun 2009 03:25:48 -0400 Received: from mx20.gnu.org ([199.232.41.8]:35894) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MBkr5-0007pf-Vf for qemu-devel@nongnu.org; Wed, 03 Jun 2009 03:25:48 -0400 Received: from mx2.redhat.com ([66.187.237.31]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MBkr5-0006NL-93 for qemu-devel@nongnu.org; Wed, 03 Jun 2009 03:25:47 -0400 Date: Wed, 3 Jun 2009 10:22:39 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up. Message-ID: <20090603072239.GB8134@redhat.com> References: <1243924970-17545-1-git-send-email-yamahata@valinux.co.jp> <1243924970-17545-4-git-send-email-yamahata@valinux.co.jp> <20090602100121.GA3397@redhat.com> <20090603023108.GJ9176%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090603023108.GJ9176%yamahata@valinux.co.jp> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: paul@codesourcery.com, mtosatti@redhat.com, avi@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote: > On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote: > > On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote: > > > +struct PCIConfigReg { > > > + uint8_t wmask; > > > + /* offset of registers in bits for 2/4 bytes function register */ > > > + uint8_t reg_offset; > > > > Sorry about being dense, but the comment still doesn't help me much. > > Can't we simply use the index in the array as offset? > > No. I believe this is helpfull. > the next patch for hw/wdt_i6300esb.c is a good example. > With this, we can replace fragile address and len comparison > with one callback per one register function. > For that, the member which represents the position in function > is necessary. > > > > > > + pci_config_written_t callback; > > > +}; > > > > > > struct PCIDevice { > > > DeviceState qdev; > > > /* PCI config space */ > > > uint8_t config[PCI_CONFIG_SPACE_SIZE]; > > > - > > > - /* Used to implement R/W bytes */ > > > - uint8_t mask[PCI_CONFIG_SPACE_SIZE]; > > > + struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE]; > > > > I still think separate mask/config/callback arrays > > are better - they are easier for devices to use. E.g. a single memset > > can make a range of register writeable, and a single function call > > does everything necessary to save a range the whole config space. > > > > Add a callback array if you like, and be done with it. > > Hmm, I don't think so. Maybe it's a matter of taste. > Looking at your MSI/MSI-X patches, our patch series conflict with > each other. That's bad. Let's resolve the conflicts. > > - mask v.s. wmask > I think mask is ambiguous because which bits it represents, > writable bits or read only bits. > let's rename it to common name. > wmask, w_mask, wr_mask, writable_mask or something like that. > What name do you prefer? Not terribly important. I guess I like wmask if you let me pick. > - array v.s. struct > I suppose this conflicts with you. > So after renaming, I'll make wmask into uint8_t wmask[] > (or whatever name we choose) > > Then, we can avoid stepping on each other. > What do you think? Agreed. So what I need to do is rename mask into wmask, is that right? > > > > > > > > > /* the following fields are read only */ > > > PCIBus *bus; > > > @@ -180,6 +261,21 @@ struct PCIDevice { > > > int irq_state[4]; > > > }; > > > > > > +typedef void(*pci_conf_init_t)(struct PCIConfigReg*); > > > + > > > +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr, > > > + pci_config_written_t callback, uint32_t wmask); > > > +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr, > > > + pci_config_written_t callback, uint32_t wmask); > > > +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr, > > > + pci_config_written_t callback, uint32_t wmask); > > > > If we got rid of reg_offset, I think we won't need these. > > We'd just do dev->callback[REGISTER] = my_callback. > > > > > > -- > yamahata -- MST