Re: [Qemu-devel] [PATCH v2 3/7] memory: iommu support

[Benjamin Herrenschmidt]

On Tue, 2012-10-30 at 19:11 +0000, Blue Swirl wrote:

> Why couple this with host endianness? I'd expect IOMMU to operate at
> target bus endianness, for example LE for PCI on PPC guest.

I'm not sure about putting the iommu "in charge" of endianness ...

On one hand it's fishy. It should be 'transparent', the device is what
controls its own endianness and playing games at the iommu level is
going to result into tears... on the other hand I can see the appeal of
not bothering at the device level and letting the iommu do it for you...
but I still think it's risky.

Besides not all PCI devices are little endian :-) Also that won't deal
with map/unmap etc...

So I'd vote for sanity here and make the iommu not affect endianness in
any way and let the devices do the "right thing" as is the expectation
today.

Ben.

> > +#else
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +#endif
> > +};
> > +
> > +void memory_region_init_iommu(MemoryRegion *mr,
> > +                              MemoryRegionIOMMUOps *ops,
> > +                              MemoryRegion *target,
> > +                              const char *name,
> > +                              uint64_t size)
> > +{
> > +    memory_region_init(mr, name, size);
> > +    mr->ops = &memory_region_iommu_ops;
> > +    mr->iommu_ops = ops,
> > +    mr->opaque = mr;
> > +    mr->terminates = true;  /* then re-forwards */
> > +    mr->destructor = memory_region_destructor_iommu;
> > +    mr->iommu_target_as = g_new(AddressSpace, 1);
> > +    address_space_init(mr->iommu_target_as, target);
> > +}
> > +
> >  static uint64_t invalid_read(void *opaque, hwaddr addr,
> >                               unsigned size)
> >  {
> > @@ -1054,6 +1155,11 @@ bool memory_region_is_rom(MemoryRegion *mr)
> >      return mr->ram && mr->readonly;
> >  }
> >
> > +bool memory_region_is_iommu(MemoryRegion *mr)
> > +{
> > +    return mr->iommu_ops;
> > +}
> > +
> >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> >  {
> >      uint8_t mask = 1 << client;
> > diff --git a/memory.h b/memory.h
> > index 9462bfd..47362c9 100644
> > --- a/memory.h
> > +++ b/memory.h
> > @@ -113,12 +113,28 @@ struct MemoryRegionOps {
> >      const MemoryRegionMmio old_mmio;
> >  };
> >
> > +typedef struct IOMMUTLBEntry IOMMUTLBEntry;
> > +typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;
> > +
> > +struct IOMMUTLBEntry {
> > +    hwaddr device_addr;
> > +    hwaddr translated_addr;
> > +    hwaddr addr_mask;  /* 0xfff = 4k translation */
> > +    bool perm[2]; /* read/write permissions */
> 
> Please document that bit 1 is write and 0 read.
> 
> > +};
> > +
> > +struct MemoryRegionIOMMUOps {
> > +    /* Returns a TLB entry that contains a given address. */
> > +    IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr);
> 
> Maybe the MemoryRegion could be const to declare that the translation
> does not change mappings.
> 
> > +};
> > +
> >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> >
> >  struct MemoryRegion {
> >      /* All fields are private - violators will be prosecuted */
> >      const MemoryRegionOps *ops;
> > +    const MemoryRegionIOMMUOps *iommu_ops;
> >      void *opaque;
> >      MemoryRegion *parent;
> >      Int128 size;
> > @@ -145,6 +161,7 @@ struct MemoryRegion {
> >      uint8_t dirty_log_mask;
> >      unsigned ioeventfd_nb;
> >      MemoryRegionIoeventfd *ioeventfds;
> > +    struct AddressSpace *iommu_target_as;
> >  };
> >
> >  struct MemoryRegionPortio {
> > @@ -334,6 +351,25 @@ void memory_region_init_rom_device(MemoryRegion *mr,
> >  void memory_region_init_reservation(MemoryRegion *mr,
> >                                      const char *name,
> >                                      uint64_t size);
> > +
> > +/**
> > + * memory_region_init_iommu: Initialize a memory region that translates 
> > addresses
> > + *
> > + * An IOMMU region translates addresses and forwards accesses to a target 
> > memory region.
> > + *
> > + * @mr: the #MemoryRegion to be initialized
> > + * @ops: a function that translates addresses into the @target region
> > + * @target: a #MemoryRegion that will be used to satisfy accesses to 
> > translated
> > + *          addresses
> > + * @name: used for debugging; not visible to the user or ABI
> > + * @size: size of the region.
> > + */
> > +void memory_region_init_iommu(MemoryRegion *mr,
> > +                              MemoryRegionIOMMUOps *ops,
> > +                              MemoryRegion *target,
> > +                              const char *name,
> > +                              uint64_t size);
> > +
> >  /**
> >   * memory_region_destroy: Destroy a memory region and reclaim all 
> > resources.
> >   *
> > @@ -373,6 +409,15 @@ static inline bool memory_region_is_romd(MemoryRegion 
> > *mr)
> >  }
> >
> >  /**
> > + * memory_region_is_ram: check whether a memory region is an iommu
> > + *
> > + * Returns %true is a memory region is an iommu.
> > + *
> > + * @mr: the memory region being queried
> > + */
> > +bool memory_region_is_iommu(MemoryRegion *mr);
> > +
> > +/**
> >   * memory_region_name: get a memory region's name
> >   *
> >   * Returns the string that was used to initialize the memory region.
> > --
> > 1.7.12
> >