From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NkbyF-0007UO-Ms for qemu-devel@nongnu.org; Thu, 25 Feb 2010 06:33:31 -0500 Received: from [199.232.76.173] (port=39788 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NkbyF-0007U3-2l for qemu-devel@nongnu.org; Thu, 25 Feb 2010 06:33:31 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NkbyD-0008Um-EX for qemu-devel@nongnu.org; Thu, 25 Feb 2010 06:33:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:26869) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NkbyC-0008Ui-Up for qemu-devel@nongnu.org; Thu, 25 Feb 2010 06:33:29 -0500 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o1PBXR1U005265 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Feb 2010 06:33:27 -0500 Date: Thu, 25 Feb 2010 13:30:13 +0200 From: "Michael S. Tsirkin" Message-ID: <20100225113013.GA9116@redhat.com> References: <1267087285-15582-1-git-send-email-kraxel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH] adding helper pci functions List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: Izik Eidus , Gerd Hoffmann , qemu-devel@nongnu.org On Thu, Feb 25, 2010 at 11:55:25AM +0100, Juan Quintela wrote: > Gerd Hoffmann wrote: > > From: Izik Eidus > > > > Signed-off-by: Izik Eidus > > Signed-off-by: Gerd Hoffmann > > --- > > hw/pci.h | 18 ++++++++++++++++++ > > 1 files changed, 18 insertions(+), 0 deletions(-) > > > > diff --git a/hw/pci.h b/hw/pci.h > > index 37ebdc4..20c670e 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -301,11 +301,29 @@ pci_config_set_device_id(uint8_t *pci_config, uint16_t val) > > } > > > > static inline void > > +pci_config_set_revision(uint8_t *pci_config, uint8_t val) > > +{ > > + pci_set_byte(&pci_config[PCI_REVISION_ID], val); > > +} > > + > > +static inline void > > pci_config_set_class(uint8_t *pci_config, uint16_t val) > > { > > pci_set_word(&pci_config[PCI_CLASS_DEVICE], val); > > } > > > > +static inline void > > +pci_config_set_prog_interface(uint8_t *pci_config, uint8_t val) > > +{ > > + pci_set_byte(&pci_config[PCI_CLASS_PROG], val); > > +} > > + > > +static inline void > > +pci_config_set_interrupt_pin(uint8_t *pci_config, uint8_t val) > > +{ > > + pci_set_byte(&pci_config[PCI_INTERRUPT_PIN], val); > > +} > > + This one is wrong I think. Devices have no business touching interrupt pin register. > > typedef int (*pci_qdev_initfn)(PCIDevice *dev); > > typedef struct { > > DeviceInfo qdev; > > mst had some reservations abotu this functions, but I have forgotten. > > mst can you comment again? > > Later, Juan. Yes, I commented on this internally. By the last count, we have about 600 registers. Adding incline functions for all of them is just a source of bugs, using pci_regs directly is better in that sense. Even worse, this creates two legal ways to do the same thing. So no one thing you can grep for. We could convert all devices, but it's a lot of churn for very little benefit, and I don't see how such a change can be tested. So I think really only the common registers should have inline functions, and for most devices, class/revision/prog interface should just have default values. Finally, the APIs as proposed here just do not get us much. You still must remember which values to put in: pci_config_set_prog_interface(config, PCI_CLASS_SERIAL_USB) will happily compile, and you still must verify that value you put in does fit in. What I would really like to see is higher level APIs. So pci_init_legacy_vga_adapter might make sense, it would not just set class, but also register IO handlers for legacy ports. -- MST