From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH 2/7] pci: memory access API and IOMMU support Date: Thu, 2 Sep 2010 09:00:46 +0300 Message-ID: <20100902060045.GA5410@redhat.com> References: <1283119703-9781-1-git-send-email-eduard.munteanu@linux360.ro> <4C7EB336.40003@mail.berlios.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Eduard - Gabriel Munteanu , kvm@vger.kernel.org, joro@8bytes.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, yamahata@valinux.co.jp, paul@codesourcery.com, avi@redhat.com To: Stefan Weil Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32915 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751383Ab0IBGHE (ORCPT ); Thu, 2 Sep 2010 02:07:04 -0400 Content-Disposition: inline In-Reply-To: <4C7EB336.40003@mail.berlios.de> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Sep 01, 2010 at 10:10:30PM +0200, Stefan Weil wrote: > >+static inline void pci_memory_read(PCIDevice *dev, > >+ pcibus_t addr, > >+ uint8_t *buf, > >+ pcibus_t len) > >+{ > >+ pci_memory_rw(dev, addr, buf, len, 0); > >+} > >+ > >+static inline void pci_memory_write(PCIDevice *dev, > >+ pcibus_t addr, > >+ const uint8_t *buf, > >+ pcibus_t len) > >+{ > >+ pci_memory_rw(dev, addr, (uint8_t *) buf, len, 1); > >+} > >+ > >#endif > > The functions pci_memory_read and pci_memory_write not only read > or write byte data but many different data types which leads to > a lot of type casts in your other patches. > > I'd prefer "void *buf" and "const void *buf" in the argument lists. > Then all those type casts could be removed. > > Regards > Stefan Weil Further, I am not sure pcibus_t is a good type to use here. This also forces use of pci specific types in e.g. ide, or resorting to casts as this patch does. We probably should use a more generic type for this. -- MST From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34499 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Or2wy-0007ML-3v for qemu-devel@nongnu.org; Thu, 02 Sep 2010 02:07:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Or2ww-0000vb-OI for qemu-devel@nongnu.org; Thu, 02 Sep 2010 02:07:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3657) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Or2ww-0000vP-H8 for qemu-devel@nongnu.org; Thu, 02 Sep 2010 02:07:02 -0400 Date: Thu, 2 Sep 2010 09:00:46 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH 2/7] pci: memory access API and IOMMU support Message-ID: <20100902060045.GA5410@redhat.com> References: <1283119703-9781-1-git-send-email-eduard.munteanu@linux360.ro> <4C7EB336.40003@mail.berlios.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4C7EB336.40003@mail.berlios.de> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Weil Cc: kvm@vger.kernel.org, joro@8bytes.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, yamahata@valinux.co.jp, paul@codesourcery.com, Eduard - Gabriel Munteanu , avi@redhat.com On Wed, Sep 01, 2010 at 10:10:30PM +0200, Stefan Weil wrote: > >+static inline void pci_memory_read(PCIDevice *dev, > >+ pcibus_t addr, > >+ uint8_t *buf, > >+ pcibus_t len) > >+{ > >+ pci_memory_rw(dev, addr, buf, len, 0); > >+} > >+ > >+static inline void pci_memory_write(PCIDevice *dev, > >+ pcibus_t addr, > >+ const uint8_t *buf, > >+ pcibus_t len) > >+{ > >+ pci_memory_rw(dev, addr, (uint8_t *) buf, len, 1); > >+} > >+ > >#endif > > The functions pci_memory_read and pci_memory_write not only read > or write byte data but many different data types which leads to > a lot of type casts in your other patches. > > I'd prefer "void *buf" and "const void *buf" in the argument lists. > Then all those type casts could be removed. > > Regards > Stefan Weil Further, I am not sure pcibus_t is a good type to use here. This also forces use of pci specific types in e.g. ide, or resorting to casts as this patch does. We probably should use a more generic type for this. -- MST