Re: [Qemu-devel] [RFC PATCH 1/4] pci: memory access API and IOMMU support

[Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>]

On Thu, Aug 05, 2010 at 09:23:30PM +0000, Blue Swirl wrote:
> On Wed, Aug 4, 2010 at 10:32 PM, Eduard - Gabriel Munteanu

[snip]

> > @@ -58,6 +58,10 @@ struct PCIBus {
> > ?? ?? ?? ??Keep a count of the number of devices with raised IRQs. ??*/
> > ?? ?? int nirq;
> > ?? ?? int *irq_count;
> > +
> > +#ifdef CONFIG_PCI_IOMMU
> 
> The code should not be conditional.
> 
> > + ?? ??PCIIOMMU *iommu;
> > +#endif
> > ??};
> >
> > ??static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
> > @@ -2029,6 +2033,147 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> > ?? ?? }
> > ??}
> >
> > +#ifdef CONFIG_PCI_IOMMU
> > +
> > +void pci_register_iommu(PCIDevice *dev, PCIIOMMU *iommu)
> > +{
> > + ?? ??dev->bus->iommu = iommu;
> > +}
> > +
> > +void pci_memory_rw(PCIDevice *dev,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t addr,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? uint8_t *buf,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t len,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? int is_write)
> > +{
> > + ?? ??int err, plen;
> > + ?? ??unsigned perms;
> > + ?? ??PCIIOMMU *iommu = dev->bus->iommu;
> > + ?? ??target_phys_addr_t paddr;
> > +
> > + ?? ??if (!iommu || !iommu->translate)
> > + ?? ?? ?? ??return cpu_physical_memory_rw(addr, buf, len, is_write);
> 
> Instead of these kind of checks, please add default handlers which
> call cpu_physical_memory_rw() etc.
> 

Ok. I'm trying to minimize impact (non-inlineable function calls) when
the IOMMU is disabled at compile-time. I think I can do it some other
way, as you suggest.

> > +
> > + ?? ??perms = is_write ? IOMMU_PERM_WRITE : IOMMU_PERM_READ;
> 
> Is this useful? How about just passing is_write as perms?
> 

Only in theory: it might come in handy if we ever support RW operations,
like read-modify-write memory maps. Also, write permissions include
zero-byte reads for the AMD IOMMU, so IOMMU_PERM_* could be further
refined.

I'm happy to remove it, though.

[snip]

> > +/*
> > + * Memory I/O and PCI IOMMU definitions.
> > + */
> > +
> > +typedef target_phys_addr_t pci_addr_t;
> 
> There is already pcibus_t.
> 

Thanks, I'll use that.

> > +
> > +typedef int PCIInvalidateIOTLBFunc(void *opaque);
> 
> I think some type safety tricks could be used with for example PCIDevice *.
> 

Note that 'opaque' belongs to the caller (the code that requests
memory maps).

Some device might make multiple maps that can be invalidated separately.
The actual stuff that describes the map might not be straightforward to
recover from a PCIDevice.

We could add another parameter to PCIInvalidateIOTLBFunc(), but since
the main user is DMA code, it's going to complicate things further.

[snip]

	Eduard