Re: [Qemu-devel] [PATCH 1/9] Add stub functions for PCI device models to do PCI DMA

[Avi Kivity <avi@redhat.com>]

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<david@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).

I think endian conversion is best made explicit in virtio (like e1000 
does explicit conversions to little endian).

-- 
error compiling committee.c: too many arguments to function