On Tue, Oct 30, 2012 at 8:03 PM, Benjamin Herrenschmidt <b...@kernel.crashing.org> 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 >> > > >