* Re: [PATCH v5 5/7] iommu: Add virtio-iommu driver
From: Michael S. Tsirkin @ 2018-11-27 18:10 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: Mark Rutland, virtio-dev@lists.oasis-open.org, Lorenzo Pieralisi,
tnowicki@caviumnetworks.com, devicetree@vger.kernel.org,
Marc Zyngier, linux-pci@vger.kernel.org, joro@8bytes.org,
Will Deacon, virtualization@lists.linux-foundation.org,
eric.auger@redhat.com, iommu@lists.linux-foundation.org,
robh+dt@kernel.org, bhelgaas@google.com, Robin Murphy,
kvmarm@lists.cs.columbia.edu
In-Reply-To: <0fed79f3-ca2e-b539-74f5-cd2a8091f65d@arm.com>
On Tue, Nov 27, 2018 at 05:58:18PM +0000, Jean-Philippe Brucker wrote:
> On 23/11/2018 21:48, Michael S. Tsirkin wrote:
> >> +struct virtio_iommu_config {
> >> + /* Supported page sizes */
> >> + __u64 page_size_mask;
> >> + /* Supported IOVA range */
> >> + struct virtio_iommu_range input_range;
> >> + /* Max domain ID size */
> >> + __u8 domain_bits;
> >> + __u8 padding[3];
> >
> > Not enough padding here it seems. Structure is 8 byte
> > aligned on 64 bit systems.
>
> The next field (probe_size) is 4 bytes, so the alignment ends up fine.
> That field is introduced in patch 6, maybe I should move it here?
>
> Thanks,
> Jean
Sounds like a good idea.
--
MST
^ permalink raw reply
* Re: [PATCH v5 5/7] iommu: Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2018-11-27 18:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: mark.rutland, virtio-dev, lorenzo.pieralisi, tnowicki, devicetree,
marc.zyngier, linux-pci, joro, will.deacon, virtualization,
eric.auger, iommu, robh+dt, bhelgaas, robin.murphy, kvmarm
In-Reply-To: <20181127125424-mutt-send-email-mst@kernel.org>
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
^ permalink raw reply
* Re: [PATCH v5 5/7] iommu: Add virtio-iommu driver
From: Michael S. Tsirkin @ 2018-11-27 18:13 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: mark.rutland, virtio-dev, lorenzo.pieralisi, tnowicki, devicetree,
marc.zyngier, linux-pci, joro, will.deacon, virtualization,
eric.auger, iommu, robh+dt, bhelgaas, robin.murphy, kvmarm
In-Reply-To: <fd221210-7e12-1d15-0c80-52a761f6893d@arm.com>
On Tue, Nov 27, 2018 at 05:50:50PM +0000, Jean-Philippe Brucker wrote:
> > 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? My tests run fine when QEMU is bound to a single CPU, even
> though vcpu and viommu run in different threads
So a kind of a solution is to add a config space field for sync.
That at least can give host a hint that yes, vcpu
is stopped now and it should do something else.
Not ideal but better than polling VQ forever.
--
MST
^ permalink raw reply
* Re: [PATCH v5 5/7] iommu: Add virtio-iommu driver
From: Michael S. Tsirkin @ 2018-11-27 18:53 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: mark.rutland, virtio-dev, lorenzo.pieralisi, tnowicki, devicetree,
marc.zyngier, linux-pci, joro, will.deacon, virtualization,
eric.auger, iommu, robh+dt, bhelgaas, robin.murphy, kvmarm
In-Reply-To: <ef97313f-4795-9537-e0b8-8bf6751db6a6@arm.com>
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.
--
MST
^ permalink raw reply
* [PATCH 1/6] drm/qxl: move qxl_primary_apply_cursor to correct place
From: Gerd Hoffmann @ 2018-11-28 10:34 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: David Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU
In-Reply-To: <20181128103418.28023-1-kraxel@redhat.com>
The cursor must be set again after creating the primary surface.
Also drop the error message.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_display.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index ce0b9c40fc..58fb2c4308 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -533,7 +533,6 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
.x2 = plane->state->fb->width,
.y2 = plane->state->fb->height
};
- int ret;
bool same_shadow = false;
if (old_state->fb) {
@@ -554,16 +553,13 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
if (!same_shadow)
qxl_io_destroy_primary(qdev);
bo_old->is_primary = false;
-
- ret = qxl_primary_apply_cursor(plane);
- if (ret)
- DRM_ERROR(
- "could not set cursor after creating primary");
}
if (!bo->is_primary) {
- if (!same_shadow)
+ if (!same_shadow) {
qxl_io_create_primary(qdev, 0, bo);
+ qxl_primary_apply_cursor(plane);
+ }
bo->is_primary = true;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 2/6] drm/qxl: drop unused offset parameter from qxl_io_create_primary()
From: Gerd Hoffmann @ 2018-11-28 10:34 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: David Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU
In-Reply-To: <20181128103418.28023-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_drv.h | 1 -
drivers/gpu/drm/qxl/qxl_cmd.c | 7 +++----
drivers/gpu/drm/qxl/qxl_display.c | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 14d3fa8557..6b8d28a23f 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -395,7 +395,6 @@ void qxl_update_screen(struct qxl_device *qxl);
/* qxl io operations (qxl_cmd.c) */
void qxl_io_create_primary(struct qxl_device *qdev,
- unsigned int offset,
struct qxl_bo *bo);
void qxl_io_destroy_primary(struct qxl_device *qdev);
void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t id);
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index dffc5093ff..f263d83775 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -375,8 +375,7 @@ void qxl_io_destroy_primary(struct qxl_device *qdev)
qdev->primary_created = false;
}
-void qxl_io_create_primary(struct qxl_device *qdev,
- unsigned int offset, struct qxl_bo *bo)
+void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
{
struct qxl_surface_create *create;
@@ -387,9 +386,9 @@ void qxl_io_create_primary(struct qxl_device *qdev,
create->height = bo->surf.height;
create->stride = bo->surf.stride;
if (bo->shadow) {
- create->mem = qxl_bo_physical_address(qdev, bo->shadow, offset);
+ create->mem = qxl_bo_physical_address(qdev, bo->shadow, 0);
} else {
- create->mem = qxl_bo_physical_address(qdev, bo, offset);
+ create->mem = qxl_bo_physical_address(qdev, bo, 0);
}
DRM_DEBUG_DRIVER("mem = %llx, from %p\n", create->mem, bo->kptr);
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 58fb2c4308..d875cae02f 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -557,7 +557,7 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
if (!bo->is_primary) {
if (!same_shadow) {
- qxl_io_create_primary(qdev, 0, bo);
+ qxl_io_create_primary(qdev, bo);
qxl_primary_apply_cursor(plane);
}
bo->is_primary = true;
--
2.9.3
^ permalink raw reply related
* [PATCH 3/6] drm/qxl: track primary bo
From: Gerd Hoffmann @ 2018-11-28 10:34 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: David Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU
In-Reply-To: <20181128103418.28023-1-kraxel@redhat.com>
Track which bo is used as primary surface. With that in place we don't
need the primary_created flag any more, we can just check the primary bo
pointer instead.
Also verify we don't already have a primary surface in
qxl_io_create_primary().
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
drivers/gpu/drm/qxl/qxl_cmd.c | 7 +++++--
drivers/gpu/drm/qxl/qxl_display.c | 2 +-
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 6b8d28a23f..92140e0d71 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -229,7 +229,7 @@ struct qxl_device {
struct qxl_ram_header *ram_header;
- unsigned int primary_created:1;
+ struct qxl_bo *primary_bo;
struct qxl_memslot *mem_slots;
uint8_t n_mem_slots;
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index f263d83775..3c606f0f7a 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -372,13 +372,16 @@ void qxl_io_flush_surfaces(struct qxl_device *qdev)
void qxl_io_destroy_primary(struct qxl_device *qdev)
{
wait_for_io_cmd(qdev, 0, QXL_IO_DESTROY_PRIMARY_ASYNC);
- qdev->primary_created = false;
+ qdev->primary_bo = NULL;
}
void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
{
struct qxl_surface_create *create;
+ if (WARN_ON(qdev->primary_bo))
+ return;
+
DRM_DEBUG_DRIVER("qdev %p, ram_header %p\n", qdev, qdev->ram_header);
create = &qdev->ram_header->create_surface;
create->format = bo->surf.format;
@@ -397,7 +400,7 @@ void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
create->type = QXL_SURF_TYPE_PRIMARY;
wait_for_io_cmd(qdev, 0, QXL_IO_CREATE_PRIMARY_ASYNC);
- qdev->primary_created = true;
+ qdev->primary_bo = bo;
}
void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t id)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index d875cae02f..69b180735d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -302,7 +302,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
struct qxl_head head;
int oldcount, i = qcrtc->index;
- if (!qdev->primary_created) {
+ if (!qdev->primary_bo) {
DRM_DEBUG_KMS("no primary surface, skip (%s)\n", reason);
return;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 4/6] drm/qxl: use shadow bo directly
From: Gerd Hoffmann @ 2018-11-28 10:34 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: David Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU
In-Reply-To: <20181128103418.28023-1-kraxel@redhat.com>
Pass the shadow bo to qxl_io_create_primary() instead of expecting
qxl_io_create_primary to check bo->shadow. Set is_primary flag on the
shadow bo. Move the is_primary tracking into qxl_io_create_primary()
and qxl_io_destroy_primary() functions.
That simplifies primary surface tracking and the workflow in
qxl_primary_atomic_update().
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_cmd.c | 8 +++-----
drivers/gpu/drm/qxl/qxl_display.c | 29 ++++++++---------------------
2 files changed, 11 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 3c606f0f7a..3957e70ca0 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -372,6 +372,7 @@ void qxl_io_flush_surfaces(struct qxl_device *qdev)
void qxl_io_destroy_primary(struct qxl_device *qdev)
{
wait_for_io_cmd(qdev, 0, QXL_IO_DESTROY_PRIMARY_ASYNC);
+ qdev->primary_bo->is_primary = false;
qdev->primary_bo = NULL;
}
@@ -388,11 +389,7 @@ void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
create->width = bo->surf.width;
create->height = bo->surf.height;
create->stride = bo->surf.stride;
- if (bo->shadow) {
- create->mem = qxl_bo_physical_address(qdev, bo->shadow, 0);
- } else {
- create->mem = qxl_bo_physical_address(qdev, bo, 0);
- }
+ create->mem = qxl_bo_physical_address(qdev, bo, 0);
DRM_DEBUG_DRIVER("mem = %llx, from %p\n", create->mem, bo->kptr);
@@ -401,6 +398,7 @@ void qxl_io_create_primary(struct qxl_device *qdev, struct qxl_bo *bo)
wait_for_io_cmd(qdev, 0, QXL_IO_CREATE_PRIMARY_ASYNC);
qdev->primary_bo = bo;
+ qdev->primary_bo->is_primary = true;
}
void qxl_io_memslot_add(struct qxl_device *qdev, uint8_t id)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 69b180735d..472df00c02 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -526,14 +526,13 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
{
struct qxl_device *qdev = plane->dev->dev_private;
struct qxl_bo *bo = gem_to_qxl_bo(plane->state->fb->obj[0]);
- struct qxl_bo *bo_old;
+ struct qxl_bo *bo_old, *primary;
struct drm_clip_rect norect = {
.x1 = 0,
.y1 = 0,
.x2 = plane->state->fb->width,
.y2 = plane->state->fb->height
};
- bool same_shadow = false;
if (old_state->fb) {
bo_old = gem_to_qxl_bo(old_state->fb->obj[0]);
@@ -541,26 +540,13 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
bo_old = NULL;
}
- if (bo == bo_old)
- return;
+ primary = bo->shadow ? bo->shadow : bo;
- if (bo_old && bo_old->shadow && bo->shadow &&
- bo_old->shadow == bo->shadow) {
- same_shadow = true;
- }
-
- if (bo_old && bo_old->is_primary) {
- if (!same_shadow)
+ if (!primary->is_primary) {
+ if (qdev->primary_bo)
qxl_io_destroy_primary(qdev);
- bo_old->is_primary = false;
- }
-
- if (!bo->is_primary) {
- if (!same_shadow) {
- qxl_io_create_primary(qdev, bo);
- qxl_primary_apply_cursor(plane);
- }
- bo->is_primary = true;
+ qxl_io_create_primary(qdev, primary);
+ qxl_primary_apply_cursor(plane);
}
qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, &norect, 1, 1);
@@ -756,6 +742,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
qxl_bo_create(qdev, user_bo->gem_base.size,
true, true, QXL_GEM_DOMAIN_VRAM, NULL,
&user_bo->shadow);
+ user_bo->shadow->surf = user_bo->surf;
}
}
@@ -784,7 +771,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
user_bo = gem_to_qxl_bo(obj);
qxl_bo_unpin(user_bo);
- if (user_bo->shadow && !user_bo->is_primary) {
+ if (user_bo->shadow && !user_bo->shadow->is_primary) {
drm_gem_object_put_unlocked(&user_bo->shadow->gem_base);
user_bo->shadow = NULL;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 5/6] drm/qxl: cover all crtcs in shadow bo.
From: Gerd Hoffmann @ 2018-11-28 10:34 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: David Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU
In-Reply-To: <20181128103418.28023-1-kraxel@redhat.com>
The qxl device supports only a single active framebuffer ("primary
surface" in spice terminology). In multihead configurations are handled
by defining rectangles within the primary surface for each head/crtc.
Userspace which uses the qxl ioctl interface (xorg qxl driver) is aware
of this limitation and will setup framebuffers and crtcs accordingly.
Userspace which uses dumb framebuffers (xorg modesetting driver,
wayland) is not aware of this limitation and tries to use two
framebuffers (one for each crtc) instead.
The qxl kms driver already has the dumb bo separated from the primary
surface, by using a (shared) shadow bo as primary surface. This is
needed to support pageflips without having to re-create the primary
surface. The qxl driver will blit from the dumb bo to the shadow bo
instead.
So we can extend the shadow logic: Maintain a global shadow bo (aka
primary surface), make it big enough that dumb bo's for all crtcs fit in
side-by-side. Adjust the pageflip blits to place the heads next to each
other in the shadow.
With this patch in place multihead qxl works with wayland.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_drv.h | 5 +-
drivers/gpu/drm/qxl/qxl_display.c | 120 +++++++++++++++++++++++++++++---------
drivers/gpu/drm/qxl/qxl_draw.c | 9 ++-
3 files changed, 105 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 92140e0d71..161be511cd 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -230,6 +230,8 @@ struct qxl_device {
struct qxl_ram_header *ram_header;
struct qxl_bo *primary_bo;
+ struct qxl_bo *dumb_shadow_bo;
+ struct qxl_head *dumb_heads;
struct qxl_memslot *mem_slots;
uint8_t n_mem_slots;
@@ -447,7 +449,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
struct qxl_bo *bo,
unsigned int flags, unsigned int color,
struct drm_clip_rect *clips,
- unsigned int num_clips, int inc);
+ unsigned int num_clips, int inc,
+ uint32_t dumb_shadow_offset);
void qxl_draw_fill(struct qxl_draw_fill *qxl_draw_fill_rec);
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 472df00c02..a6110ec5ba 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -323,6 +323,8 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
head.y = crtc->y;
if (qdev->monitors_config->count < i + 1)
qdev->monitors_config->count = i + 1;
+ if (qdev->primary_bo == qdev->dumb_shadow_bo)
+ head.x += qdev->dumb_heads[i].x;
} else if (i > 0) {
head.width = 0;
head.height = 0;
@@ -424,7 +426,7 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
}
qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
- clips, num_clips, inc);
+ clips, num_clips, inc, 0);
drm_modeset_unlock_all(fb->dev);
@@ -533,6 +535,7 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
.x2 = plane->state->fb->width,
.y2 = plane->state->fb->height
};
+ uint32_t dumb_shadow_offset = 0;
if (old_state->fb) {
bo_old = gem_to_qxl_bo(old_state->fb->obj[0]);
@@ -549,7 +552,12 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
qxl_primary_apply_cursor(plane);
}
- qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, &norect, 1, 1);
+ if (bo->is_dumb)
+ dumb_shadow_offset =
+ qdev->dumb_heads[plane->state->crtc->index].x;
+
+ qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, &norect, 1, 1,
+ dumb_shadow_offset);
}
static void qxl_primary_atomic_disable(struct drm_plane *plane,
@@ -705,12 +713,68 @@ static void qxl_cursor_atomic_disable(struct drm_plane *plane,
qxl_release_fence_buffer_objects(release);
}
+static void qxl_update_dumb_head(struct qxl_device *qdev,
+ int index, struct qxl_bo *bo)
+{
+ uint32_t width, height;
+
+ if (index >= qdev->monitors_config->max_allowed)
+ return;
+
+ if (bo && bo->is_dumb) {
+ width = bo->surf.width;
+ height = bo->surf.height;
+ } else {
+ width = 0;
+ height = 0;
+ }
+
+ if (qdev->dumb_heads[index].width == width &&
+ qdev->dumb_heads[index].height == height)
+ return;
+
+ DRM_DEBUG("#%d: %dx%d -> %dx%d\n", index,
+ qdev->dumb_heads[index].width,
+ qdev->dumb_heads[index].height,
+ width, height);
+ qdev->dumb_heads[index].width = width;
+ qdev->dumb_heads[index].height = height;
+}
+
+static void qxl_calc_dumb_shadow(struct qxl_device *qdev,
+ struct qxl_surface *surf)
+{
+ struct qxl_head *head;
+ int i;
+
+ memset(surf, 0, sizeof(*surf));
+ for (i = 0; i < qdev->monitors_config->max_allowed; i++) {
+ head = qdev->dumb_heads + i;
+ head->x = surf->width;
+ surf->width += head->width;
+ if (surf->height < head->height)
+ surf->height = head->height;
+ }
+ if (surf->width < 64)
+ surf->width = 64;
+ if (surf->height < 64)
+ surf->height = 64;
+ surf->format = SPICE_SURFACE_FMT_32_xRGB;
+ surf->stride = surf->width * 4;
+
+ if (!qdev->dumb_shadow_bo ||
+ qdev->dumb_shadow_bo->surf.width != surf->width ||
+ qdev->dumb_shadow_bo->surf.height != surf->height)
+ DRM_DEBUG("%dx%d\n", surf->width, surf->height);
+}
+
static int qxl_plane_prepare_fb(struct drm_plane *plane,
struct drm_plane_state *new_state)
{
struct qxl_device *qdev = plane->dev->dev_private;
struct drm_gem_object *obj;
- struct qxl_bo *user_bo, *old_bo = NULL;
+ struct qxl_bo *user_bo;
+ struct qxl_surface surf;
int ret;
if (!new_state->fb)
@@ -720,29 +784,31 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
user_bo = gem_to_qxl_bo(obj);
if (plane->type == DRM_PLANE_TYPE_PRIMARY &&
- user_bo->is_dumb && !user_bo->shadow) {
- if (plane->state->fb) {
- obj = plane->state->fb->obj[0];
- old_bo = gem_to_qxl_bo(obj);
- }
- if (old_bo && old_bo->shadow &&
- user_bo->gem_base.size == old_bo->gem_base.size &&
- plane->state->crtc == new_state->crtc &&
- plane->state->crtc_w == new_state->crtc_w &&
- plane->state->crtc_h == new_state->crtc_h &&
- plane->state->src_x == new_state->src_x &&
- plane->state->src_y == new_state->src_y &&
- plane->state->src_w == new_state->src_w &&
- plane->state->src_h == new_state->src_h &&
- plane->state->rotation == new_state->rotation &&
- plane->state->zpos == new_state->zpos) {
- drm_gem_object_get(&old_bo->shadow->gem_base);
- user_bo->shadow = old_bo->shadow;
- } else {
- qxl_bo_create(qdev, user_bo->gem_base.size,
+ user_bo->is_dumb) {
+ qxl_update_dumb_head(qdev, new_state->crtc->index,
+ user_bo);
+ qxl_calc_dumb_shadow(qdev, &surf);
+ if (!qdev->dumb_shadow_bo ||
+ qdev->dumb_shadow_bo->surf.width != surf.width ||
+ qdev->dumb_shadow_bo->surf.height != surf.height) {
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put_unlocked
+ (&qdev->dumb_shadow_bo->gem_base);
+ qdev->dumb_shadow_bo = NULL;
+ }
+ qxl_bo_create(qdev, surf.width * surf.stride,
true, true, QXL_GEM_DOMAIN_VRAM, NULL,
- &user_bo->shadow);
- user_bo->shadow->surf = user_bo->surf;
+ &qdev->dumb_shadow_bo);
+ qdev->dumb_shadow_bo->surf = surf;
+ }
+ if (user_bo->shadow != qdev->dumb_shadow_bo) {
+ if (user_bo->shadow) {
+ drm_gem_object_put_unlocked
+ (&user_bo->shadow->gem_base);
+ user_bo->shadow = NULL;
+ }
+ drm_gem_object_get(&qdev->dumb_shadow_bo->gem_base);
+ user_bo->shadow = qdev->dumb_shadow_bo;
}
}
@@ -771,7 +837,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
user_bo = gem_to_qxl_bo(obj);
qxl_bo_unpin(user_bo);
- if (user_bo->shadow && !user_bo->shadow->is_primary) {
+ if (user_bo->shadow) {
drm_gem_object_put_unlocked(&user_bo->shadow->gem_base);
user_bo->shadow = NULL;
}
@@ -1105,6 +1171,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
memset(qdev->monitors_config, 0, monitors_config_size);
qdev->monitors_config->max_allowed = max_allowed;
+
+ qdev->dumb_heads = kcalloc(max_allowed, sizeof(qdev->dumb_heads[0]), GFP_KERNEL);
return 0;
}
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index c408bb83c7..5313ad21c1 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -267,7 +267,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
struct qxl_bo *bo,
unsigned int flags, unsigned int color,
struct drm_clip_rect *clips,
- unsigned int num_clips, int inc)
+ unsigned int num_clips, int inc,
+ uint32_t dumb_shadow_offset)
{
/*
* TODO: if flags & DRM_MODE_FB_DIRTY_ANNOTATE_FILL then we should
@@ -295,6 +296,9 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
if (ret)
return;
+ clips->x1 += dumb_shadow_offset;
+ clips->x2 += dumb_shadow_offset;
+
left = clips->x1;
right = clips->x2;
top = clips->y1;
@@ -342,7 +346,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
goto out_release_backoff;
ret = qxl_image_init(qdev, release, dimage, surface_base,
- left, top, width, height, depth, stride);
+ left - dumb_shadow_offset,
+ top, width, height, depth, stride);
qxl_bo_kunmap(bo);
if (ret)
goto out_release_backoff;
--
2.9.3
^ permalink raw reply related
* [PATCH 6/6] drm/qxl: use qxl_num_crtc directly
From: Gerd Hoffmann @ 2018-11-28 10:34 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: David Airlie, open list, open list:DRM DRIVER FOR QXL VIRTUAL GPU
In-Reply-To: <20181128103418.28023-1-kraxel@redhat.com>
Skip the pointless and slightly confusing indirection via
qdev->monitors_config->max_allowed. Just use qxl_num_crtc instead.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/qxl/qxl_display.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index a6110ec5ba..731a17ba50 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -80,10 +80,10 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
DRM_DEBUG_KMS("no client monitors configured\n");
return status;
}
- if (num_monitors > qdev->monitors_config->max_allowed) {
+ if (num_monitors > qxl_num_crtc) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < %d\n",
- qdev->monitors_config->max_allowed, num_monitors);
- num_monitors = qdev->monitors_config->max_allowed;
+ qxl_num_crtc, num_monitors);
+ num_monitors = qxl_num_crtc;
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
@@ -96,8 +96,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
return status;
}
/* we copy max from the client but it isn't used */
- qdev->client_monitors_config->max_allowed =
- qdev->monitors_config->max_allowed;
+ qdev->client_monitors_config->max_allowed = qxl_num_crtc;
for (i = 0 ; i < qdev->client_monitors_config->count ; ++i) {
struct qxl_urect *c_rect =
&qdev->rom->client_monitors_config.heads[i];
@@ -204,7 +203,7 @@ static int qxl_add_monitors_config_modes(struct drm_connector *connector,
if (!qdev->monitors_config)
return 0;
- if (h >= qdev->monitors_config->max_allowed)
+ if (h >= qxl_num_crtc)
return 0;
if (!qdev->client_monitors_config)
return 0;
@@ -307,8 +306,7 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
return;
}
- if (!qdev->monitors_config ||
- qdev->monitors_config->max_allowed <= i)
+ if (!qdev->monitors_config || qxl_num_crtc <= i)
return;
head.id = i;
@@ -350,9 +348,10 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
if (oldcount != qdev->monitors_config->count)
DRM_DEBUG_KMS("active heads %d -> %d (%d total)\n",
oldcount, qdev->monitors_config->count,
- qdev->monitors_config->max_allowed);
+ qxl_num_crtc);
qdev->monitors_config->heads[i] = head;
+ qdev->monitors_config->max_allowed = qxl_num_crtc;
qxl_send_monitors_config(qdev);
}
@@ -1146,9 +1145,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
{
int ret;
struct drm_gem_object *gobj;
- int max_allowed = qxl_num_crtc;
int monitors_config_size = sizeof(struct qxl_monitors_config) +
- max_allowed * sizeof(struct qxl_head);
+ qxl_num_crtc * sizeof(struct qxl_head);
ret = qxl_gem_object_create(qdev, monitors_config_size, 0,
QXL_GEM_DOMAIN_VRAM,
@@ -1170,9 +1168,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0);
memset(qdev->monitors_config, 0, monitors_config_size);
- qdev->monitors_config->max_allowed = max_allowed;
-
- qdev->dumb_heads = kcalloc(max_allowed, sizeof(qdev->dumb_heads[0]), GFP_KERNEL);
+ qdev->dumb_heads = kcalloc(qxl_num_crtc, sizeof(qdev->dumb_heads[0]),
+ GFP_KERNEL);
return 0;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 1/2] drm/virtio: fence: pass plain pointer
From: Gerd Hoffmann @ 2018-11-28 15:10 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: robert.foss, David Airlie, open list, open list:VIRTIO GPU DRIVER
In-Reply-To: <20181128151021.29565-1-kraxel@redhat.com>
Since commit "9fdd90c0f4 drm/virtio: add virtio_gpu_alloc_fence()"
fences are not allocated any more by virtio_gpu_fence_emit(). So there
is no need to pass down a reference to the fence pointer, a plain
pointer is enough now.
Convert virtio_gpu_fence_emit() and callers.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 14 +++++++-------
drivers/gpu/drm/virtio/virtgpu_fence.c | 10 +++++-----
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 8 ++++----
drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_vq.c | 20 ++++++++++----------
5 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 7bec6e3688..d6cc1a92ca 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -273,7 +273,7 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
uint64_t offset,
__le32 width, __le32 height,
__le32 x, __le32 y,
- struct virtio_gpu_fence **fence);
+ struct virtio_gpu_fence *fence);
void virtio_gpu_cmd_resource_flush(struct virtio_gpu_device *vgdev,
uint32_t resource_id,
uint32_t x, uint32_t y,
@@ -284,7 +284,7 @@ void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev,
uint32_t x, uint32_t y);
int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *obj,
- struct virtio_gpu_fence **fence);
+ struct virtio_gpu_fence *fence);
void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *obj);
int virtio_gpu_attach_status_page(struct virtio_gpu_device *vgdev);
@@ -309,23 +309,23 @@ void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,
uint32_t resource_id);
void virtio_gpu_cmd_submit(struct virtio_gpu_device *vgdev,
void *data, uint32_t data_size,
- uint32_t ctx_id, struct virtio_gpu_fence **fence);
+ uint32_t ctx_id, struct virtio_gpu_fence *fence);
void virtio_gpu_cmd_transfer_from_host_3d(struct virtio_gpu_device *vgdev,
uint32_t resource_id, uint32_t ctx_id,
uint64_t offset, uint32_t level,
struct virtio_gpu_box *box,
- struct virtio_gpu_fence **fence);
+ struct virtio_gpu_fence *fence);
void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo,
uint32_t ctx_id,
uint64_t offset, uint32_t level,
struct virtio_gpu_box *box,
- struct virtio_gpu_fence **fence);
+ struct virtio_gpu_fence *fence);
void
virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo,
struct virtio_gpu_resource_create_3d *rc_3d,
- struct virtio_gpu_fence **fence);
+ struct virtio_gpu_fence *fence);
void virtio_gpu_ctrl_ack(struct virtqueue *vq);
void virtio_gpu_cursor_ack(struct virtqueue *vq);
void virtio_gpu_fence_ack(struct virtqueue *vq);
@@ -358,7 +358,7 @@ struct virtio_gpu_fence *virtio_gpu_fence_alloc(
void virtio_gpu_fence_cleanup(struct virtio_gpu_fence *fence);
int virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
struct virtio_gpu_ctrl_hdr *cmd_hdr,
- struct virtio_gpu_fence **fence);
+ struct virtio_gpu_fence *fence);
void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
u64 last_seq);
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 6b5d92215c..4d6826b278 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -91,19 +91,19 @@ void virtio_gpu_fence_cleanup(struct virtio_gpu_fence *fence)
int virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
struct virtio_gpu_ctrl_hdr *cmd_hdr,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
unsigned long irq_flags;
spin_lock_irqsave(&drv->lock, irq_flags);
- (*fence)->seq = ++drv->sync_seq;
- dma_fence_get(&(*fence)->f);
- list_add_tail(&(*fence)->node, &drv->fences);
+ fence->seq = ++drv->sync_seq;
+ dma_fence_get(&fence->f);
+ list_add_tail(&fence->node, &drv->fences);
spin_unlock_irqrestore(&drv->lock, irq_flags);
cmd_hdr->flags |= cpu_to_le32(VIRTIO_GPU_FLAG_FENCE);
- cmd_hdr->fence_id = cpu_to_le64((*fence)->seq);
+ cmd_hdr->fence_id = cpu_to_le64(fence->seq);
return 0;
}
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 340f2513d8..e9cdb4c7f6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -221,7 +221,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
}
virtio_gpu_cmd_submit(vgdev, buf, exbuf->size,
- vfpriv->ctx_id, &out_fence);
+ vfpriv->ctx_id, out_fence);
ttm_eu_fence_buffer_objects(&ticket, &validate_list, &out_fence->f);
@@ -349,7 +349,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
}
virtio_gpu_cmd_resource_create_3d(vgdev, qobj, &rc_3d, NULL);
- ret = virtio_gpu_object_attach(vgdev, qobj, &fence);
+ ret = virtio_gpu_object_attach(vgdev, qobj, fence);
if (ret) {
virtio_gpu_fence_cleanup(fence);
goto fail_backoff;
@@ -450,7 +450,7 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
virtio_gpu_cmd_transfer_from_host_3d
(vgdev, qobj->hw_res_handle,
vfpriv->ctx_id, offset, args->level,
- &box, &fence);
+ &box, fence);
reservation_object_add_excl_fence(qobj->tbo.resv,
&fence->f);
@@ -504,7 +504,7 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data,
virtio_gpu_cmd_transfer_to_host_3d
(vgdev, qobj,
vfpriv ? vfpriv->ctx_id : 0, offset,
- args->level, &box, &fence);
+ args->level, &box, fence);
reservation_object_add_excl_fence(qobj->tbo.resv,
&fence->f);
dma_fence_put(&fence->f);
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index b84ac8c258..ead5c53d4e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -204,7 +204,7 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
(vgdev, bo, 0,
cpu_to_le32(plane->state->crtc_w),
cpu_to_le32(plane->state->crtc_h),
- 0, 0, &vgfb->fence);
+ 0, 0, vgfb->fence);
ret = virtio_gpu_object_reserve(bo, false);
if (!ret) {
reservation_object_add_excl_fence(bo->tbo.resv,
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 2c6764f08f..97038662b9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -298,7 +298,7 @@ static int virtio_gpu_queue_ctrl_buffer(struct virtio_gpu_device *vgdev,
static int virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
struct virtio_gpu_vbuffer *vbuf,
struct virtio_gpu_ctrl_hdr *hdr,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtqueue *vq = vgdev->ctrlq.vq;
int rc;
@@ -405,7 +405,7 @@ void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
static void virtio_gpu_cmd_resource_inval_backing(struct virtio_gpu_device *vgdev,
uint32_t resource_id,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_resource_detach_backing *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -467,7 +467,7 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
uint64_t offset,
__le32 width, __le32 height,
__le32 x, __le32 y,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_transfer_to_host_2d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -497,7 +497,7 @@ virtio_gpu_cmd_resource_attach_backing(struct virtio_gpu_device *vgdev,
uint32_t resource_id,
struct virtio_gpu_mem_entry *ents,
uint32_t nents,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_resource_attach_backing *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -821,7 +821,7 @@ void
virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo,
struct virtio_gpu_resource_create_3d *rc_3d,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_resource_create_3d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -842,7 +842,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
uint32_t ctx_id,
uint64_t offset, uint32_t level,
struct virtio_gpu_box *box,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_transfer_host_3d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -870,7 +870,7 @@ void virtio_gpu_cmd_transfer_from_host_3d(struct virtio_gpu_device *vgdev,
uint32_t resource_id, uint32_t ctx_id,
uint64_t offset, uint32_t level,
struct virtio_gpu_box *box,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
struct virtio_gpu_transfer_host_3d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -890,7 +890,7 @@ void virtio_gpu_cmd_transfer_from_host_3d(struct virtio_gpu_device *vgdev,
void virtio_gpu_cmd_submit(struct virtio_gpu_device *vgdev,
void *data, uint32_t data_size,
- uint32_t ctx_id, struct virtio_gpu_fence **fence)
+ uint32_t ctx_id, struct virtio_gpu_fence *fence)
{
struct virtio_gpu_cmd_submit *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -910,7 +910,7 @@ void virtio_gpu_cmd_submit(struct virtio_gpu_device *vgdev,
int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *obj,
- struct virtio_gpu_fence **fence)
+ struct virtio_gpu_fence *fence)
{
bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);
struct virtio_gpu_mem_entry *ents;
@@ -967,7 +967,7 @@ void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
if (use_dma_api && obj->mapped) {
struct virtio_gpu_fence *fence = virtio_gpu_fence_alloc(vgdev);
/* detach backing and wait for the host process it ... */
- virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, &fence);
+ virtio_gpu_cmd_resource_inval_backing(vgdev, obj->hw_res_handle, fence);
dma_fence_wait(&fence->f, true);
dma_fence_put(&fence->f);
--
2.9.3
^ permalink raw reply related
* [PATCH 2/2] drm/virtio: virtio_gpu_cmd_resource_create_3d: drop unused fence arg
From: Gerd Hoffmann @ 2018-11-28 15:10 UTC (permalink / raw)
To: David Airlie, dri-devel
Cc: robert.foss, David Airlie, open list, open list:VIRTIO GPU DRIVER
In-Reply-To: <20181128151021.29565-1-kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +--
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 +-
drivers/gpu/drm/virtio/virtgpu_vq.c | 5 ++---
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index d6cc1a92ca..4e522e0b59 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -324,8 +324,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev,
void
virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo,
- struct virtio_gpu_resource_create_3d *rc_3d,
- struct virtio_gpu_fence *fence);
+ struct virtio_gpu_resource_create_3d *rc_3d);
void virtio_gpu_ctrl_ack(struct virtqueue *vq);
void virtio_gpu_cursor_ack(struct virtqueue *vq);
void virtio_gpu_fence_ack(struct virtqueue *vq);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index e9cdb4c7f6..161b80fee4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -348,7 +348,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
goto fail_backoff;
}
- virtio_gpu_cmd_resource_create_3d(vgdev, qobj, &rc_3d, NULL);
+ virtio_gpu_cmd_resource_create_3d(vgdev, qobj, &rc_3d);
ret = virtio_gpu_object_attach(vgdev, qobj, fence);
if (ret) {
virtio_gpu_fence_cleanup(fence);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 97038662b9..e27c4aedb8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -820,8 +820,7 @@ void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,
void
virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
struct virtio_gpu_object *bo,
- struct virtio_gpu_resource_create_3d *rc_3d,
- struct virtio_gpu_fence *fence)
+ struct virtio_gpu_resource_create_3d *rc_3d)
{
struct virtio_gpu_resource_create_3d *cmd_p;
struct virtio_gpu_vbuffer *vbuf;
@@ -833,7 +832,7 @@ virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_CREATE_3D);
cmd_p->hdr.flags = 0;
- virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence);
+ virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
bo->created = true;
}
--
2.9.3
^ permalink raw reply related
* [PATCH net] virtio-net: keep vnet header zeroed after processing XDP
From: Jason Wang @ 2018-11-29 5:53 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, davem, linux-kernel, virtualization
We copy vnet header unconditionally in page_to_skb() this is wrong
since XDP may modify the packet data. So let's keep a zeroed vnet
header for not confusing the conversion between vnet header and skb
metadata.
In the future, we should able to detect whether or not the packet was
modified and keep using the vnet header when packet was not touched.
Fixes: f600b6905015 ("virtio_net: Add XDP support")
Reported-by: Pavel Popa <pashinho1990@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index cecfd77c9f3c..ea672145f6a6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -365,7 +365,8 @@ static unsigned int mergeable_ctx_to_truesize(void *mrg_ctx)
static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
struct page *page, unsigned int offset,
- unsigned int len, unsigned int truesize)
+ unsigned int len, unsigned int truesize,
+ bool hdr_valid)
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -387,7 +388,8 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- memcpy(hdr, p, hdr_len);
+ if (hdr_valid)
+ memcpy(hdr, p, hdr_len);
len -= hdr_len;
offset += hdr_padded_len;
@@ -739,7 +741,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
struct virtnet_rq_stats *stats)
{
struct page *page = buf;
- struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
+ struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
+ PAGE_SIZE, true);
stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
@@ -842,7 +845,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
rcu_read_unlock();
put_page(page);
head_skb = page_to_skb(vi, rq, xdp_page,
- offset, len, PAGE_SIZE);
+ offset, len,
+ PAGE_SIZE, false);
return head_skb;
}
break;
@@ -898,7 +902,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_skb;
}
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize);
+ head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
curr_skb = head_skb;
if (unlikely(!curr_skb))
--
2.17.1
^ permalink raw reply related
* Re: PROPOSAL: Extend inline asm syntax with size spec
From: Masahiro Yamada @ 2018-11-29 11:46 UTC (permalink / raw)
To: Segher Boessenkool, Nadav Amit, Ingo Molnar, H. Peter Anvin
Cc: Kate Stewart, Peter Zijlstra (Intel), Christopher Li,
virtualization, Max Filippov, Jan Beulich, Sam Ravnborg,
Thomas Gleixner, X86 ML, linux-sparse, linux-xtensa, Kees Cook,
Chris Zankel, matz, Borislav Petkov, Josh Poimboeuf, Alok Kataria,
Juergen Gross, gcc, rguenther, Greg Kroah-Hartman,
Linux Kernel Mailing List, Philippe Ombredanne, Linus Torvalds
In-Reply-To: <20181009145330.GT29268@gate.crashing.org>
Hi.
On Wed, Oct 10, 2018 at 1:14 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Mon, Oct 08, 2018 at 11:07:46AM +0200, Richard Biener wrote:
> > On Mon, 8 Oct 2018, Segher Boessenkool wrote:
> > > On Sun, Oct 07, 2018 at 03:53:26PM +0000, Michael Matz wrote:
> > > > On Sun, 7 Oct 2018, Segher Boessenkool wrote:
> > > > > On Sun, Oct 07, 2018 at 11:18:06AM +0200, Borislav Petkov wrote:
> > > > > > Now, Richard suggested doing something like:
> > > > > >
> > > > > > 1) inline asm ("...")
> > > > >
> > > > > What would the semantics of this be?
> > > >
> > > > The size of the inline asm wouldn't be counted towards the inliner size
> > > > limits (or be counted as "1").
> > >
> > > That sounds like a good option.
> >
> > Yes, I also like it for simplicity. It also avoids the requirement
> > of translating the number (in bytes?) given by the user to
> > "number of GIMPLE instructions" as needed by the inliner.
>
> This patch implements this, for C only so far. And the syntax is
> "asm inline", which is more in line with other syntax.
>
> How does this look?
Thank you very much for your work.
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01932.html
How is the progress of this in GCC ML?
I am really hoping the issue will be solved by compiler
instead of the in-kernel workaround.
Since commit 77b0bf55bc675233d22cd5df97605d516d64525e,
DISTCC breakage was reported.
Then, another problem showed up.
Debian linux-headers package is broken
due to missing arch/x86/kernel/macros.s
https://www.spinics.net/lists/linux-kbuild/msg20037.html
The kernel-devel RPM package is broken as well.
More fundamentally, the external module building itself is broken;
'make clean' must keep all files needed for external modules, but
*.s files are all gone.
Of course, we can fix the problems at the cost of uglifying Makefiles.
I wrote a patch to fix the external module building
and packages, and now have it in hand locally.
But, I'd like to ask if x86 people want to keep this macros.s approach.
Revert 77b0bf55bc675 right now
assuming the compiler will eventually solve the issue?
Do you have ideas? Comments?
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: PROPOSAL: Extend inline asm syntax with size spec
From: Borislav Petkov via Virtualization @ 2018-11-29 13:07 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Kate Stewart, Peter Zijlstra (Intel), Christopher Li,
virtualization, Max Filippov, Nadav Amit, Jan Beulich,
H. Peter Anvin, Sam Ravnborg, Thomas Gleixner, X86 ML,
linux-sparse, Ingo Molnar, linux-xtensa, Kees Cook,
Segher Boessenkool, Chris Zankel, matz, Josh Poimboeuf,
Alok Kataria, Juergen Gross, gcc, rguenther, Greg Kroah-Hartman,
Linux Kernel Mailing List
In-Reply-To: <CAK7LNARmrdtyE7JRAdJH_zbfvD_cej+TGLh+5KfT20o538KUcA@mail.gmail.com>
On Thu, Nov 29, 2018 at 08:46:34PM +0900, Masahiro Yamada wrote:
> But, I'd like to ask if x86 people want to keep this macros.s approach.
> Revert 77b0bf55bc675 right now
> assuming the compiler will eventually solve the issue?
Yap, considering how elegant the compiler solution is and how much
problems this macros-in-asm thing causes, I think we should patch
out the latter and wait for gcc9. I mean, the savings are not so
mind-blowing to have to deal with the fallout.
But this is just my opinion.
Thx.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
^ permalink raw reply
* Re: PROPOSAL: Extend inline asm syntax with size spec
From: Borislav Petkov via Virtualization @ 2018-11-29 13:16 UTC (permalink / raw)
To: Richard Biener
Cc: Kate Stewart, Peter Zijlstra (Intel), Christopher Li,
virtualization, Masahiro Yamada, Nadav Amit, Jan Beulich,
H. Peter Anvin, Sam Ravnborg, Thomas Gleixner, X86 ML,
linux-sparse, Ingo Molnar, linux-xtensa, Kees Cook,
Segher Boessenkool, Chris Zankel, matz, Josh Poimboeuf,
Alok Kataria, Juergen Gross, gcc, Max Filippov,
Greg Kroah-Hartman
In-Reply-To: <alpine.LSU.2.20.1811291408400.1827@zhemvz.fhfr.qr>
On Thu, Nov 29, 2018 at 02:09:25PM +0100, Richard Biener wrote:
> I'd be not opposed to backporting the asm inline support.
Even better! :-)
> Of course we still have to be happy with it and install the patch ;)
>
> Are you (kernel folks) happy with asm inline ()?
Yes, I think we are:
https://lkml.kernel.org/r/20181031125526.GA13219@hirez.programming.kicks-ass.net
Thx.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
^ permalink raw reply
* Re: Bug#912087: reassign to systemd #912087 | openssh-server: Slow startup after the upgrade to 7.9p1
From: Sebastian Andrzej Siewior @ 2018-11-29 13:54 UTC (permalink / raw)
To: Olaf van der Spek, Alok Kataria, Hans de Goede, Arnd Bergmann,
Greg Kroah-Hartman
Cc: Michael Biebl, 912087, linux-kernel, virtualization
In-Reply-To: <CAA7U3HN8aV-BrYGS4XNgXsfAL8eFcxdjReM-5VZOSnJntVanJw@mail.gmail.com>
On 2018-11-28 13:43:07 [+0100], Olaf van der Spek wrote:
> > > They might just as well install haveged or configure virtio-rng in such
> > > a case.
> >
> > Right. Do you think, that it would necessary to add something to the
> > release notes?
>
> I do. ;)
> What's the workaround for VMware?
>
> Does it just take longer to start or do some services not start at all?
It will take longer to start, it will start. Let me pass that workaround
question to someone from vmware/virtualbox and #912087:
On a recent kernel you see something like that:
[ 68.823013] random: crng init done
which means it took almost 69 seconds for the system to initialize its
rng. OpenSSH 7.9 can be compilied against OpenSSL 1.1.1 which in turn
switched to getrandom() (for its randomness).
This syscall will block until kernel's rng is ready which took in this
example almost 69 seconds.
Those "high" numbers are not a problem on decent/recent HW but occur
oftern on in virtualized environments.
For KVM we have CONFIG_HW_RANDOM_VIRTIO. Are there any plans to get
something similar for VMware/Vbox?
[0] http://bugs.debian.org/912087
Sebastian
^ permalink raw reply
* Re: [RFC] Discuss about an new idea "Vsock over Virtio-net"
From: Michael S. Tsirkin @ 2018-11-29 14:00 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, stefanha, netdev
In-Reply-To: <61d57505-7ff6-23c6-d26c-6a0062e08445@redhat.com>
On Thu, Nov 15, 2018 at 04:24:38PM +0800, Jason Wang wrote:
>
> On 2018/11/15 下午3:04, Michael S. Tsirkin wrote:
> > On Thu, Nov 15, 2018 at 11:56:03AM +0800, jiangyiwen wrote:
> > > Hi Stefan, Michael, Jason and everyone,
> > >
> > > Several days ago, I discussed with jason about "Vsock over Virtio-net".
> > > This idea has two advantages:
> > > First, it can use many great features of virtio-net, like batching,
> > > mergeable rx buffer and multiqueue, etc.
> > > Second, it can reduce many duplicate codes and make it easy to be
> > > maintained.
> > I'm not sure I get the motivation. Which features of
> > virtio net are relevant to vsock?
>
>
> Vsock is just a L2 (and above) protocol from the view of the device.
I don't believe so. I think virtio-vsock operates at a transport level.
There is in theory a bit of network level but we don't really implement
it as it's only host to guest. I am not aware of any data link
functionality n virtio-vsock. virtio-vsock provides services such as
connection-oriented communication, reliability, flow control and
multiplexing.
> So I
> think we should answer the question why we need two different paths for
> networking traffic? Or what is the fundamental reason that makes vsock does
> not go for virtio-net?
So virtio-vsock ensures reliability. If you want to compare it with
something that would be TCP or QUIC. The fundamental difference between
virtio-vsock and e.g. TCP is that TCP operates in a packet loss environment.
So they are using timers for reliability, and receiver is always free to
discard any unacked data.
> I agree they could be different type of devices but codes could be shared in
> both guest and host (or even qemu) for not duplicating features(bugs).
>
> Thanks
>
>
> > The ones that you mention
> > all seem to be mostly of use to the networking stack.
> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] VSOCK: About Virtio-vsock support "Multiqueue" feature ?
From: Stefan Hajnoczi @ 2018-11-29 14:06 UTC (permalink / raw)
To: jiangyiwen; +Cc: netdev, Jorgen Hansen, kvm, virtualization
In-Reply-To: <5BBB0203.1010400@huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 867 bytes --]
On Mon, Oct 08, 2018 at 03:06:43PM +0800, jiangyiwen wrote:
> Hi Stefan & All:
>
> Now virtio-vsock only supports two vqs(tx and rx), that means
> if multiple sockets in the guest will use the same vq to transmit
> the message and get the response. In this way, the bandwidth will
> be limited to ~700MB/s. So if there are multiple applications in
> the guest, we should support "Multiqueue" feature for Virtio-vsock.
>
> I want to know whether we already have plans to support multiqueue
> or already have simple demo that can be used. If not, I will try
> to implement this feature.
Multiqueue is certainly interesting. It would be interesting to see
if/how it affects common net/vmw_vsock/ code. Hopefully nothing much
will change there but I haven't checked if any common locking would
prevent multiqueue from working efficiently.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/5] VSOCK: support mergeable rx buffer in vhost-vsock
From: Stefan Hajnoczi @ 2018-11-29 14:19 UTC (permalink / raw)
To: jiangyiwen; +Cc: netdev, kvm, virtualization
In-Reply-To: <5BE12C72.6000204@huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 5297 bytes --]
On Tue, Nov 06, 2018 at 01:53:54PM +0800, jiangyiwen wrote:
> On 2018/11/6 11:32, Jason Wang wrote:
> >
> > On 2018/11/6 上午11:17, jiangyiwen wrote:
> >> On 2018/11/6 10:41, Jason Wang wrote:
> >>> On 2018/11/6 上午10:17, jiangyiwen wrote:
> >>>> On 2018/11/5 17:21, Jason Wang wrote:
> >>>>> On 2018/11/5 下午3:43, jiangyiwen wrote:
> >>>>>> Now vsock only support send/receive small packet, it can't achieve
> >>>>>> high performance. As previous discussed with Jason Wang, I revisit the
> >>>>>> idea of vhost-net about mergeable rx buffer and implement the mergeable
> >>>>>> rx buffer in vhost-vsock, it can allow big packet to be scattered in
> >>>>>> into different buffers and improve performance obviously.
> >>>>>>
> >>>>>> I write a tool to test the vhost-vsock performance, mainly send big
> >>>>>> packet(64K) included guest->Host and Host->Guest. The result as
> >>>>>> follows:
> >>>>>>
> >>>>>> Before performance:
> >>>>>> Single socket Multiple sockets(Max Bandwidth)
> >>>>>> Guest->Host ~400MB/s ~480MB/s
> >>>>>> Host->Guest ~1450MB/s ~1600MB/s
> >>>>>>
> >>>>>> After performance:
> >>>>>> Single socket Multiple sockets(Max Bandwidth)
> >>>>>> Guest->Host ~1700MB/s ~2900MB/s
> >>>>>> Host->Guest ~1700MB/s ~2900MB/s
> >>>>>>
> >>>>>> From the test results, the performance is improved obviously, and guest
> >>>>>> memory will not be wasted.
> >>>>> Hi:
> >>>>>
> >>>>> Thanks for the patches and the numbers are really impressive.
> >>>>>
> >>>>> But instead of duplicating codes between sock and net. I was considering to use virtio-net as a transport of vsock. Then we may have all existed features likes batching, mergeable rx buffers and multiqueue. Want to consider this idea? Thoughts?
> >>>>>
> >>>>>
> >>>> Hi Jason,
> >>>>
> >>>> I am not very familiar with virtio-net, so I am afraid I can't give too
> >>>> much effective advice. Then I have several problems:
> >>>>
> >>>> 1. If use virtio-net as a transport, guest should see a virtio-net
> >>>> device instead of virtio-vsock device, right? Is vsock only as a
> >>>> transport between socket and net_device? User should still use
> >>>> AF_VSOCK type to create socket, right?
> >>>
> >>> Well, there're many choices. What you need is just to keep the socket API and hide the implementation. For example, you can keep the vosck device in guest and switch to use vhost-net in host. We probably need a new feature bit or header to let vhost know we are passing vsock packet. And vhost-net could forward the packet to vsock core on host.
> >>>
> >>>
> >>>> 2. I want to know if this idea has already started, and how is
> >>>> the current progress?
> >>>
> >>> Not yet started. Just want to listen from the community. If this sounds good, do you have interest in implementing this?
> >>>
> >>>
> >>>> 3. And what is stefan's idea?
> >>>
> >>> Talk with Stefan a little on this during KVM Forum. I think he tends to agree on this idea. Anyway, let's wait for his reply.
> >>>
> >>>
> >>> Thanks
> >>>
> >>>
> >> Hi Jason,
> >>
> >> Thanks your reply, what you want is try to avoid duplicate code, and still
> >> use the existed features with virtio-net.
> >
> >
> > Yes, technically we can use virtio-net driver is guest as well but we could do it step by step.
> >
> >
> >> Yes, if this sounds good and most people can recognize this idea, I am very
> >> happy to implement this.
> >
> >
> > Cool, thanks.
> >
> >
> >>
> >> In addition, I hope you can review these patches before the new idea is
> >> implemented, after all the performance can be improved. :-)
> >
> >
> > Ok.
> >
> >
> > So the patch actually did three things:
> >
> > - mergeable buffer implementation
> >
> > - increase the default rx buffer size
> >
> > - add used and signal guest in a batch
> >
> > It would be helpful if you can measure the performance improvement independently. This can give reviewer a better understanding on how much did each part help.
> >
> > Thanks
> >
> >
>
> Great, I will test the performance independently in the later version.
I'm catching up on email so maybe you've already discussed this, but a
key design point in virtio-vsock is reliable in-order delivery. When
using virtio-net code it's important to keep those properties so that
AF_VSOCK SOCK_STREAM sockets work as expected. Packets must not be
reordered or dropped.
In addition, there's the virtio-vsock flow control scheme that allows
multiple sockets to share a ring without starvation or denial-of-service
problems. The guest knows how much socket buffer space is available on
the host (and vice versa). A well-behaved guest only sends up to the
available buffer space so that the host can copy the data into the
socket buffer and free up ring space for other sockets. This scheme is
how virtio-vsock achieves guaranteed delivery while avoiding starvation
or denial-of-service.
So you'll need to use some kind of framing (protocol) that preserves
these properties on top of virtio-net. This framing could be based on
virtio-vsock's packet headers.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/9] drm: remove deprecated functions
From: Daniel Vetter @ 2018-11-29 14:55 UTC (permalink / raw)
To: Linus Walleij
Cc: Haneen Mohammed, Alexandre Belloni, Gilles Muller, Maxime Ripard,
dri-devel, Linux Kernel Mailing List, Andrzej Hajda, cocci,
Marek Vasut, Archit Taneja, Jonathan Corbet,
Linux Doc Mailing List, Alexey Brodkin, Dave Airlie,
Ludovic Desroches, greenfoo, Russell King, Nicolas Palix,
Maarten Lankhorst, etnaviv, Boris Brezillon, Christian
In-Reply-To: <CACRpkdYmBSVKBvu=YOV87zWK8WCC+pcn9ux_W4VEYwzRzexQ_A@mail.gmail.com>
On Thu, Nov 29, 2018 at 3:45 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Nov 26, 2018 at 3:12 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sat, Nov 24, 2018 at 10:17:13PM +0100, Linus Walleij wrote:
>
> > > It was especially scary.
> > >
> > > But I think I managed to apply the patches and push the
> > > branch now.
> >
> > Except when you're racing with someone else you should only see conflicts
> > with stuff you've just pushed. Or if someone forgot to fix up their mess.
> > What was the conflict?
>
> dim push-branches was complaining that one of the commits was
> missing the proper committer sign-off, it was the bottom commit under
> mine (IIRC "drm/atomic-helper: WARN if fake_commit->hw_done is not
> completed as expected")
> and dim update-branches seemed to rebase and fix up my patches
> and then everything was fine.
This sounds like you (or dim?) accidentaly amended that commit (which
changes the committer and results in the warning), and a rebase would
indeed have fixed that. If the first patch conflicts this can happen
because dim apply-branch doesn't bail out correctly. Or at least did
in the past, I recently fixed that in
commit ee299e510ae468aab27610bcbc4fdd4de932f74b
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Wed Oct 17 08:53:00 2018 +0200
dim: make apply-patch fail again
> I just felt slightly out of control :D
If your dim didn't have the above commit and you had a conflict it's
all explained. Otherwise I'm not sure what's been going on ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PULL 0/2] virtio-ccw fixes
From: Cornelia Huck @ 2018-11-29 14:56 UTC (permalink / raw)
To: Michael S . Tsirkin; +Cc: Halil Pasic, linux-s390, kvm, virtualization
In-Reply-To: <20180926164830.8435-1-cohuck@redhat.com>
On Wed, 26 Sep 2018 18:48:28 +0200
Cornelia Huck <cohuck@redhat.com> wrote:
> The following changes since commit 8c0f9f5b309d627182d5da72a69246f58bde1026:
>
> Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name" (2018-09-25 13:28:58 +0200)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-ccw-20180926
>
> for you to fetch changes up to 1fd7ccf82385b3b14e4b3f009a82ada17ddc1e6f:
>
> virtio/s390: fix race in ccw_io_helper() (2018-09-25 15:29:10 +0200)
>
> ----------------------------------------------------------------
> Two patches fixing races in the virtio-ccw driver.
>
> ----------------------------------------------------------------
>
> Halil Pasic (2):
> virtio/s390: avoid race on vcdev->config
> virtio/s390: fix race in ccw_io_helper()
>
> drivers/s390/virtio/virtio_ccw.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
Ping. I don't see these patches in master :(
^ permalink raw reply
* Re: [REBASE PATCH net-next v9 1/4] net: vhost: lock the vqs one by one
From: Jean-Philippe Brucker @ 2018-11-29 19:28 UTC (permalink / raw)
To: xiangxia.m.yue, jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization
In-Reply-To: <1537879012-20859-2-git-send-email-xiangxia.m.yue@gmail.com>
Hi,
On 25/09/2018 13:36, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> This patch changes the way that lock all vqs
> at the same, to lock them one by one. It will
> be used for next patch to avoid the deadlock.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/vhost.c | 24 +++++++-----------------
> 1 file changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index b13c6b4..f52008b 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
> {
> int i;
>
> - for (i = 0; i < d->nvqs; ++i)
> + for (i = 0; i < d->nvqs; ++i) {
> + mutex_lock(&d->vqs[i]->mutex);
> __vhost_vq_meta_reset(d->vqs[i]);
> + mutex_unlock(&d->vqs[i]->mutex);
> + }
> }
>
> static void vhost_vq_reset(struct vhost_dev *dev,
> @@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
> #define vhost_get_used(vq, x, ptr) \
> vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
>
> -static void vhost_dev_lock_vqs(struct vhost_dev *d)
> -{
> - int i = 0;
> - for (i = 0; i < d->nvqs; ++i)
> - mutex_lock_nested(&d->vqs[i]->mutex, i);
> -}
> -
> -static void vhost_dev_unlock_vqs(struct vhost_dev *d)
> -{
> - int i = 0;
> - for (i = 0; i < d->nvqs; ++i)
> - mutex_unlock(&d->vqs[i]->mutex);
> -}
> -
> static int vhost_new_umem_range(struct vhost_umem *umem,
> u64 start, u64 size, u64 end,
> u64 userspace_addr, int perm)
> @@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
> if (msg->iova <= vq_msg->iova &&
> msg->iova + msg->size - 1 >= vq_msg->iova &&
> vq_msg->type == VHOST_IOTLB_MISS) {
> + mutex_lock(&node->vq->mutex);
This seems to introduce a deadlock (and sleep-in-atomic): the vq->mutex
is taken while the IOTLB spinlock is held (taken earlier in
vhost_iotlb_notify_vq()). On the vhost_iotlb_miss() path, the IOTLB
spinlock is taken while the vq->mutex is held.
I'm not sure how to fix it. Given that we're holding dev->mutex, that
vq->poll only seems to be modified under dev->mutex, and assuming that
vhost_poll_queue(vq->poll) can be called concurrently, is it safe to
simply not take vq->mutex here?
Thanks,
Jean
> vhost_poll_queue(&node->vq->poll);
> + mutex_unlock(&node->vq->mutex);
> +
> list_del(&node->node);
> kfree(node);
> }
> @@ -986,7 +978,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
> int ret = 0;
>
> mutex_lock(&dev->mutex);
> - vhost_dev_lock_vqs(dev);
> switch (msg->type) {
> case VHOST_IOTLB_UPDATE:
> if (!dev->iotlb) {
> @@ -1020,7 +1011,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
> break;
> }
>
> - vhost_dev_unlock_vqs(dev);
> mutex_unlock(&dev->mutex);
>
> return ret;
>
^ permalink raw reply
* Re: [REBASE PATCH net-next v9 1/4] net: vhost: lock the vqs one by one
From: Jason Wang @ 2018-11-30 2:34 UTC (permalink / raw)
To: Jean-Philippe Brucker, xiangxia.m.yue, mst, makita.toshiaki,
davem
Cc: netdev, virtualization
In-Reply-To: <f0ebc0b1-e559-5966-dbcb-800067c3260e@arm.com>
On 2018/11/30 上午3:28, Jean-Philippe Brucker wrote:
> Hi,
>
> On 25/09/2018 13:36,xiangxia.m.yue@gmail.com wrote:
>> From: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>
>> This patch changes the way that lock all vqs
>> at the same, to lock them one by one. It will
>> be used for next patch to avoid the deadlock.
>>
>> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>> Acked-by: Jason Wang<jasowang@redhat.com>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>> drivers/vhost/vhost.c | 24 +++++++-----------------
>> 1 file changed, 7 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index b13c6b4..f52008b 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
>> {
>> int i;
>>
>> - for (i = 0; i < d->nvqs; ++i)
>> + for (i = 0; i < d->nvqs; ++i) {
>> + mutex_lock(&d->vqs[i]->mutex);
>> __vhost_vq_meta_reset(d->vqs[i]);
>> + mutex_unlock(&d->vqs[i]->mutex);
>> + }
>> }
>>
>> static void vhost_vq_reset(struct vhost_dev *dev,
>> @@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
>> #define vhost_get_used(vq, x, ptr) \
>> vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
>>
>> -static void vhost_dev_lock_vqs(struct vhost_dev *d)
>> -{
>> - int i = 0;
>> - for (i = 0; i < d->nvqs; ++i)
>> - mutex_lock_nested(&d->vqs[i]->mutex, i);
>> -}
>> -
>> -static void vhost_dev_unlock_vqs(struct vhost_dev *d)
>> -{
>> - int i = 0;
>> - for (i = 0; i < d->nvqs; ++i)
>> - mutex_unlock(&d->vqs[i]->mutex);
>> -}
>> -
>> static int vhost_new_umem_range(struct vhost_umem *umem,
>> u64 start, u64 size, u64 end,
>> u64 userspace_addr, int perm)
>> @@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
>> if (msg->iova <= vq_msg->iova &&
>> msg->iova + msg->size - 1 >= vq_msg->iova &&
>> vq_msg->type == VHOST_IOTLB_MISS) {
>> + mutex_lock(&node->vq->mutex);
> This seems to introduce a deadlock (and sleep-in-atomic): the vq->mutex
> is taken while the IOTLB spinlock is held (taken earlier in
> vhost_iotlb_notify_vq()). On the vhost_iotlb_miss() path, the IOTLB
> spinlock is taken while the vq->mutex is held.
Good catch.
> I'm not sure how to fix it. Given that we're holding dev->mutex, that
> vq->poll only seems to be modified under dev->mutex, and assuming that
> vhost_poll_queue(vq->poll) can be called concurrently, is it safe to
> simply not take vq->mutex here?
Yes, I think it can be removed here.
Want to post a patch for this?
Thanks
> Thanks,
> Jean
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v3 01/13] virtio: add packed ring types and macros
From: Jason Wang @ 2018-11-30 8:10 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, netdev, virtio-dev
Cc: maxime.coquelin, wexu
In-Reply-To: <20181121100330.24846-2-tiwei.bie@intel.com>
On 2018/11/21 下午6:03, Tiwei Bie wrote:
> Add types and macros for packed ring.
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> include/uapi/linux/virtio_config.h | 3 +++
> include/uapi/linux/virtio_ring.h | 52 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index 449132c76b1c..1196e1c1d4f6 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -75,6 +75,9 @@
> */
> #define VIRTIO_F_IOMMU_PLATFORM 33
>
> +/* This feature indicates support for the packed virtqueue layout. */
> +#define VIRTIO_F_RING_PACKED 34
> +
> /*
> * Does the device support Single Root I/O Virtualization?
> */
> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> index 6d5d5faa989b..2414f8af26b3 100644
> --- a/include/uapi/linux/virtio_ring.h
> +++ b/include/uapi/linux/virtio_ring.h
> @@ -44,6 +44,13 @@
> /* This means the buffer contains a list of buffer descriptors. */
> #define VRING_DESC_F_INDIRECT 4
>
> +/*
> + * Mark a descriptor as available or used in packed ring.
> + * Notice: they are defined as shifts instead of shifted values.
This looks inconsistent to previous flags, any reason for using shifts?
> + */
> +#define VRING_PACKED_DESC_F_AVAIL 7
> +#define VRING_PACKED_DESC_F_USED 15
> +
> /* The Host uses this in used->flags to advise the Guest: don't kick me when
> * you add a buffer. It's unreliable, so it's simply an optimization. Guest
> * will still kick if it's out of buffers. */
> @@ -53,6 +60,23 @@
> * optimization. */
> #define VRING_AVAIL_F_NO_INTERRUPT 1
>
> +/* Enable events in packed ring. */
> +#define VRING_PACKED_EVENT_FLAG_ENABLE 0x0
> +/* Disable events in packed ring. */
> +#define VRING_PACKED_EVENT_FLAG_DISABLE 0x1
> +/*
> + * Enable events for a specific descriptor in packed ring.
> + * (as specified by Descriptor Ring Change Event Offset/Wrap Counter).
> + * Only valid if VIRTIO_RING_F_EVENT_IDX has been negotiated.
> + */
> +#define VRING_PACKED_EVENT_FLAG_DESC 0x2
Any reason for using _FLAG_ instead of _F_?
Thanks
> +
> +/*
> + * Wrap counter bit shift in event suppression structure
> + * of packed ring.
> + */
> +#define VRING_PACKED_EVENT_F_WRAP_CTR 15
> +
> /* We support indirect buffer descriptors */
> #define VIRTIO_RING_F_INDIRECT_DESC 28
>
> @@ -171,4 +195,32 @@ static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
> return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);
> }
>
> +struct vring_packed_desc_event {
> + /* Descriptor Ring Change Event Offset/Wrap Counter. */
> + __le16 off_wrap;
> + /* Descriptor Ring Change Event Flags. */
> + __le16 flags;
> +};
> +
> +struct vring_packed_desc {
> + /* Buffer Address. */
> + __le64 addr;
> + /* Buffer Length. */
> + __le32 len;
> + /* Buffer ID. */
> + __le16 id;
> + /* The flags depending on descriptor type. */
> + __le16 flags;
> +};
> +
> +struct vring_packed {
> + unsigned int num;
> +
> + struct vring_packed_desc *desc;
> +
> + struct vring_packed_desc_event *driver;
> +
> + struct vring_packed_desc_event *device;
> +};
> +
> #endif /* _UAPI_LINUX_VIRTIO_RING_H */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox