From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54585) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RAK1n-00084N-Bb for qemu-devel@nongnu.org; Sun, 02 Oct 2011 07:16:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RAK1k-0002y9-Im for qemu-devel@nongnu.org; Sun, 02 Oct 2011 07:16:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56251) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RAK1k-0002xd-BR for qemu-devel@nongnu.org; Sun, 02 Oct 2011 07:16:12 -0400 Date: Sun, 2 Oct 2011 13:17:00 +0200 From: "Michael S. Tsirkin" Message-ID: <20111002111700.GF30747@redhat.com> References: <1315197304-22469-1-git-send-email-david@gibson.dropbear.id.au> <1315197304-22469-2-git-send-email-david@gibson.dropbear.id.au> <20111002102547.GC30747@redhat.com> <4E883CF4.6060606@redhat.com> <20111002105238.GE30747@redhat.com> <4E8843DB.1020404@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4E8843DB.1020404@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: aliguori@us.ibm.com, joerg.roedel@amd.com, qemu-devel@nongnu.org, agraf@suse.de, kraxel@redhat.com, eduard.munteanu@linux360.ro, David Gibson , rth@twiddle.net On Sun, Oct 02, 2011 at 12:58:35PM +0200, Avi Kivity wrote: > On 10/02/2011 12:52 PM, Michael S. Tsirkin wrote: > >On Sun, Oct 02, 2011 at 12:29:08PM +0200, Avi Kivity wrote: > >> On 10/02/2011 12:25 PM, Michael S. Tsirkin wrote: > >> >On Mon, Sep 05, 2011 at 02:34:56PM +1000, David Gibson wrote: > >> >> This patch adds functions to pci.[ch] to perform PCI DMA operations. At > >> >> present, these are just stubs which perform directly cpu physical memory > >> >> accesses. > >> >> > >> >> Using these stubs, however, distinguishes PCI device DMA transactions from > >> >> other accesses to physical memory, which will allow PCI IOMMU support to > >> >> be added in one place, rather than updating every PCI driver at that time. > >> >> > >> >> That is, it allows us to update individual PCI drivers to support an IOMMU > >> >> without having yet determined the details of how the IOMMU emulation will > >> >> operate. This will let us remove the most bitrot-sensitive part of an > >> >> IOMMU patch in advance. > >> >> > >> >> Signed-off-by: David Gibson > >> > > >> >So something I just thought about: > >> > > >> >all wrappers now go through cpu_physical_memory_rw. > >> >This is a problem as e.g. virtio assumes that > >> >accesses such as stw are atomic. cpu_physical_memory_rw > >> >is a memcpy which makes no such guarantees. > >> > > >> > >> Let's change cpu_physical_memory_rw() to provide that guarantee for > >> aligned two and four byte accesses. Having separate paths just for > >> that is not maintainable. > > > >Well, we also have stX_phys convert to target native endian-ness > >(nop for KVM but not necessarily for qemu). > > > >So if we do what you suggest, this patch will become more correct, but > >it would still need to duplicate the endian-ness work. > > > >For that reason, I think calling stX_phys and friends from pci > >makes more sense - we get more simple inline wrappers > >but that code duplication worries me much less than tricky > >endian-ness hidden within a macro. > > > > Good point. Though this is really a virtio specific issue since > other devices have explicit endianness (not guest dependent). Hmm, not entirely virtio specific, some devices use stX macros to do the conversion. E.g. stw_be_phys and stl_le_phys are used in several places. > I think endian conversion is best made explicit in virtio (like > e1000 does explicit conversions to little endian). That's certainly possible. Though it's hard to see why duplicating e.g. static void e100_stw_le_phys(target_phys_addr_t addr, uint16_t val) { val = cpu_to_le16(val); cpu_physical_memory_write(addr, &val, sizeof(val)); } is a better idea than a central utility that does this. Maybe the address is not guaranteed to be aligned in the e100 case. > -- > error compiling committee.c: too many arguments to function