Re: [Qemu-devel] [PATCH for-2.8] dma/rc4030: translate memory accesses only when they occur

[Yongbok Kim]

On 10/03/2017 18:31, Hervé Poussineau wrote:
> Ping again.
> 
> Le 26/02/2017 à 20:19, Hervé Poussineau a écrit :
>> Ping again.
>>
>> Le 22/02/2017 à 07:19, Hervé Poussineau a écrit :
>>> Ping?
>>>
>>> Le 18/11/2016 à 23:43, Hervé Poussineau a écrit :
>>>> This simplifies the code a lot, and this fixes big memory leaks
>>>> introduced in a3d586f704609a45b6037534cb2f34da5dfd8895
>>>>
>>>> Windows NT is now able to boot without using gigabytes of ram on the host.
>>>>
>>>> Signed-off-by: Hervé Poussineau <address@hidden>
>>>> ---
>>>>  hw/dma/rc4030.c | 158
>>>> +++++++++++++-------------------------------------------
>>>>  1 file changed, 36 insertions(+), 122 deletions(-)
>>>>
>>>> diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
>>>> index 41fc043..5f10b9d 100644
>>>> --- a/hw/dma/rc4030.c
>>>> +++ b/hw/dma/rc4030.c
>>>> @@ -34,8 +34,6 @@
>>>>  /********************************************************/
>>>>  /* rc4030 emulation                                     */
>>>>
>>>> -#define MAX_TL_ENTRIES 512
>>>> -
>>>>  typedef struct dma_pagetable_entry {
>>>>      int32_t frame;
>>>>      int32_t owner;
>>>> @@ -91,14 +89,8 @@ typedef struct rc4030State
>>>>      qemu_irq timer_irq;
>>>>      qemu_irq jazz_bus_irq;
>>>>
>>>> -    /* biggest translation table */
>>>> -    MemoryRegion dma_tt;
>>>> -    /* translation table memory region alias, added to system RAM */
>>>> -    MemoryRegion dma_tt_alias;
>>>>      /* whole DMA memory region, root of DMA address space */
>>>>      MemoryRegion dma_mr;
>>>> -    /* translation table entry aliases, added to DMA memory region */
>>>> -    MemoryRegion dma_mrs[MAX_TL_ENTRIES];
>>>>      AddressSpace dma_as;
>>>>
>>>>      MemoryRegion iomem_chipset;
>>>> @@ -256,96 +248,6 @@ static uint64_t rc4030_read(void *opaque, hwaddr
>>>> addr, unsigned int size)
>>>>      return val;
>>>>  }
>>>>
>>>> -static void rc4030_dma_as_update_one(rc4030State *s, int index,
>>>> uint32_t frame)
>>>> -{
>>>> -    if (index < MAX_TL_ENTRIES) {
>>>> -        memory_region_set_enabled(&s->dma_mrs[index], false);
>>>> -    }
>>>> -
>>>> -    if (!frame) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    if (index >= MAX_TL_ENTRIES) {
>>>> -        qemu_log_mask(LOG_UNIMP,
>>>> -                      "rc4030: trying to use too high "
>>>> -                      "translation table entry %d (max allowed=%d)",
>>>> -                      index, MAX_TL_ENTRIES);
>>>> -        return;
>>>> -    }
>>>> -    memory_region_set_alias_offset(&s->dma_mrs[index], frame);
>>>> -    memory_region_set_enabled(&s->dma_mrs[index], true);
>>>> -}
>>>> -
>>>> -static void rc4030_dma_tt_write(void *opaque, hwaddr addr, uint64_t data,
>>>> -                                unsigned int size)
>>>> -{
>>>> -    rc4030State *s = opaque;
>>>> -
>>>> -    /* write memory */
>>>> -    memcpy(memory_region_get_ram_ptr(&s->dma_tt) + addr, &data, size);
>>>> -
>>>> -    /* update dma address space (only if frame field has been written) */
>>>> -    if (addr % sizeof(dma_pagetable_entry) == 0) {
>>>> -        int index = addr / sizeof(dma_pagetable_entry);
>>>> -        memory_region_transaction_begin();
>>>> -        rc4030_dma_as_update_one(s, index, (uint32_t)data);
>>>> -        memory_region_transaction_commit();
>>>> -    }
>>>> -}
>>>> -
>>>> -static const MemoryRegionOps rc4030_dma_tt_ops = {
>>>> -    .write = rc4030_dma_tt_write,
>>>> -    .impl.min_access_size = 4,
>>>> -    .impl.max_access_size = 4,
>>>> -};
>>>> -
>>>> -static void rc4030_dma_tt_update(rc4030State *s, uint32_t new_tl_base,
>>>> -                                 uint32_t new_tl_limit)
>>>> -{
>>>> -    int entries, i;
>>>> -    dma_pagetable_entry *dma_tl_contents;
>>>> -
>>>> -    if (s->dma_tl_limit) {
>>>> -        /* write old dma tl table to physical memory */
>>>> -        memory_region_del_subregion(get_system_memory(),
>>>> &s->dma_tt_alias);
>>>> -        cpu_physical_memory_write(s->dma_tl_limit & 0x7fffffff,
>>>> -                                  memory_region_get_ram_ptr(&s->dma_tt),
>>>> -                                  memory_region_size(&s->dma_tt_alias));
>>>> -    }
>>>> -    object_unparent(OBJECT(&s->dma_tt_alias));
>>>> -
>>>> -    s->dma_tl_base = new_tl_base;
>>>> -    s->dma_tl_limit = new_tl_limit;
>>>> -    new_tl_base &= 0x7fffffff;
>>>> -
>>>> -    if (s->dma_tl_limit) {
>>>> -        uint64_t dma_tt_size;
>>>> -        if (s->dma_tl_limit <= memory_region_size(&s->dma_tt)) {
>>>> -            dma_tt_size = s->dma_tl_limit;
>>>> -        } else {
>>>> -            dma_tt_size = memory_region_size(&s->dma_tt);
>>>> -        }
>>>> -        memory_region_init_alias(&s->dma_tt_alias, OBJECT(s),
>>>> -                                 "dma-table-alias",
>>>> -                                 &s->dma_tt, 0, dma_tt_size);
>>>> -        dma_tl_contents = memory_region_get_ram_ptr(&s->dma_tt);
>>>> -        cpu_physical_memory_read(new_tl_base, dma_tl_contents,
>>>> dma_tt_size);
>>>> -
>>>> -        memory_region_transaction_begin();
>>>> -        entries = dma_tt_size / sizeof(dma_pagetable_entry);
>>>> -        for (i = 0; i < entries; i++) {
>>>> -            rc4030_dma_as_update_one(s, i, dma_tl_contents[i].frame);
>>>> -        }
>>>> -        memory_region_add_subregion(get_system_memory(), new_tl_base,
>>>> -                                    &s->dma_tt_alias);
>>>> -        memory_region_transaction_commit();
>>>> -    } else {
>>>> -        memory_region_init(&s->dma_tt_alias, OBJECT(s),
>>>> -                           "dma-table-alias", 0);
>>>> -    }
>>>> -}
>>>> -
>>>>  static void rc4030_write(void *opaque, hwaddr addr, uint64_t data,
>>>>                           unsigned int size)
>>>>  {
>>>> @@ -362,11 +264,11 @@ static void rc4030_write(void *opaque, hwaddr
>>>> addr, uint64_t data,
>>>>          break;
>>>>      /* DMA transl. table base */
>>>>      case 0x0018:
>>>> -        rc4030_dma_tt_update(s, val, s->dma_tl_limit);
>>>> +        s->dma_tl_base = val;
>>>>          break;
>>>>      /* DMA transl. table limit */
>>>>      case 0x0020:
>>>> -        rc4030_dma_tt_update(s, s->dma_tl_base, val);
>>>> +        s->dma_tl_limit = val;
>>>>          break;
>>>>      /* DMA transl. table invalidated */
>>>>      case 0x0028:
>>>> @@ -586,6 +488,38 @@ static const MemoryRegionOps jazzio_ops = {
>>>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>>>  };
>>>>
>>>> +static IOMMUTLBEntry rc4030_dma_translate(MemoryRegion *iommu, hwaddr
>>>> addr,
>>>> +                                          bool is_write)
>>>> +{
>>>> +    rc4030State *s = container_of(iommu, rc4030State, dma_mr);
>>>> +    IOMMUTLBEntry ret = {
>>>> +        .target_as = &address_space_memory,
>>>> +        .iova = addr & ~(DMA_PAGESIZE - 1),
>>>> +        .translated_addr = 0,
>>>> +        .addr_mask = DMA_PAGESIZE - 1,
>>>> +        .perm = IOMMU_NONE,
>>>> +    };
>>>> +    uint64_t i, entry_address;
>>>> +    dma_pagetable_entry entry;
>>>> +
>>>> +    i = addr / DMA_PAGESIZE;
>>>> +    if (i < s->dma_tl_limit / sizeof(entry)) {
>>>> +        entry_address = (s->dma_tl_base & 0x7fffffff) + i *
>>>> sizeof(entry);
>>>> +        if (address_space_read(ret.target_as, entry_address,
>>>> +                               MEMTXATTRS_UNSPECIFIED, (unsigned char
>>>> *)&entry,
>>>> +                               sizeof(entry)) == MEMTX_OK) {
>>>> +            ret.translated_addr = entry.frame & ~(DMA_PAGESIZE - 1);
>>>> +            ret.perm = IOMMU_RW;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const MemoryRegionIOMMUOps rc4030_dma_ops = {
>>>> +    .translate = rc4030_dma_translate,
>>>> +};
>>>> +
>>>>  static void rc4030_reset(DeviceState *dev)
>>>>  {
>>>>      rc4030State *s = RC4030(dev);
>>>> @@ -596,7 +530,6 @@ static void rc4030_reset(DeviceState *dev)
>>>>      s->invalid_address_register = 0;
>>>>
>>>>      memset(s->dma_regs, 0, sizeof(s->dma_regs));
>>>> -    rc4030_dma_tt_update(s, 0, 0);
>>>>
>>>>      s->remote_failed_address = s->memory_failed_address = 0;
>>>>      s->cache_maint = 0;
>>>> @@ -735,7 +668,6 @@ static void rc4030_realize(DeviceState *dev, Error
>>>> **errp)
>>>>  {
>>>>      rc4030State *s = RC4030(dev);
>>>>      Object *o = OBJECT(dev);
>>>> -    int i;
>>>>
>>>>      s->periodic_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>>>                                       rc4030_periodic_timer, s);
>>>> @@ -745,37 +677,19 @@ static void rc4030_realize(DeviceState *dev,
>>>> Error **errp)
>>>>      memory_region_init_io(&s->iomem_jazzio, NULL, &jazzio_ops, s,
>>>>                            "rc4030.jazzio", 0x00001000);
>>>>
>>>> -    memory_region_init_rom_device(&s->dma_tt, o,
>>>> -                                  &rc4030_dma_tt_ops, s, "dma-table",
>>>> -                                  MAX_TL_ENTRIES *
>>>> sizeof(dma_pagetable_entry),
>>>> -                                  NULL);
>>>> -    memory_region_init(&s->dma_tt_alias, o, "dma-table-alias", 0);
>>>> -    memory_region_init(&s->dma_mr, o, "dma", INT32_MAX);
>>>> -    for (i = 0; i < MAX_TL_ENTRIES; ++i) {
>>>> -        memory_region_init_alias(&s->dma_mrs[i], o, "dma-alias",
>>>> -                                 get_system_memory(), 0, DMA_PAGESIZE);
>>>> -        memory_region_set_enabled(&s->dma_mrs[i], false);
>>>> -        memory_region_add_subregion(&s->dma_mr, i * DMA_PAGESIZE,
>>>> -                                    &s->dma_mrs[i]);
>>>> -    }
>>>> +    memory_region_init_iommu(&s->dma_mr, o, &rc4030_dma_ops,
>>>> +                             "rc4030.dma", UINT32_MAX);
>>>>      address_space_init(&s->dma_as, &s->dma_mr, "rc4030-dma");
>>>>  }
>>>>
>>>>  static void rc4030_unrealize(DeviceState *dev, Error **errp)
>>>>  {
>>>>      rc4030State *s = RC4030(dev);
>>>> -    int i;
>>>>
>>>>      timer_free(s->periodic_timer);
>>>>
>>>>      address_space_destroy(&s->dma_as);
>>>> -    object_unparent(OBJECT(&s->dma_tt));
>>>> -    object_unparent(OBJECT(&s->dma_tt_alias));
>>>>      object_unparent(OBJECT(&s->dma_mr));
>>>> -    for (i = 0; i < MAX_TL_ENTRIES; ++i) {
>>>> -        memory_region_del_subregion(&s->dma_mr, &s->dma_mrs[i]);
>>>> -        object_unparent(OBJECT(&s->dma_mrs[i]));
>>>> -    }
>>>>  }
>>>>
>>>>  static void rc4030_class_init(ObjectClass *klass, void *class_data)
>>>>
>>>
>>
> 

Reviewed-by: Yongbok Kim <address@hidden>

Regards,
Yongbok