Re: [Qemu-devel] [V6 1/4] hw/i386: Introduce AMD IOMMU

[Marcel Apfelbaum]

On 03/02/2016 09:11 PM, David Kiarie wrote:
> On 25/02/16 18:43, Marcel Apfelbaum wrote:
> > On 02/21/2016 08:10 PM, David Kiarie wrote:
> > > Add AMD IOMMU emulaton to Qemu in addition to Intel IOMMU
> > > The IOMMU does basic translation, error checking and has a
> > > mininal IOTLB implementation
> >
> > Hi,
> >
> > > Signed-off-by: David Kiarie <address@hidden>
> > > ---

[...]

> > > +
> > > +static void amd_iommu_realize(PCIDevice *dev, Error **error)
> > > +{
> > > +    AMDIOMMUState *s = container_of(dev, AMDIOMMUState, dev);
> > > +
> > > +    s->iotlb = g_hash_table_new_full(amd_iommu_uint64_hash,
> > > +                                     amd_iommu_uint64_equal, g_free, g_free);
> > > +
> > > +    s->capab_offset = pci_add_capability(dev, IOMMU_CAPAB_ID_SEC, 0,
> > > +                                         IOMMU_CAPAB_SIZE);
> > > +
> > > +    /* add msi and hypertransport capabilities */
> > > +    pci_add_capability(&s->dev, PCI_CAP_ID_MSI, 0, IOMMU_CAPAB_REG_SIZE);
> > > +    pci_add_capability(&s->dev, PCI_CAP_ID_HT, 0, IOMMU_CAPAB_REG_SIZE);
> > > +
> > > +    amd_iommu_init(s);
> > > +
> > > +    /* set up MMIO */
> > > +    memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "mmio",
> > > +                          IOMMU_MMIO_SIZE);
> > > +
> > > +    if (s->mmio.addr == IOMMU_BASE_ADDR) {
> >
> > I don't understand why is need here. realize is called only once in the init 
> > process
> > and you set it a few lines below.
> >
> > > +        return;
> > > +    }
> > > +
> > > +    s->mmio.addr = IOMMU_BASE_ADDR;
> > > +    memory_region_add_subregion(get_system_memory(), IOMMU_BASE_ADDR, 
> > > &s->mmio);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_amd_iommu = {
> > > +    .name = "amd-iommu",
> > > +    .fields  = (VMStateField[]) {
> > > +        VMSTATE_PCI_DEVICE(dev, AMDIOMMUState),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > > +
> > > +static Property amd_iommu_properties[] = {
> > > +    DEFINE_PROP_UINT32("version", AMDIOMMUState, version, 2),
> > > +    DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > +static void amd_iommu_uninit(PCIDevice *dev)
> > > +{
> > > +    AMDIOMMUState *s = container_of(dev, AMDIOMMUState, dev);
> > > +    amd_iommu_iotlb_reset(s);
> >
> > at this point you also need to clean also the memory regions you use.
>
> What exactly do you mean by clean up the memory regions ?

You have an memory_region_add_subregion on realize, you need
a memory_region_del_subregion on exit.

Thanks,
Marcel

> > > +}
> > > +
> > > +static void amd_iommu_class_init(ObjectClass *klass, void* data)
> > > +{
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > +
> > > +    k->realize = amd_iommu_realize;
> > > +    k->exit = amd_iommu_uninit;
> > > +
> > > +    dc->reset = amd_iommu_reset;
> > > +    dc->vmsd = &vmstate_amd_iommu;
> > > +    dc->props = amd_iommu_properties;
> > > +}
> > > +
> > > +static const TypeInfo amd_iommu = {
> > > +    .name = TYPE_AMD_IOMMU_DEVICE,
> > > +    .parent = TYPE_PCI_DEVICE,
> > > +    .instance_size = sizeof(AMDIOMMUState),
> > > +    .class_init = amd_iommu_class_init
> > > +};
> > > +
> > > +static void amd_iommu_register_types(void)
> > > +{
> > > +    type_register_static(&amd_iommu);
> > > +}
> > > +
> > > +type_init(amd_iommu_register_types);

[...]