From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MEQ52-0002Za-7C for qemu-devel@nongnu.org; Wed, 10 Jun 2009 11:51:12 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MEQ4x-0002Yw-Gk for qemu-devel@nongnu.org; Wed, 10 Jun 2009 11:51:11 -0400 Received: from [199.232.76.173] (port=51546 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MEQ4x-0002Yt-9F for qemu-devel@nongnu.org; Wed, 10 Jun 2009 11:51:07 -0400 Received: from mx2.redhat.com ([66.187.237.31]:54488) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MEQ4w-0001nh-PC for qemu-devel@nongnu.org; Wed, 10 Jun 2009 11:51:07 -0400 Date: Wed, 10 Jun 2009 18:48:06 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up. Message-ID: <20090610154806.GF28601@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. So maybe this is going too far into a table-driven direction then. Tables are good for common case, exceptions are better handled by regular functional design. I agree addr/len comparisons are fragile, but can't we simply implement functions to encapsulate them? Along the lines of: static inline int offset_in_range(int offset, int address, int len) { return address <= offset && address + len > offset; } static inline int ranges_match(int addr1, int len1, int addr2, int len2) { return offset_affected(addr1, addr2, len2) || offset_affected(addr2, addr1, len1); } Switching address and len comparison to use this would be a good cleanup IMO. > > > > > > + 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? > > - 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? > > > > > > > > > > /* 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