[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/7] memory: iommu support
From: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/7] memory: iommu support |
Date: |
Tue, 30 Oct 2012 21:13:53 +0000 |
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
>> >
>
>
[Qemu-devel] [PATCH v2 4/7] memory: provide a MemoryRegion for IOMMUs to log faults, Avi Kivity, 2012/10/30
[Qemu-devel] [PATCH v2 5/7] pci: use memory core for iommu support, Avi Kivity, 2012/10/30
[Qemu-devel] [PATCH v2 6/7] vfio: abort if an emulated iommu is used, Avi Kivity, 2012/10/30
[Qemu-devel] [PATCH v2 7/7] i440fx: add an iommu, Avi Kivity, 2012/10/30