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

[Marcel Apfelbaum <marcel@redhat.com>]

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 <davidkiarie4@gmail.com>
>>> ---
>

[...]

>>> +
>>> +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);

[...]