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
> <eduard.munteanu@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
