On Sun, Oct 02, 2011 at 02:01:10PM +0200, Avi Kivity wrote: > On 10/02/2011 01:17 PM, Michael S. Tsirkin wrote: > >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<da...@gibson.dropbear.id.au> > >> >> > > >> >> >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. > > These are fine - explicit endianness.
Right. So changing these to e.g. stl_dma and assuming LE is default seems like a step backwards. > >> 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. > > The general case is dma'ing a structure, not a single field. That > doesn't mean we shouldn't have a helper. > > -- > error compiling committee.c: too many arguments to function