On 27/11/2018 18:53, Michael S. Tsirkin wrote: > On Tue, Nov 27, 2018 at 06:10:46PM +0000, Jean-Philippe Brucker wrote: >> On 27/11/2018 18:04, Michael S. Tsirkin wrote: >>> On Tue, Nov 27, 2018 at 05:50:50PM +0000, Jean-Philippe Brucker wrote: >>>> On 23/11/2018 22:02, Michael S. Tsirkin wrote: >>>>>> +/* >>>>>> + * __viommu_sync_req - Complete all in-flight requests >>>>>> + * >>>>>> + * Wait for all added requests to complete. When this function returns, all >>>>>> + * requests that were in-flight at the time of the call have completed. >>>>>> + */ >>>>>> +static int __viommu_sync_req(struct viommu_dev *viommu) >>>>>> +{ >>>>>> + int ret = 0; >>>>>> + unsigned int len; >>>>>> + size_t write_len; >>>>>> + struct viommu_request *req; >>>>>> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ]; >>>>>> + >>>>>> + assert_spin_locked(&viommu->request_lock); >>>>>> + >>>>>> + virtqueue_kick(vq); >>>>>> + >>>>>> + while (!list_empty(&viommu->requests)) { >>>>>> + len = 0; >>>>>> + req = virtqueue_get_buf(vq, &len); >>>>>> + if (!req) >>>>>> + continue; >>>>>> + >>>>>> + if (!len) >>>>>> + viommu_set_req_status(req->buf, req->len, >>>>>> + VIRTIO_IOMMU_S_IOERR); >>>>>> + >>>>>> + write_len = req->len - req->write_offset; >>>>>> + if (req->writeback && len == write_len) >>>>>> + memcpy(req->writeback, req->buf + req->write_offset, >>>>>> + write_len); >>>>>> + >>>>>> + list_del(&req->list); >>>>>> + kfree(req); >>>>>> + } >>>>> >>>>> I didn't notice this in the past but it seems this will spin >>>>> with interrupts disabled until host handles the request. >>>>> Please do not do this - host execution can be another >>>>> task that needs the same host CPU. This will then disable >>>>> interrupts for a very very long time. >>>> >>>> In the guest yes, but that doesn't prevent the host from running another >>>> task right? >>> >>> Doesn't prevent it but it will delay it significantly >>> until scheduler decides to kick the VCPU task out. >>> >>>> My tests run fine when QEMU is bound to a single CPU, even >>>> though vcpu and viommu run in different threads >>>> >>>>> What to do then? Queue in software and wake up task. >>>> >>>> Unfortunately I can't do anything here, because IOMMU drivers can't >>>> sleep in the iommu_map() or iommu_unmap() path. >>>> >>>> The problem is the same >>>> for all IOMMU drivers. That's because the DMA API allows drivers to call >>>> some functions with interrupts disabled. For example >>>> Documentation/DMA-API-HOWTO.txt allows dma_alloc_coherent() and >>>> dma_unmap_single() to be called in interrupt context. >>> >>> In fact I don't really understand how it's supposed to >>> work at all: you only sync when ring is full. >>> So host may not have seen your map request if ring >>> is not full. >>> Why is it safe to use the address with a device then? >> >> viommu_map() calls viommu_send_req_sync(), which does the sync >> immediately after adding the MAP request. >> >> Thanks, >> Jean > > I see. So it happens on every request. Maybe you should clear > event index then. This way if exits are disabled you know that > host is processing the ring. Event index is good for when > you don't care when it will be processed, you just want > to reduce number of exits as much as possible. > I think that's already the case: since we don't attach a callback to the request queue, VRING_AVAIL_F_NO_INTERRUPT is set in avail_flags_shadow, which causes the used event index to stay clear. Thanks, Jean _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm