Re: [Qemu-devel] [RFC PATCH 2/4] AMD IOMMU emulation

[Eduard - Gabriel Munteanu]

On Thu, Aug 05, 2010 at 09:31:58PM +0000, Blue Swirl wrote:
> On Wed, Aug 4, 2010 at 10:32 PM, Eduard - Gabriel Munteanu
> <address@hidden> wrote:

[snip]

> > diff --git a/Makefile.target b/Makefile.target
> > index 70a9c1b..86226a0 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -219,6 +219,8 @@ obj-i386-y += pcspk.o i8254.o
> > ??obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> > ??obj-i386-$(CONFIG_KVM_DEVICE_ASSIGNMENT) += device-assignment.o
> >
> > +obj-i386-$(CONFIG_AMD_IOMMU) += amd_iommu.o
> 
> Make this unconditional.
> 

[snip]

> 
> Drop all configure changes.
> 

Alright, so it's not going to be a compile-time configurable option.
I'll make some cmdline option for it and make really sure I don't mess
performance in hot paths.

(I'm actually happy to know it's gonna go in that way.)

[snip]

> > +struct amd_iommu_invalidator {
> > + ?? ??int ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? devfn;
> > + ?? ??PCIInvalidateIOTLBFunc ??*func;
> > + ?? ??void ?? ?? ?? ?? ?? ?? ?? ?? ?? ??*opaque;
> > + ?? ??QLIST_ENTRY(amd_iommu_invalidator) list;
> > +};
> 
> This should be AMDIOMMUInvalidator with typedef.
> 
> > +
> > +struct amd_iommu_state {

[snip]

> > +};
> 
> Likewise, AMDIOMMUState.
>

[snip]

> > +static int amd_iommu_translate(PCIIOMMU *iommu,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? PCIDevice *dev,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? pci_addr_t addr,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? target_phys_addr_t *paddr,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? int *len,
> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? unsigned perms);
> 
> Please move the implementation here to avoid this declaration.
> 

[snip]

> > + ?? ??iommu = qemu_mallocz(sizeof(PCIIOMMU));
> > + ?? ??iommu->opaque = st;
> > + ?? ??iommu->translate = amd_iommu_translate;
> > + ?? ??iommu->register_iotlb_invalidator = amd_iommu_register_invalidator;
> > + ?? ??pci_register_iommu(dev, iommu);
> 
> I'd avoid the structure and just pass the stuff to pci_register_iommu
> as function arguments.
> 

[snip]

> > +void amd_iommu_init(PCIBus *bus)
> > +{
> > + ?? ??pci_create_simple(bus, -1, "amd-iommu");
> > +}
> 
> Just open code this in pc.c.
> 

Roger, I'll fix these.

[snip]

> > ??#define PCI_VENDOR_ID_MOTOROLA ?? ?? ?? ?? ?? 0x1057
> > ??#define PCI_DEVICE_ID_MOTOROLA_MPC106 ?? ??0x0002
> > diff --git a/hw/pci_regs.h b/hw/pci_regs.h
> > index 1c675dc..6399b5d 100644
> > --- a/hw/pci_regs.h
> > +++ b/hw/pci_regs.h
> > @@ -216,6 +216,7 @@
> > ??#define ??PCI_CAP_ID_SHPC ?? ?? ?? 0x0C ?? ??/* PCI Standard Hot-Plug 
> > Controller */
> > ??#define ??PCI_CAP_ID_SSVID ?? ?? ??0x0D ?? ??/* Bridge subsystem 
> > vendor/device ID */
> > ??#define ??PCI_CAP_ID_AGP3 ?? ?? ?? 0x0E ?? ??/* AGP Target PCI-PCI bridge 
> > */
> > +#define ??PCI_CAP_ID_SEC ?? ?? 0x0F ?? ??/* Secure Device (AMD IOMMU) */
> 
> Indentation seems to be off.
> 
> > ??#define ??PCI_CAP_ID_EXP ?? ?? ?? ??0x10 ?? ??/* PCI Express */
> > ??#define ??PCI_CAP_ID_MSIX ?? ?? ?? 0x11 ?? ??/* MSI-X */
> > ??#define ??PCI_CAP_ID_AF ?? ?? ?? ?? 0x13 ?? ??/* PCI Advanced Features */
> > --
> > 1.7.1

The original has tabs instead of spaces, but my changes line up
properly. Which way should I go, convert it all to spaces, add my line
with tabs or leave it like this? Of course, any cleanup would go in a
separate patch.


        Thanks,
        Eduard