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

[Blue Swirl]

On Tue, Oct 30, 2012 at 8:03 PM, Benjamin Herrenschmidt
<address@hidden> wrote:
> 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.

Yes. Does the IOMMU even need to touch the data bus at all, because it
only translates address bus bits (ignoring caches/TLBs)?

>
> 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
>> >
>
>