Re: [Qemu-devel] [PATCH 03/66] dma: keep a device alive while it has SGLists

[Jan Kiszka]

On 2013-07-04 17:51, Jan Kiszka wrote:
> On 2013-07-04 17:12, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  dma-helpers.c         |  6 +++++-
>>  hw/ide/ahci.c         |  3 ++-
>>  hw/ide/macio.c        |  4 ++--
>>  hw/scsi/virtio-scsi.c | 10 ++++++----
>>  hw/usb/hcd-ehci.c     |  4 ++--
>>  include/hw/pci/pci.h  |  2 +-
>>  include/sysemu/dma.h  |  4 +++-
>>  7 files changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/dma-helpers.c b/dma-helpers.c
>> index 5afba47..499550f 100644
>> --- a/dma-helpers.c
>> +++ b/dma-helpers.c
>> @@ -34,13 +34,16 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, 
>> uint8_t c, dma_addr_t len)
>>      return error;
>>  }
>>  
>> -void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint, AddressSpace *as)
>> +void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
>> +                      AddressSpace *as)
>>  {
>>      qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>>      qsg->nsg = 0;
>>      qsg->nalloc = alloc_hint;
>>      qsg->size = 0;
>>      qsg->as = as;
>> +    qsg->dev = dev;
>> +    object_ref(OBJECT(dev));
>>  }
>>  
>>  void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
>> @@ -57,6 +60,7 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, 
>> dma_addr_t len)
>>  
>>  void qemu_sglist_destroy(QEMUSGList *qsg)
>>  {
>> +    object_unref(OBJECT(qsg->dev));
>>      g_free(qsg->sg);
>>      memset(qsg, 0, sizeof(*qsg));
>>  }
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 1adfa0b..6b87549 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -691,7 +691,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, 
>> QEMUSGList *sglist, int offset)
>>              goto out;
>>          }
>>  
>> -        qemu_sglist_init(sglist, (sglist_alloc_hint - off_idx), 
>> ad->hba->as);
>> +        qemu_sglist_init(sglist, DEVICE(ad->hba), (sglist_alloc_hint - 
>> off_idx),
>> +                         ad->hba->as);
>>          qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_pos),
>>                          le32_to_cpu(tbl[off_idx].flags_size) + 1 - off_pos);
>>  
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index a1952b0..84288ae 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -70,7 +70,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int 
>> ret)
>>  
>>      s->io_buffer_size = io->len;
>>  
>> -    qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>> +    qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
>>                       &address_space_memory);
>>      qemu_sglist_add(&s->sg, io->addr, io->len);
>>      io->addr += io->len;
>> @@ -127,7 +127,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>>      s->io_buffer_index = 0;
>>      s->io_buffer_size = io->len;
>>  
>> -    qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>> +    qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
>>                       &address_space_memory);
>>      qemu_sglist_add(&s->sg, io->addr, io->len);
>>      io->addr += io->len;
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index b8a0abf..712f0ad 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -77,10 +77,12 @@ static void virtio_scsi_bad_req(void)
>>      exit(1);
>>  }
>>  
>> -static void qemu_sgl_init_external(QEMUSGList *qsgl, struct iovec *sg,
>> +static void qemu_sgl_init_external(VirtIOSCSIReq *req, struct iovec *sg,
>>                                     hwaddr *addr, int num)
>>  {
>> -    qemu_sglist_init(qsgl, num, &address_space_memory);
>> +    QEMUSGList *qsgl = &req->qsgl;
>> +
>> +    qemu_sglist_init(qsgl, DEVICE(req->dev), num, &address_space_memory);
>>      while (num--) {
>>          qemu_sglist_add(qsgl, *(addr++), (sg++)->iov_len);
>>      }
>> @@ -99,11 +101,11 @@ static void virtio_scsi_parse_req(VirtIOSCSI *s, 
>> VirtQueue *vq,
>>      req->resp.buf = req->elem.in_sg[0].iov_base;
>>  
>>      if (req->elem.out_num > 1) {
>> -        qemu_sgl_init_external(&req->qsgl, &req->elem.out_sg[1],
>> +        qemu_sgl_init_external(req, &req->elem.out_sg[1],
>>                                 &req->elem.out_addr[1],
>>                                 req->elem.out_num - 1);
>>      } else {
>> -        qemu_sgl_init_external(&req->qsgl, &req->elem.in_sg[1],
>> +        qemu_sgl_init_external(req, &req->elem.in_sg[1],
>>                                 &req->elem.in_addr[1],
>>                                 req->elem.in_num - 1);
>>      }
>> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
>> index 16d6356..65e6680 100644
>> --- a/hw/usb/hcd-ehci.c
>> +++ b/hw/usb/hcd-ehci.c
>> @@ -1245,7 +1245,7 @@ static int ehci_init_transfer(EHCIPacket *p)
>>      cpage  = get_field(p->qtd.token, QTD_TOKEN_CPAGE);
>>      bytes  = get_field(p->qtd.token, QTD_TOKEN_TBYTES);
>>      offset = p->qtd.bufptr[0] & ~QTD_BUFPTR_MASK;
>> -    qemu_sglist_init(&p->sgl, 5, p->queue->ehci->as);
>> +    qemu_sglist_init(&p->sgl, DEVICE(p->queue->ehci), 5, 
>> p->queue->ehci->as);
> 
> I was just testing this from your current git tree, and it causes
> 
> qemu/hw/usb/hcd-ehci.c:1248:ehci_init_transfer: Object 0x7f3eb39c6500 is
> not an instance of type device
> 
> when booting a q35 machine (as it pulls in EHCI). The cast is obviously
> wrong, but I didn't spot yet what is required instead.

The problem is that EHCIState is embedded into the actual QOM device
states, but not at a known location. So you likely need a generic device
pointer from EHCIState back to the device using it.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux