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

[Michael S. Tsirkin]

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<address@hidden>
> >>  >
> >>  >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