From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NUSAe-0001QW-Kh for qemu-devel@nongnu.org; Mon, 11 Jan 2010 16:51:32 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NUSAZ-0001QK-0M for qemu-devel@nongnu.org; Mon, 11 Jan 2010 16:51:31 -0500 Received: from [199.232.76.173] (port=53828 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NUSAY-0001QH-Qr for qemu-devel@nongnu.org; Mon, 11 Jan 2010 16:51:26 -0500 Received: from qw-out-1920.google.com ([74.125.92.148]:63362) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NUSAY-000666-CY for qemu-devel@nongnu.org; Mon, 11 Jan 2010 16:51:26 -0500 Received: by qw-out-1920.google.com with SMTP id 5so3793670qwc.4 for ; Mon, 11 Jan 2010 13:51:25 -0800 (PST) Message-ID: <4B4B9D5A.9050805@codemonkey.ws> Date: Mon, 11 Jan 2010 15:51:22 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <4B45C209.1030802@mail.berlios.de> <1262862925-5205-1-git-send-email-weil@mail.berlios.de> <20100107123400.GE599@redhat.com> <4B45F8AE.1050400@mail.berlios.de> <20100111183437.GA13712@redhat.com> <4B4B7E4D.5010101@mail.berlios.de> <20100111194053.GA15857@redhat.com> <4B4B87AB.10100@mail.berlios.de> <20100111203011.GC15944@redhat.com> In-Reply-To: <20100111203011.GC15944@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [RFC] API change for pci_set_word and related functions List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On 01/11/2010 02:30 PM, Michael S. Tsirkin wrote: > On Mon, Jan 11, 2010 at 09:18:51PM +0100, Stefan Weil wrote: > >> Michael S. Tsirkin schrieb: >> >>> On Mon, Jan 11, 2010 at 08:38:53PM +0100, Stefan Weil wrote: >>> >>>> Michael S. Tsirkin schrieb: >>>> >>>>> On Thu, Jan 07, 2010 at 04:07:26PM +0100, Stefan Weil wrote: >>>>> >>>>>> Michael S. Tsirkin schrieb: >>>>>> >>>>>>> On Thu, Jan 07, 2010 at 12:15:25PM +0100, Stefan Weil wrote: >>>>>>> ... >>>>>>> >>>>>>>> - PCI_CONFIG_16(PCI_STATUS, >>>>>>>> - PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_SIG_TARGET_ABORT); >>>>>>>> + PCI_CONFIG_16(PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | >>>>>>>> PCI_STATUS_FAST_BACK); >>>>>>>> /* PCI Revision ID */ >>>>>>>> PCI_CONFIG_8(PCI_REVISION_ID, 0x08); >>>>>>>> >>>>>>> BTW if you are not afraid of churn, there's no reason >>>>>>> for PCI_CONFIG_8 and friends anymore, because pci.h >>>>>>> has much nicer pci_set_byte etc. >>>>>>> >>>>>> Hello Michael, >>>>>> >>>>>> I already noticed pci_set_byte, pci_set_word, pci_set_long and >>>>>> the corresponding pci_get_xxx functions and thought about using them. >>>>>> >>>>>> I did not start it because I want to suggest a different API >>>>>> for use in PCI device emulations: >>>>>> >>>>>> instead of >>>>>> >>>>>> pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST); >>>>>> >>>>>> or >>>>>> >>>>>> pci_set_word(&pci_conf[PCI_STATUS], PCI_STATUS_CAP_LIST); >>>>>> >>>>>> it would be better to call >>>>>> >>>>>> pci_set_word(pci_conf, PCI_STATUS, PCI_STATUS_CAP_LIST); >>>>>> >>>>>> >>>>>> The prototypes would look like this: >>>>>> >>>>>> /* Set PCI config value. */ >>>>>> void pci_set_word(PCIDevice *s, uint8_t offset, uint16_t val); >>>>>> >>>>>> /* Set PCI cmask value. */ >>>>>> void pci_set_cmask_word(PCIDevice *s, uint8_t offset, uint16_t val); >>>>>> >>>>>> /* Set PCI wmask value. */ >>>>>> void pci_set_wmask_word(PCIDevice *s, uint8_t offset, uint16_t val); >>>>>> >>>>>> What are the advantages? >>>>>> * strict type checking (the old API takes any uint8_t *) >>>>>> >>>>> So IMO it's easier to make mistakes with your proposed API because if >>>>> you confuse offset and value compiler does not complain. More >>>>> importantly, if you want to pass some other uint8_t pointer to e.g. >>>>> pci_set_word, you can *and nothing unexpected will happen* >>>>> it will set the word to the given value. So the current >>>>> API is more type safe than what you propose. >>>>> >>>>> >>>> No. The current API takes any uint8_t pointer to read or write >>>> a value. This is not safe. >>>> >>> Why isn't it? >>> >> It is not safe, because it allows programmers to write silly code >> like these examples: >> >> pci_set_word(&gen_opc_cc_op[PCI_STATUS], PCI_STATUS_CAP_LIST); >> > This might be valid and useful for all I know. > > >> pci_set_word(&pci_conf[UINT32_MAX], PCI_STATUS_CAP_LIST); >> >> for (i = 0; i< sizeof(pci_conf); i++) { >> pci_set_long(&pci_conf[i], 0); >> } >> >> All three will result in runtime failures which can be very >> difficult to detect. >> > In theory. In practice I expect pci_set_word(pci_dev, 0x0, PCI_STATUS) > to be much more common with your proposed API. Just look how > common swapping memset parameters is. > > >>> >>>> The proposed API only takes a PCIDevice pointer >>>> and reads or writes only configuration (or cmask or >>>> wmask) values. Yes, you can take offset for value. >>>> If you are lucky and value is an uint16_t or uint32_t, >>>> your compiler will complain. >>>> >>> Such a compiler will also complain over most of qemu code. >>> >> Yes, but it's good to see that QEMU's code is >> improving. >> >> By the way, such a compiler is gcc when called with >> -Wconversion, so it is easy to see how much code >> is affected. >> > Didn't try it. So it will warn on > pci_set_word(pci_dev, 0x0, PCI_STATUS) > but not > pci_set_word(pci_dev, PCI_STATUS, 0x0) > ? > I haven't read this whole thread, but I really prefer things like pci_set_vendor_id(pci_dev, XXXX); A close alternative, would be some refactoring to allow PCI config space to be represented as a C structure. Gerd had some patches at one point for this. Regards, Anthony Liguori