Re: [PATCH v2 4/4] drm/qxl: use qxl pin function
Am 29.09.20 um 12:53 schrieb Daniel Vetter:
On Tue, Sep 29, 2020 at 11:51:15AM +0200, Gerd Hoffmann wrote:
Otherwise ttm throws a WARN because we try to pin without a reservation.
Fixes: 9d36d4320462 ("drm/qxl: switch over to the new pin interface")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index d3635e3e3267..eb45267d51db 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -145,7 +145,7 @@ int qxl_bo_create(struct qxl_device *qdev,
return r;
}
if (pinned)
- ttm_bo_pin(>tbo);
+ qxl_bo_pin(bo);
I think this is now after ttm_bo_init, and at that point the object is
visible to lru users and everything. So I do think you need to grab locks
here instead of just incrementing the pin count alone.
It's also I think a bit racy, since ttm_bo_init drops the lock, so someone
might have snuck in and evicted the object already.
I think what you need is to call ttm_bo_init_reserved, then ttm_bo_pin,
then ttm_bo_unreserve, all explicitly.
Ah, yes Daniel is right. I thought I've fixed that up, but looks like I
only did that for VMWGFX.
Sorry for the noise, fix to correctly address this is underway.
Regards,
Christian.
-Daniel
*bo_ptr = bo;
return 0;
}
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/4] drm/qxl: use qxl pin function
On Tue, Sep 29, 2020 at 11:51:15AM +0200, Gerd Hoffmann wrote:
> Otherwise ttm throws a WARN because we try to pin without a reservation.
>
> Fixes: 9d36d4320462 ("drm/qxl: switch over to the new pin interface")
> Signed-off-by: Gerd Hoffmann
> ---
> drivers/gpu/drm/qxl/qxl_object.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c
> b/drivers/gpu/drm/qxl/qxl_object.c
> index d3635e3e3267..eb45267d51db 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -145,7 +145,7 @@ int qxl_bo_create(struct qxl_device *qdev,
> return r;
> }
> if (pinned)
> - ttm_bo_pin(>tbo);
> + qxl_bo_pin(bo);
I think this is now after ttm_bo_init, and at that point the object is
visible to lru users and everything. So I do think you need to grab locks
here instead of just incrementing the pin count alone.
It's also I think a bit racy, since ttm_bo_init drops the lock, so someone
might have snuck in and evicted the object already.
I think what you need is to call ttm_bo_init_reserved, then ttm_bo_pin,
then ttm_bo_unreserve, all explicitly.
-Daniel
> *bo_ptr = bo;
> return 0;
> }
> --
> 2.27.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH -next] drm/qxl: simplify the return expression of qxl_plane_prepare_fb()
On Mon, Sep 21, 2020 at 09:10:22PM +0800, Qinglang Miao wrote: > Simplify the return expression. Pushed to drm-misc-next. thanks, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/4] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 5bef8f121e54..1d9c51022be4 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1220,5 +1220,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 4/4] drm/qxl: use qxl pin function
Otherwise ttm throws a WARN because we try to pin without a reservation.
Fixes: 9d36d4320462 ("drm/qxl: switch over to the new pin interface")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index d3635e3e3267..eb45267d51db 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -145,7 +145,7 @@ int qxl_bo_create(struct qxl_device *qdev,
return r;
}
if (pinned)
- ttm_bo_pin(>tbo);
+ qxl_bo_pin(bo);
*bo_ptr = bo;
return 0;
}
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/4] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 65de1f69af58..5bef8f121e54 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1186,7 +1186,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1219,5 +1221,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/4] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 1d9c51022be4..d133e6c2aaf4 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -561,6 +561,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.27.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 4:00 AM wrote:
>
> From: Tonghao Zhang
>
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: Michael S. Tsirkin
> Cc: Jason Wang
> Cc: Willem de Bruijn
> Signed-off-by: Tonghao Zhang
Acked-by: Willem de Bruijn
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 9:56 AM Tonghao Zhang wrote: > > On Tue, Sep 29, 2020 at 3:32 PM Willem de Bruijn > wrote: > > > > On Tue, Sep 29, 2020 at 4:00 AM wrote: > > > > > > From: Tonghao Zhang > > > > > > Open vSwitch and Linux bridge will disable LRO of the interface > > > when this interface added to them. Now when disable the LRO, the > > > virtio-net csum is disable too. That drops the forwarding performance. > > > > I had focused on the code previously. > > > > The s/w checksum verification cost is significant in a VM with traffic > > to local destinations. A bridge does not verify transport layer > > checksums OTOH? > Hi Willem. > No, think about GRO(In the GRO we don't know packets will be forwarded > to other ports or to local). I had expected a pure bridging setup that disables LRO to disable GRO as well. But if not, then, indeed, the checksum needs to be verified before coalescing. Makes sense. > The call tree as below: >+ 5.41% secondary_startup_64 >- 1.22% ret_from_fork > > net_rx_action > napi_poll > virtnet_poll > virtnet_receive > napi_gro_receive > dev_gro_receive > inet_gro_receive > tcp4_gro_receive > __skb_gro_checksum_complete > skb_checksum > __skb_checksum > csum_partial > do_csum >- 1.13% do_csum > > $ brctl show > bridge name bridge id STP enabled interfaces > br0 8000.001122330001 no eth1 > eth2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
The new helper ttm_kmap_obj_to_dma_buf() extracts address and location
from and instance of TTM's kmap_obj and initializes struct dma_buf_map
with these values. Helpful for TTM-based drivers.
Signed-off-by: Thomas Zimmermann
---
include/drm/ttm/ttm_bo_api.h | 24
include/linux/dma-buf-map.h | 20
2 files changed, 44 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index c96a25d571c8..62d89f05a801 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -486,6 +487,29 @@ static inline void *ttm_kmap_obj_virtual(struct
ttm_bo_kmap_obj *map,
return map->virtual;
}
+/**
+ * ttm_kmap_obj_to_dma_buf_map
+ *
+ * @kmap: A struct ttm_bo_kmap_obj returned from ttm_bo_kmap.
+ * @map: Returns the mapping as struct dma_buf_map
+ *
+ * Converts struct ttm_bo_kmap_obj to struct dma_buf_map. If the memory
+ * is not mapped, the returned mapping is initialized to NULL.
+ */
+static inline void ttm_kmap_obj_to_dma_buf_map(struct ttm_bo_kmap_obj *kmap,
+ struct dma_buf_map *map)
+{
+ bool is_iomem;
+ void *vaddr = ttm_kmap_obj_virtual(kmap, _iomem);
+
+ if (!vaddr)
+ dma_buf_map_clear(map);
+ else if (is_iomem)
+ dma_buf_map_set_vaddr_iomem(map, (void __force __iomem *)vaddr);
+ else
+ dma_buf_map_set_vaddr(map, vaddr);
+}
+
/**
* ttm_bo_kmap
*
diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
index fd1aba545fdf..2e8bbecb5091 100644
--- a/include/linux/dma-buf-map.h
+++ b/include/linux/dma-buf-map.h
@@ -45,6 +45,12 @@
*
* dma_buf_map_set_vaddr( 0xdeadbeaf);
*
+ * To set an address in I/O memory, use dma_buf_map_set_vaddr_iomem().
+ *
+ * .. code-block:: c
+ *
+ * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
+ *
* Test if a mapping is valid with either dma_buf_map_is_set() or
* dma_buf_map_is_null().
*
@@ -118,6 +124,20 @@ static inline void dma_buf_map_set_vaddr(struct
dma_buf_map *map, void *vaddr)
map->is_iomem = false;
}
+/**
+ * dma_buf_map_set_vaddr_iomem - Sets a dma-buf mapping structure to an
address in I/O memory
+ * @map: The dma-buf mapping structure
+ * @vaddr_iomem: An I/O-memory address
+ *
+ * Sets the address and the I/O-memory flag.
+ */
+static inline void dma_buf_map_set_vaddr_iomem(struct dma_buf_map *map,
+ void __iomem *vaddr_iomem)
+{
+ map->vaddr_iomem = vaddr_iomem;
+ map->is_iomem = true;
+}
+
/**
* dma_buf_map_is_equal - Compares two dma-buf mapping structures for equality
* @lhs: The dma-buf mapping structure
--
2.28.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 5/7] drm/gem: Store client buffer mappings as struct dma_buf_map
Kernel DRM clients now store their framebuffer address in an instance
of struct dma_buf_map. Depending on the buffer's location, the address
refers to system or I/O memory.
Callers of drm_client_buffer_vmap() receive a copy of the value in
the call's supplied arguments. It can be accessed and modified with
dma_buf_map interfaces.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_client.c| 34 +++--
drivers/gpu/drm/drm_fb_helper.c | 23 +-
include/drm/drm_client.h| 7 ---
3 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index ac0082bed966..fe573acf1067 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -235,7 +235,7 @@ static void drm_client_buffer_delete(struct
drm_client_buffer *buffer)
{
struct drm_device *dev = buffer->client->dev;
- drm_gem_vunmap(buffer->gem, buffer->vaddr);
+ drm_gem_vunmap(buffer->gem, >map);
if (buffer->gem)
drm_gem_object_put(buffer->gem);
@@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev *client,
u32 width, u32 height, u
/**
* drm_client_buffer_vmap - Map DRM client buffer into address space
* @buffer: DRM client buffer
+ * @map_copy: Returns the mapped memory's address
*
* This function maps a client buffer into kernel address space. If the
- * buffer is already mapped, it returns the mapping's address.
+ * buffer is already mapped, it returns the existing mapping's address.
*
* Client buffer mappings are not ref'counted. Each call to
* drm_client_buffer_vmap() should be followed by a call to
* drm_client_buffer_vunmap(); or the client buffer should be mapped
* throughout its lifetime.
*
+ * The returned address is a copy of the internal value. In contrast to
+ * other vmap interfaces, you don't need it for the client's vunmap
+ * function. So you can modify it at will during blit and draw operations.
+ *
* Returns:
- * The mapped memory's address
+ * 0 on success, or a negative errno code otherwise.
*/
-void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
+int
+drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map
*map_copy)
{
- struct dma_buf_map map;
+ struct dma_buf_map *map = >map;
int ret;
- if (buffer->vaddr)
- return buffer->vaddr;
+ if (dma_buf_map_is_set(map))
+ goto out;
/*
* FIXME: The dependency on GEM here isn't required, we could
@@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct drm_client_buffer
*buffer)
* fd_install step out of the driver backend hooks, to make that
* final step optional for internal users.
*/
- ret = drm_gem_vmap(buffer->gem, );
+ ret = drm_gem_vmap(buffer->gem, map);
if (ret)
- return ERR_PTR(ret);
+ return ret;
- buffer->vaddr = map.vaddr;
+out:
+ *map_copy = *map;
- return map.vaddr;
+ return 0;
}
EXPORT_SYMBOL(drm_client_buffer_vmap);
@@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
*/
void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
{
- struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
+ struct dma_buf_map *map = >map;
- drm_gem_vunmap(buffer->gem, );
- buffer->vaddr = NULL;
+ drm_gem_vunmap(buffer->gem, map);
}
EXPORT_SYMBOL(drm_client_buffer_vunmap);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 8697554ccd41..343a292f2c7c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -394,7 +394,7 @@ static void drm_fb_helper_dirty_blit_real(struct
drm_fb_helper *fb_helper,
unsigned int cpp = fb->format->cpp[0];
size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
void *src = fb_helper->fbdev->screen_buffer + offset;
- void *dst = fb_helper->buffer->vaddr + offset;
+ void *dst = fb_helper->buffer->map.vaddr + offset;
size_t len = (clip->x2 - clip->x1) * cpp;
unsigned int y;
@@ -416,7 +416,8 @@ static void drm_fb_helper_dirty_work(struct work_struct
*work)
struct drm_clip_rect *clip = >dirty_clip;
struct drm_clip_rect clip_copy;
unsigned long flags;
- void *vaddr;
+ struct dma_buf_map map;
+ int ret;
spin_lock_irqsave(>dirty_lock, flags);
clip_copy = *clip;
@@ -429,8 +430,8 @@ static void drm_fb_helper_dirty_work(struct work_struct
*work)
/* Generic fbdev uses a shadow buffer */
if (helper->buffer) {
- vaddr = drm_client_buffer_vmap(helper->buffer);
- if (IS_ERR(vaddr))
+ ret = drm_client_buffer_vmap(helper->buffer, );
+ if (ret)
[PATCH v3 0/7] Support GEM object mappings from I/O memory
DRM's fbdev console uses regular load and store operations to update framebuffer memory. The bochs driver on sparc64 requires the use of I/O-specific load and store operations. We have a workaround, but need a long-term solution to the problem. This patchset changes GEM's vmap/vunmap interfaces to forward pointers of type struct dma_buf_map and updates the generic fbdev emulation to use them correctly. This enables I/O-memory operations on all framebuffers that require and support them. Patches #1 and #2 prepare VRAM helpers and TTM for the changes in patch #3. Patch #3 updates GEM's vmap/vunmap callback to forward instances of type struct dma_buf_map. While the patch touches many files throughout the DRM modules, the applied changes are mostly trivial interface fixes. Patch #4 updates GEM's internal vmap/vunmap functions to forward struct dma_buf_map. Patches #5 and #6 convert DRM clients and generic fbdev emulation to use struct dma_buf_map. Updating the fbdev framebuffer will select the correct functions, either for system or I/O memory. The patch set is just enough to fix the bochs issue on sparc64 and a correct way. Patch #7 updates the TODO list with ideas for further improvements v3: * recreate the whole patchset on top of struct dma_buf_map v2: * RFC patchset Thomas Zimmermann (7): drm/vram-helper: Remove invariant parameters from internal kmap function drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion drm/gem: Use struct dma_buf_map in GEM vmap ops and convert GEM backends drm/gem: Update internal GEM vmap/vunmap interfaces to use struct dma_buf_map drm/gem: Store client buffer mappings as struct dma_buf_map drm/fb_helper: Support framebuffers in I/O memory drm/todo: Update entries around struct dma_buf_map Documentation/gpu/todo.rst | 24 +- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h | 4 +- drivers/gpu/drm/ast/ast_cursor.c| 29 ++- drivers/gpu/drm/ast/ast_drv.h | 7 +- drivers/gpu/drm/bochs/bochs_kms.c | 1 - drivers/gpu/drm/drm_client.c| 38 ++-- drivers/gpu/drm/drm_fb_helper.c | 238 ++-- drivers/gpu/drm/drm_gem.c | 28 ++- drivers/gpu/drm/drm_gem_cma_helper.c| 14 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 48 ++-- drivers/gpu/drm/drm_gem_vram_helper.c | 93 drivers/gpu/drm/drm_internal.h | 5 +- drivers/gpu/drm/drm_prime.c | 14 +- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 4 +- drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 11 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 6 +- drivers/gpu/drm/exynos/exynos_drm_gem.h | 4 +- drivers/gpu/drm/lima/lima_gem.c | 6 +- drivers/gpu/drm/lima/lima_sched.c | 11 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 12 +- drivers/gpu/drm/nouveau/nouveau_gem.h | 4 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 9 +- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 14 +- drivers/gpu/drm/qxl/qxl_display.c | 13 +- drivers/gpu/drm/qxl/qxl_draw.c | 16 +- drivers/gpu/drm/qxl/qxl_drv.h | 8 +- drivers/gpu/drm/qxl/qxl_object.c| 23 +- drivers/gpu/drm/qxl/qxl_object.h| 2 +- drivers/gpu/drm/qxl/qxl_prime.c | 12 +- drivers/gpu/drm/radeon/radeon_gem.c | 4 +- drivers/gpu/drm/radeon/radeon_prime.c | 9 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 22 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.h | 4 +- drivers/gpu/drm/tiny/cirrus.c | 10 +- drivers/gpu/drm/tiny/gm12u320.c | 10 +- drivers/gpu/drm/udl/udl_modeset.c | 8 +- drivers/gpu/drm/vboxvideo/vbox_mode.c | 11 +- drivers/gpu/drm/vc4/vc4_bo.c| 6 +- drivers/gpu/drm/vc4/vc4_drv.h | 2 +- drivers/gpu/drm/vgem/vgem_drv.c | 16 +- drivers/gpu/drm/xen/xen_drm_front_gem.c | 18 +- drivers/gpu/drm/xen/xen_drm_front_gem.h | 6 +- include/drm/drm_client.h| 7 +- include/drm/drm_gem.h | 5 +- include/drm/drm_gem_cma_helper.h| 4 +- include/drm/drm_gem_shmem_helper.h | 4 +- include/drm/drm_gem_vram_helper.h | 4 +- include/drm/drm_mode_config.h | 12 - include/drm/ttm/ttm_bo_api.h| 24 ++ include/linux/dma-buf-map.h | 92 +++- 51 files changed, 685 insertions(+), 305 deletions(-) -- 2.28.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
On Tue, Sep 29, 2020 at 5:35 PM Christian König
wrote:
>
> Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:
> > The new helper ttm_kmap_obj_to_dma_buf() extracts address and location
> > from and instance of TTM's kmap_obj and initializes struct dma_buf_map
> > with these values. Helpful for TTM-based drivers.
>
> We could completely drop that if we use the same structure inside TTM as
> well.
> Additional to that which driver is going to use this?
Patch 3 in this series.
I also think this makes sense for gradual conversion:
1. add this helper
2. convert over all users of vmap, this should get rid of is_iomem
flags (and will probably result in a pile of small additions to
dma-buf-map.h)
3. push the struct dma_buf_map down into ttm drivers
Cheers, Daniel
> Regards,
> Christian.
>
> >
> > Signed-off-by: Thomas Zimmermann
> > ---
> > include/drm/ttm/ttm_bo_api.h | 24
> > include/linux/dma-buf-map.h | 20
> > 2 files changed, 44 insertions(+)
> >
> > diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> > index c96a25d571c8..62d89f05a801 100644
> > --- a/include/drm/ttm/ttm_bo_api.h
> > +++ b/include/drm/ttm/ttm_bo_api.h
> > @@ -34,6 +34,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -486,6 +487,29 @@ static inline void *ttm_kmap_obj_virtual(struct
> > ttm_bo_kmap_obj *map,
> > return map->virtual;
> > }
> >
> > +/**
> > + * ttm_kmap_obj_to_dma_buf_map
> > + *
> > + * @kmap: A struct ttm_bo_kmap_obj returned from ttm_bo_kmap.
> > + * @map: Returns the mapping as struct dma_buf_map
> > + *
> > + * Converts struct ttm_bo_kmap_obj to struct dma_buf_map. If the memory
> > + * is not mapped, the returned mapping is initialized to NULL.
> > + */
> > +static inline void ttm_kmap_obj_to_dma_buf_map(struct ttm_bo_kmap_obj
> > *kmap,
> > +struct dma_buf_map *map)
> > +{
> > + bool is_iomem;
> > + void *vaddr = ttm_kmap_obj_virtual(kmap, _iomem);
> > +
> > + if (!vaddr)
> > + dma_buf_map_clear(map);
> > + else if (is_iomem)
> > + dma_buf_map_set_vaddr_iomem(map, (void __force __iomem
> > *)vaddr);
> > + else
> > + dma_buf_map_set_vaddr(map, vaddr);
> > +}
> > +
> > /**
> >* ttm_bo_kmap
> >*
> > diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> > index fd1aba545fdf..2e8bbecb5091 100644
> > --- a/include/linux/dma-buf-map.h
> > +++ b/include/linux/dma-buf-map.h
> > @@ -45,6 +45,12 @@
> >*
> >* dma_buf_map_set_vaddr( 0xdeadbeaf);
> >*
> > + * To set an address in I/O memory, use dma_buf_map_set_vaddr_iomem().
> > + *
> > + * .. code-block:: c
> > + *
> > + * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> > + *
> >* Test if a mapping is valid with either dma_buf_map_is_set() or
> >* dma_buf_map_is_null().
> >*
> > @@ -118,6 +124,20 @@ static inline void dma_buf_map_set_vaddr(struct
> > dma_buf_map *map, void *vaddr)
> > map->is_iomem = false;
> > }
> >
> > +/**
> > + * dma_buf_map_set_vaddr_iomem - Sets a dma-buf mapping structure to an
> > address in I/O memory
> > + * @map: The dma-buf mapping structure
> > + * @vaddr_iomem: An I/O-memory address
> > + *
> > + * Sets the address and the I/O-memory flag.
> > + */
> > +static inline void dma_buf_map_set_vaddr_iomem(struct dma_buf_map *map,
> > +void __iomem *vaddr_iomem)
> > +{
> > + map->vaddr_iomem = vaddr_iomem;
> > + map->is_iomem = true;
> > +}
> > +
> > /**
> >* dma_buf_map_is_equal - Compares two dma-buf mapping structures for
> > equality
> >* @lhs:The dma-buf mapping structure
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:
The new helper ttm_kmap_obj_to_dma_buf() extracts address and location
from and instance of TTM's kmap_obj and initializes struct dma_buf_map
with these values. Helpful for TTM-based drivers.
We could completely drop that if we use the same structure inside TTM as
well.
Additional to that which driver is going to use this?
Regards,
Christian.
Signed-off-by: Thomas Zimmermann
---
include/drm/ttm/ttm_bo_api.h | 24
include/linux/dma-buf-map.h | 20
2 files changed, 44 insertions(+)
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index c96a25d571c8..62d89f05a801 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -486,6 +487,29 @@ static inline void *ttm_kmap_obj_virtual(struct
ttm_bo_kmap_obj *map,
return map->virtual;
}
+/**
+ * ttm_kmap_obj_to_dma_buf_map
+ *
+ * @kmap: A struct ttm_bo_kmap_obj returned from ttm_bo_kmap.
+ * @map: Returns the mapping as struct dma_buf_map
+ *
+ * Converts struct ttm_bo_kmap_obj to struct dma_buf_map. If the memory
+ * is not mapped, the returned mapping is initialized to NULL.
+ */
+static inline void ttm_kmap_obj_to_dma_buf_map(struct ttm_bo_kmap_obj *kmap,
+ struct dma_buf_map *map)
+{
+ bool is_iomem;
+ void *vaddr = ttm_kmap_obj_virtual(kmap, _iomem);
+
+ if (!vaddr)
+ dma_buf_map_clear(map);
+ else if (is_iomem)
+ dma_buf_map_set_vaddr_iomem(map, (void __force __iomem *)vaddr);
+ else
+ dma_buf_map_set_vaddr(map, vaddr);
+}
+
/**
* ttm_bo_kmap
*
diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
index fd1aba545fdf..2e8bbecb5091 100644
--- a/include/linux/dma-buf-map.h
+++ b/include/linux/dma-buf-map.h
@@ -45,6 +45,12 @@
*
*dma_buf_map_set_vaddr( 0xdeadbeaf);
*
+ * To set an address in I/O memory, use dma_buf_map_set_vaddr_iomem().
+ *
+ * .. code-block:: c
+ *
+ * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
+ *
* Test if a mapping is valid with either dma_buf_map_is_set() or
* dma_buf_map_is_null().
*
@@ -118,6 +124,20 @@ static inline void dma_buf_map_set_vaddr(struct
dma_buf_map *map, void *vaddr)
map->is_iomem = false;
}
+/**
+ * dma_buf_map_set_vaddr_iomem - Sets a dma-buf mapping structure to an
address in I/O memory
+ * @map: The dma-buf mapping structure
+ * @vaddr_iomem: An I/O-memory address
+ *
+ * Sets the address and the I/O-memory flag.
+ */
+static inline void dma_buf_map_set_vaddr_iomem(struct dma_buf_map *map,
+ void __iomem *vaddr_iomem)
+{
+ map->vaddr_iomem = vaddr_iomem;
+ map->is_iomem = true;
+}
+
/**
* dma_buf_map_is_equal - Compares two dma-buf mapping structures for equality
* @lhs: The dma-buf mapping structure
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 7/7] drm/todo: Update entries around struct dma_buf_map
Instances of struct dma_buf_map should be useful throughout DRM's memory management code. Furthermore, several drivers can now use generic fbdev emulation. Signed-off-by: Thomas Zimmermann --- Documentation/gpu/todo.rst | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 3751ac976c3e..023626c1837b 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -197,8 +197,10 @@ Convert drivers to use drm_fbdev_generic_setup() Most drivers can use drm_fbdev_generic_setup(). Driver have to implement -atomic modesetting and GEM vmap support. Current generic fbdev emulation -expects the framebuffer in system memory (or system-like memory). +atomic modesetting and GEM vmap support. Historically, generic fbdev emulation +expected the framebuffer in system memory or system-like memory. By employing +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported +as well. Contact: Maintainer of the driver you plan to convert @@ -446,6 +448,24 @@ Contact: Ville Syrjälä, Daniel Vetter Level: Intermediate +Use struct dma_buf_map throughout codebase +-- + +Pointers to shared device memory are stored in struct dma_buf_map. Each +instance knows whether it refers to system or I/O memory. Most of the DRM-wide +interface have been converted to use struct dma_buf_map, but implementations +often still use raw pointers. + +The task is to use struct dma_buf_map where it makes sense. + +* Memory managers should use struct dma_buf_map for dma-buf-imported buffers. +* TTM might benefit from using struct dma_buf_map internally. +* Framebuffer copying and blitting helpers should operate on struct dma_buf_map. + +Contact: Thomas Zimmermann , Christian König, Daniel Vetter + +Level: Intermediate + Core refactorings = -- 2.28.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 3/7] drm/gem: Use struct dma_buf_map in GEM vmap ops and convert GEM backends
This patch replaces the vmap/vunmap's use of raw pointers in GEM object
functions with instances of struct dma_buf_map. GEM backends are
converted as well.
For most GEM backends, this simply change the returned type. GEM VRAM
helpers are also updated to indicate whether the returned framebuffer
address is in system or I/O memory.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h | 4 +-
drivers/gpu/drm/ast/ast_cursor.c| 29 +++
drivers/gpu/drm/ast/ast_drv.h | 7 +-
drivers/gpu/drm/drm_gem.c | 22 ++---
drivers/gpu/drm/drm_gem_cma_helper.c| 14 ++--
drivers/gpu/drm/drm_gem_shmem_helper.c | 48 ++-
drivers/gpu/drm/drm_gem_vram_helper.c | 90 +++--
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 4 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 11 ++-
drivers/gpu/drm/exynos/exynos_drm_gem.c | 6 +-
drivers/gpu/drm/exynos/exynos_drm_gem.h | 4 +-
drivers/gpu/drm/lima/lima_gem.c | 6 +-
drivers/gpu/drm/lima/lima_sched.c | 11 ++-
drivers/gpu/drm/mgag200/mgag200_mode.c | 12 +--
drivers/gpu/drm/nouveau/nouveau_gem.h | 4 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 9 ++-
drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 14 ++--
drivers/gpu/drm/qxl/qxl_display.c | 13 +--
drivers/gpu/drm/qxl/qxl_draw.c | 16 ++--
drivers/gpu/drm/qxl/qxl_drv.h | 8 +-
drivers/gpu/drm/qxl/qxl_object.c| 23 +++---
drivers/gpu/drm/qxl/qxl_object.h| 2 +-
drivers/gpu/drm/qxl/qxl_prime.c | 12 +--
drivers/gpu/drm/radeon/radeon_gem.c | 4 +-
drivers/gpu/drm/radeon/radeon_prime.c | 9 ++-
drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 22 +++--
drivers/gpu/drm/rockchip/rockchip_drm_gem.h | 4 +-
drivers/gpu/drm/tiny/cirrus.c | 10 ++-
drivers/gpu/drm/tiny/gm12u320.c | 10 ++-
drivers/gpu/drm/udl/udl_modeset.c | 8 +-
drivers/gpu/drm/vboxvideo/vbox_mode.c | 11 ++-
drivers/gpu/drm/vc4/vc4_bo.c| 6 +-
drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
drivers/gpu/drm/vgem/vgem_drv.c | 16 ++--
drivers/gpu/drm/xen/xen_drm_front_gem.c | 18 +++--
drivers/gpu/drm/xen/xen_drm_front_gem.h | 6 +-
include/drm/drm_gem.h | 5 +-
include/drm/drm_gem_cma_helper.h| 4 +-
include/drm/drm_gem_shmem_helper.h | 4 +-
include/drm/drm_gem_vram_helper.h | 4 +-
41 files changed, 304 insertions(+), 222 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 5b465ab774d1..de7d0cfe1b93 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -44,13 +44,14 @@
/**
* amdgpu_gem_prime_vmap - _buf_ops.vmap implementation
* @obj: GEM BO
+ * @map: The virtual address of the mapping.
*
* Sets up an in-kernel virtual mapping of the BO's memory.
*
* Returns:
- * The virtual address of the mapping or an error pointer.
+ * 0 on success, or a negative errno code otherwise.
*/
-void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj)
+int amdgpu_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
{
struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
int ret;
@@ -58,19 +59,20 @@ void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj)
ret = ttm_bo_kmap(>tbo, 0, bo->tbo.num_pages,
>dma_buf_vmap);
if (ret)
- return ERR_PTR(ret);
+ return ret;
+ ttm_kmap_obj_to_dma_buf_map(>dma_buf_vmap, map);
- return bo->dma_buf_vmap.virtual;
+ return 0;
}
/**
* amdgpu_gem_prime_vunmap - _buf_ops.vunmap implementation
* @obj: GEM BO
- * @vaddr: Virtual address (unused)
+ * @map: Virtual address (unused)
*
* Tears down the in-kernel virtual mapping of the BO's memory.
*/
-void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
+void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, struct dma_buf_map
*map)
{
struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
index 2c5c84a06bb9..622642793064 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
@@ -31,8 +31,8 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct
drm_device *dev,
struct dma_buf *dma_buf);
bool amdgpu_dmabuf_is_xgmi_accessible(struct amdgpu_device *adev,
struct amdgpu_bo *bo);
-void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj);
-void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
+int amdgpu_gem_prime_vmap(struct drm_gem_object
[PATCH v3 4/7] drm/gem: Update internal GEM vmap/vunmap interfaces to use struct dma_buf_map
GEM's vmap and vunmap interfaces now wrap memory pointers in struct
dma_buf_map.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_client.c | 18 +++---
drivers/gpu/drm/drm_gem.c | 28 ++--
drivers/gpu/drm/drm_internal.h | 5 +++--
drivers/gpu/drm/drm_prime.c| 14 --
4 files changed, 32 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 495f47d23d87..ac0082bed966 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -3,6 +3,7 @@
* Copyright 2018 Noralf Trønnes
*/
+#include
#include
#include
#include
@@ -304,7 +305,8 @@ drm_client_buffer_create(struct drm_client_dev *client, u32
width, u32 height, u
*/
void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
{
- void *vaddr;
+ struct dma_buf_map map;
+ int ret;
if (buffer->vaddr)
return buffer->vaddr;
@@ -317,13 +319,13 @@ void *drm_client_buffer_vmap(struct drm_client_buffer
*buffer)
* fd_install step out of the driver backend hooks, to make that
* final step optional for internal users.
*/
- vaddr = drm_gem_vmap(buffer->gem);
- if (IS_ERR(vaddr))
- return vaddr;
+ ret = drm_gem_vmap(buffer->gem, );
+ if (ret)
+ return ERR_PTR(ret);
- buffer->vaddr = vaddr;
+ buffer->vaddr = map.vaddr;
- return vaddr;
+ return map.vaddr;
}
EXPORT_SYMBOL(drm_client_buffer_vmap);
@@ -337,7 +339,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
*/
void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
{
- drm_gem_vunmap(buffer->gem, buffer->vaddr);
+ struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
+
+ drm_gem_vunmap(buffer->gem, );
buffer->vaddr = NULL;
}
EXPORT_SYMBOL(drm_client_buffer_vunmap);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 0c4a66dea5c2..f2b2f37d41c4 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1205,32 +1205,32 @@ void drm_gem_unpin(struct drm_gem_object *obj)
obj->funcs->unpin(obj);
}
-void *drm_gem_vmap(struct drm_gem_object *obj)
+int drm_gem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
{
- struct dma_buf_map map;
int ret;
- if (!obj->funcs->vmap) {
- return ERR_PTR(-EOPNOTSUPP);
+ if (!obj->funcs->vmap)
+ return -EOPNOTSUPP;
- ret = obj->funcs->vmap(obj, );
+ ret = obj->funcs->vmap(obj, map);
if (ret)
- return ERR_PTR(ret);
- else if (dma_buf_map_is_null())
- return ERR_PTR(-ENOMEM);
+ return ret;
+ else if (dma_buf_map_is_null(map))
+ return -ENOMEM;
- return map.vaddr;
+ return 0;
}
-void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
+void drm_gem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map)
{
- struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(vaddr);
-
- if (!vaddr)
+ if (dma_buf_map_is_null(map))
return;
if (obj->funcs->vunmap)
- obj->funcs->vunmap(obj, );
+ obj->funcs->vunmap(obj, map);
+
+ /* Always set the mapping to NULL. Callers may rely on this. */
+ dma_buf_map_clear(map);
}
/**
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index b65865c630b0..58832d75a9bd 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -33,6 +33,7 @@
struct dentry;
struct dma_buf;
+struct dma_buf_map;
struct drm_connector;
struct drm_crtc;
struct drm_framebuffer;
@@ -187,8 +188,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int
indent,
int drm_gem_pin(struct drm_gem_object *obj);
void drm_gem_unpin(struct drm_gem_object *obj);
-void *drm_gem_vmap(struct drm_gem_object *obj);
-void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
+int drm_gem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
+void drm_gem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map);
/* drm_debugfs.c drm_debugfs_crc.c */
#if defined(CONFIG_DEBUG_FS)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 89e2a2496734..cb8fbeeb731b 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -667,21 +667,15 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
*
* Sets up a kernel virtual mapping. This can be used as the _buf_ops.vmap
* callback. Calls into _gem_object_funcs.vmap for device specific
handling.
+ * The kernel virtual address is returned in map.
*
- * Returns the kernel virtual address or NULL on failure.
+ * Returns 0 on success or a negative errno code otherwise.
*/
int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
{
struct drm_gem_object *obj = dma_buf->priv;
-
[PATCH v3 6/7] drm/fb_helper: Support framebuffers in I/O memory
At least sparc64 requires I/O-specific access to framebuffers. This
patch updates the fbdev console accordingly.
For drivers with direct access to the framebuffer memory, the callback
functions in struct fb_ops test for the type of memory and call the rsp
fb_sys_ of fb_cfb_ functions.
For drivers that employ a shadow buffer, fbdev's blit function retrieves
the framebuffer address as struct dma_buf_map, and uses dma_buf_map
interfaces to access the buffer.
The bochs driver on sparc64 uses a workaround to flag the framebuffer as
I/O memory and avoid a HW exception. With the introduction of struct
dma_buf_map, this is not required any longer. The patch removes the rsp
code from both, bochs and fbdev.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/bochs/bochs_kms.c | 1 -
drivers/gpu/drm/drm_fb_helper.c | 217 --
include/drm/drm_mode_config.h | 12 --
include/linux/dma-buf-map.h | 72 --
4 files changed, 265 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
b/drivers/gpu/drm/bochs/bochs_kms.c
index 13d0d04c4457..853081d186d5 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
bochs->dev->mode_config.preferred_depth = 24;
bochs->dev->mode_config.prefer_shadow = 0;
bochs->dev->mode_config.prefer_shadow_fbdev = 1;
- bochs->dev->mode_config.fbdev_use_iomem = true;
bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
bochs->dev->mode_config.funcs = _mode_funcs;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 343a292f2c7c..f345a314a437 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -388,24 +388,22 @@ static void drm_fb_helper_resume_worker(struct
work_struct *work)
}
static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
- struct drm_clip_rect *clip)
+ struct drm_clip_rect *clip,
+ struct dma_buf_map *dst)
{
struct drm_framebuffer *fb = fb_helper->fb;
unsigned int cpp = fb->format->cpp[0];
size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
void *src = fb_helper->fbdev->screen_buffer + offset;
- void *dst = fb_helper->buffer->map.vaddr + offset;
size_t len = (clip->x2 - clip->x1) * cpp;
unsigned int y;
- for (y = clip->y1; y < clip->y2; y++) {
- if (!fb_helper->dev->mode_config.fbdev_use_iomem)
- memcpy(dst, src, len);
- else
- memcpy_toio((void __iomem *)dst, src, len);
+ dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */
+ for (y = clip->y1; y < clip->y2; y++) {
+ dma_buf_map_memcpy_to(dst, src, len);
+ dma_buf_map_incr(dst, fb->pitches[0]);
src += fb->pitches[0];
- dst += fb->pitches[0];
}
}
@@ -433,8 +431,9 @@ static void drm_fb_helper_dirty_work(struct work_struct
*work)
ret = drm_client_buffer_vmap(helper->buffer, );
if (ret)
return;
- drm_fb_helper_dirty_blit_real(helper, _copy);
+ drm_fb_helper_dirty_blit_real(helper, _copy, );
}
+
if (helper->fb->funcs->dirty)
helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
_copy, 1);
@@ -771,6 +770,136 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
}
EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
+static ssize_t drm_fb_helper_cfb_read(struct fb_info *info, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ unsigned long p = *ppos;
+ u8 *dst;
+ u8 __iomem *src;
+ int c, err = 0;
+ unsigned long total_size;
+ unsigned long alloc_size;
+ ssize_t ret = 0;
+
+ if (info->state != FBINFO_STATE_RUNNING)
+ return -EPERM;
+
+ total_size = info->screen_size;
+
+ if (total_size == 0)
+ total_size = info->fix.smem_len;
+
+ if (p >= total_size)
+ return 0;
+
+ if (count >= total_size)
+ count = total_size;
+
+ if (count + p > total_size)
+ count = total_size - p;
+
+ src = (u8 __iomem *)(info->screen_base + p);
+
+ alloc_size = min(count, PAGE_SIZE);
+
+ dst = kmalloc(alloc_size, GFP_KERNEL);
+ if (!dst)
+ return -ENOMEM;
+
+ while (count) {
+ c = min(count, alloc_size);
+
+ memcpy_fromio(dst, src, c);
+ if (copy_to_user(buf, dst, c)) {
+ err =
[PATCH v3 1/7] drm/vram-helper: Remove invariant parameters from internal kmap function
The parameters map and is_iomem are always of the same value. Removed them
to prepares the function for conversion to struct dma_buf_map.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_vram_helper.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
b/drivers/gpu/drm/drm_gem_vram_helper.c
index 3fe4b326e18e..256b346664f2 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -382,16 +382,16 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
}
EXPORT_SYMBOL(drm_gem_vram_unpin);
-static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo,
- bool map, bool *is_iomem)
+static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo)
{
int ret;
struct ttm_bo_kmap_obj *kmap = >kmap;
+ bool is_iomem;
if (gbo->kmap_use_count > 0)
goto out;
- if (kmap->virtual || !map)
+ if (kmap->virtual)
goto out;
ret = ttm_bo_kmap(>bo, 0, gbo->bo.num_pages, kmap);
@@ -399,15 +399,10 @@ static void *drm_gem_vram_kmap_locked(struct
drm_gem_vram_object *gbo,
return ERR_PTR(ret);
out:
- if (!kmap->virtual) {
- if (is_iomem)
- *is_iomem = false;
+ if (!kmap->virtual)
return NULL; /* not mapped; don't increment ref */
- }
++gbo->kmap_use_count;
- if (is_iomem)
- return ttm_kmap_obj_virtual(kmap, is_iomem);
- return kmap->virtual;
+ return ttm_kmap_obj_virtual(kmap, _iomem);
}
static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo)
@@ -452,7 +447,7 @@ void *drm_gem_vram_vmap(struct drm_gem_vram_object *gbo)
ret = drm_gem_vram_pin_locked(gbo, 0);
if (ret)
goto err_ttm_bo_unreserve;
- base = drm_gem_vram_kmap_locked(gbo, true, NULL);
+ base = drm_gem_vram_kmap_locked(gbo);
if (IS_ERR(base)) {
ret = PTR_ERR(base);
goto err_drm_gem_vram_unpin_locked;
--
2.28.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
From: xiangxia.m@gmail.com
Date: Tue, 29 Sep 2020 09:58:06 +0800
> From: Tonghao Zhang
>
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: Michael S. Tsirkin
> Cc: Jason Wang
> Cc: Willem de Bruijn
> Signed-off-by: Tonghao Zhang
Applied and queued up for -stable, thank you.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixes
The pull request you sent on Tue, 29 Sep 2020 03:50:34 -0400: > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/1ccfa66d94cf65d3e89eeb95676a03e8f90edd99 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion
Hi Christian
Am 29.09.20 um 17:35 schrieb Christian König:
> Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:
>> The new helper ttm_kmap_obj_to_dma_buf() extracts address and location
>> from and instance of TTM's kmap_obj and initializes struct dma_buf_map
>> with these values. Helpful for TTM-based drivers.
>
> We could completely drop that if we use the same structure inside TTM as
> well.
>
> Additional to that which driver is going to use this?
As Daniel mentioned, it's in patch 3. The TTM-based drivers will
retrieve the pointer via this function.
I do want to see all that being more tightly integrated into TTM, but
not in this series. This one is about fixing the bochs-on-sparc64
problem for good. Patch 7 adds an update to TTM to the DRM TODO list.
Best regards
Thomas
>
> Regards,
> Christian.
>
>>
>> Signed-off-by: Thomas Zimmermann
>> ---
>> include/drm/ttm/ttm_bo_api.h | 24
>> include/linux/dma-buf-map.h | 20
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>> index c96a25d571c8..62d89f05a801 100644
>> --- a/include/drm/ttm/ttm_bo_api.h
>> +++ b/include/drm/ttm/ttm_bo_api.h
>> @@ -34,6 +34,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -486,6 +487,29 @@ static inline void *ttm_kmap_obj_virtual(struct
>> ttm_bo_kmap_obj *map,
>> return map->virtual;
>> }
>> +/**
>> + * ttm_kmap_obj_to_dma_buf_map
>> + *
>> + * @kmap: A struct ttm_bo_kmap_obj returned from ttm_bo_kmap.
>> + * @map: Returns the mapping as struct dma_buf_map
>> + *
>> + * Converts struct ttm_bo_kmap_obj to struct dma_buf_map. If the memory
>> + * is not mapped, the returned mapping is initialized to NULL.
>> + */
>> +static inline void ttm_kmap_obj_to_dma_buf_map(struct ttm_bo_kmap_obj
>> *kmap,
>> + struct dma_buf_map *map)
>> +{
>> + bool is_iomem;
>> + void *vaddr = ttm_kmap_obj_virtual(kmap, _iomem);
>> +
>> + if (!vaddr)
>> + dma_buf_map_clear(map);
>> + else if (is_iomem)
>> + dma_buf_map_set_vaddr_iomem(map, (void __force __iomem *)vaddr);
>> + else
>> + dma_buf_map_set_vaddr(map, vaddr);
>> +}
>> +
>> /**
>> * ttm_bo_kmap
>> *
>> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
>> index fd1aba545fdf..2e8bbecb5091 100644
>> --- a/include/linux/dma-buf-map.h
>> +++ b/include/linux/dma-buf-map.h
>> @@ -45,6 +45,12 @@
>> *
>> * dma_buf_map_set_vaddr( 0xdeadbeaf);
>> *
>> + * To set an address in I/O memory, use dma_buf_map_set_vaddr_iomem().
>> + *
>> + * .. code-block:: c
>> + *
>> + * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
>> + *
>> * Test if a mapping is valid with either dma_buf_map_is_set() or
>> * dma_buf_map_is_null().
>> *
>> @@ -118,6 +124,20 @@ static inline void dma_buf_map_set_vaddr(struct
>> dma_buf_map *map, void *vaddr)
>> map->is_iomem = false;
>> }
>> +/**
>> + * dma_buf_map_set_vaddr_iomem - Sets a dma-buf mapping structure to
>> an address in I/O memory
>> + * @map: The dma-buf mapping structure
>> + * @vaddr_iomem: An I/O-memory address
>> + *
>> + * Sets the address and the I/O-memory flag.
>> + */
>> +static inline void dma_buf_map_set_vaddr_iomem(struct dma_buf_map *map,
>> + void __iomem *vaddr_iomem)
>> +{
>> + map->vaddr_iomem = vaddr_iomem;
>> + map->is_iomem = true;
>> +}
>> +
>> /**
>> * dma_buf_map_is_equal - Compares two dma-buf mapping structures
>> for equality
>> * @lhs: The dma-buf mapping structure
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
signature.asc
Description: OpenPGP digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
On Tue, Sep 29, 2020 at 3:25 PM Michael S. Tsirkin wrote:
>
> On Tue, Sep 29, 2020 at 03:17:50PM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 2:22 PM Michael S. Tsirkin wrote:
> > >
> > > On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang wrote:
> > > > On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > > > > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin
> > > > > > wrote:
> > > > > > >
> > > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800,
> > > > > > > xiangxia.m@gmail.com wrote:
> > > > > > > > From: Tonghao Zhang
> > > > > > > >
> > > > > > > > Allow user configuring RXCSUM separately with ethtool -K,
> > > > > > > > reusing the existing virtnet_set_guest_offloads helper
> > > > > > > > that configures RXCSUM for XDP. This is conditional on
> > > > > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > > > >
> > > > > > > > Cc: Michael S. Tsirkin
> > > > > > > > Cc: Jason Wang
> > > > > > > > Signed-off-by: Tonghao Zhang
> > > > > > > > ---
> > > > > > > > drivers/net/virtio_net.c | 40
> > > > > > > >
> > > > > > > > 1 file changed, 28 insertions(+), 12 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > > @@ -68,6 +68,8 @@ static const unsigned long guest_offloads[] =
> > > > > > > > {
> > > > > > > > (1ULL << VIRTIO_NET_F_GUEST_ECN)
> > > > > > > > | \
> > > > > > > > (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > > > > >
> > > > > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL <<
> > > > > > > > VIRTIO_NET_F_GUEST_CSUM)
> > > > > > > > +
> > > > > > > > struct virtnet_stat_desc {
> > > > > > > > char desc[ETH_GSTRING_LEN];
> > > > > > > > size_t offset;
> > > > > > > > @@ -2526,25 +2528,37 @@ static int virtnet_set_features(struct
> > > > > > > > net_device *dev,
> > > > > > > > netdev_features_t features)
> > > > > > > > {
> > > > > > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > > > > > - u64 offloads;
> > > > > > > > + u64 offloads = vi->guest_offloads &
> > > > > > > > +vi->guest_offloads_capable;
> > > > > > > > int err;
> > > > > > > >
> > > > > > > > - if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > > > > - if (vi->xdp_queue_pairs)
> > > > > > > > - return -EBUSY;
> > > > > > > > + /* Don't allow configuration while XDP is active. */
> > > > > > > > + if (vi->xdp_queue_pairs)
> > > > > > > > + return -EBUSY;
> > > > > > > >
> > > > > > > > + if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > > > > if (features & NETIF_F_LRO)
> > > > > > > > - offloads = vi->guest_offloads_capable;
> > > > > > > > + offloads |= GUEST_OFFLOAD_LRO_MASK;
> > > > > > > > else
> > > > > > > > - offloads = vi->guest_offloads_capable &
> > > > > > > > -~GUEST_OFFLOAD_LRO_MASK;
> > > > > > > > + offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > - err = virtnet_set_guest_offloads(vi, offloads);
> > > > > > > > - if (err)
> > > > > > > > - return err;
> > > > > > > > - vi->guest_offloads = offloads;
> > > > > > > > + if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> > > > > > > > + if (features & NETIF_F_RXCSUM)
> > > > > > > > + offloads |= GUEST_OFFLOAD_CSUM_MASK;
> > > > > > > > + else
> > > > > > > > + offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + if (offloads == (vi->guest_offloads &
> > > > > > > > + vi->guest_offloads_capable))
> > > > > > > > + return 0;
> > > > > > >
> > > > > > > Hmm, what exactly does this do?
> > > > > > If the features(lro, rxcsum) we supported, are not changed, it is
> > > > > > not
> > > > > > necessary to invoke virtnet_set_guest_offloads.
> > > > >
> > > > > okay, could you describe the cases where this triggers in a bit more
> > > > > detail pls?
> > > > Hi
> > > > As I known, when we run che commands show as below:
> > > > ethtool -K eth1 sg off
> > > > ethtool -K eth1 tso off
> > > >
> > > > In that case, we will not invoke virtnet_set_guest_offloads.
> > >
> > > How about initialization though? E.g. it looks like guest_offloads
> > > is 0-initialized, won't this skip the first command to disable
> > > offloads?
> > I guest you mean that: if guest_offloads == 0, and run
[PATCH net-next v2] virtio-net: ethtool configurable RXCSUM
From: Tonghao Zhang
Allow user configuring RXCSUM separately with ethtool -K,
reusing the existing virtnet_set_guest_offloads helper
that configures RXCSUM for XDP. This is conditional on
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
If Rx checksum is disabled, LRO should also be disabled.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Signed-off-by: Tonghao Zhang
---
v2:
* LRO depends the rx csum
* remove the unnecessary check
---
drivers/net/virtio_net.c | 49 ++--
1 file changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 21b71148c532..5407a0106771 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -68,6 +68,8 @@ static const unsigned long guest_offloads[] = {
(1ULL << VIRTIO_NET_F_GUEST_ECN) | \
(1ULL << VIRTIO_NET_F_GUEST_UFO))
+#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
+
struct virtnet_stat_desc {
char desc[ETH_GSTRING_LEN];
size_t offset;
@@ -2522,29 +2524,49 @@ static int virtnet_get_phys_port_name(struct net_device
*dev, char *buf,
return 0;
}
+static netdev_features_t virtnet_fix_features(struct net_device *netdev,
+ netdev_features_t features)
+{
+ /* If Rx checksum is disabled, LRO should also be disabled.
+* That is life. :)
+*/
+ if (!(features & NETIF_F_RXCSUM))
+ features &= ~NETIF_F_LRO;
+
+ return features;
+}
+
static int virtnet_set_features(struct net_device *dev,
netdev_features_t features)
{
struct virtnet_info *vi = netdev_priv(dev);
- u64 offloads;
+ u64 offloads = vi->guest_offloads &
+ vi->guest_offloads_capable;
int err;
- if ((dev->features ^ features) & NETIF_F_LRO) {
- if (vi->xdp_queue_pairs)
- return -EBUSY;
+ /* Don't allow configuration while XDP is active. */
+ if (vi->xdp_queue_pairs)
+ return -EBUSY;
+ if ((dev->features ^ features) & NETIF_F_LRO) {
if (features & NETIF_F_LRO)
- offloads = vi->guest_offloads_capable;
+ offloads |= GUEST_OFFLOAD_LRO_MASK;
else
- offloads = vi->guest_offloads_capable &
- ~GUEST_OFFLOAD_LRO_MASK;
+ offloads &= ~GUEST_OFFLOAD_LRO_MASK;
+ }
- err = virtnet_set_guest_offloads(vi, offloads);
- if (err)
- return err;
- vi->guest_offloads = offloads;
+ if ((dev->features ^ features) & NETIF_F_RXCSUM) {
+ if (features & NETIF_F_RXCSUM)
+ offloads |= GUEST_OFFLOAD_CSUM_MASK;
+ else
+ offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
}
+ err = virtnet_set_guest_offloads(vi, offloads);
+ if (err)
+ return err;
+
+ vi->guest_offloads = offloads;
return 0;
}
@@ -2563,6 +2585,7 @@ static const struct net_device_ops virtnet_netdev = {
.ndo_features_check = passthru_features_check,
.ndo_get_phys_port_name = virtnet_get_phys_port_name,
.ndo_set_features = virtnet_set_features,
+ .ndo_fix_features = virtnet_fix_features,
};
static void virtnet_config_changed_work(struct work_struct *work)
@@ -3013,8 +3036,10 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
dev->features |= NETIF_F_LRO;
- if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
+ dev->hw_features |= NETIF_F_RXCSUM;
dev->hw_features |= NETIF_F_LRO;
+ }
dev->vlan_features = dev->features;
--
2.23.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V1 vhost-next] vdpa/mlx5: Make vdpa core driver a distinct module
On Tue, Sep 29, 2020 at 09:34:33AM +0300, Eli Cohen wrote:
> On Tue, Sep 29, 2020 at 02:26:44AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Sep 29, 2020 at 09:20:26AM +0300, Eli Cohen wrote:
> > > On Mon, Sep 28, 2020 at 03:55:09PM -0400, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 24, 2020 at 05:32:31PM +0300, Eli Cohen wrote:
> > > > > Change core vdpa functionality into a loadbale module such that
> > > > > upcoming
> > > > > block implementation will be able to use it.
> > > > >
> > > > > Signed-off-by: Eli Cohen
> > > >
> > > > Why don't we merge this patch together with the block module?
> > > >
> > >
> > > Since there are still not too many users of this driver, I would prefer
> > > to merge this as early as possible so pepole get used to the involved
> > > modules.
> > >
> > > Anyways, I will send another version of the patch which makes use of
> > > 'select' instead of 'depends'.
> > >
> > > Hope you agree to merge this.
> >
> > Are you quite sure there will be a block driver though?
> > I'd like to avoid a situation in which we have infrastructure
> > in place but no users.
> >
>
> I know it's in our plans but I see your point. Let me know if you
> prefer me to send the patch that fixes the 'depends' thing or defer it
> to a later time.
Not sure what's the depends thing.
> > > > > ---
> > > > > V0 --> V1:
> > > > > Removed "default n" for configu options as 'n' is the default
> > > > >
> > > > > drivers/vdpa/Kconfig | 8 +++-
> > > > > drivers/vdpa/Makefile | 2 +-
> > > > > drivers/vdpa/mlx5/Makefile | 7 +--
> > > > > drivers/vdpa/mlx5/core/core_main.c | 20
> > > > > drivers/vdpa/mlx5/core/mr.c| 3 +++
> > > > > drivers/vdpa/mlx5/core/resources.c | 10 ++
> > > > > 6 files changed, 42 insertions(+), 8 deletions(-)
> > > > > create mode 100644 drivers/vdpa/mlx5/core/core_main.c
> > > > >
> > > > > diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
> > > > > index 4271c408103e..57ff6a7f7401 100644
> > > > > --- a/drivers/vdpa/Kconfig
> > > > > +++ b/drivers/vdpa/Kconfig
> > > > > @@ -29,10 +29,9 @@ config IFCVF
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will
> > > > > be called ifcvf.
> > > > >
> > > > > -config MLX5_VDPA
> > > > > - bool "MLX5 VDPA support library for ConnectX devices"
> > > > > +config MLX5_VDPA_CORE
> > > > > + tristate "MLX5 VDPA support library for ConnectX devices"
> > > > > depends on MLX5_CORE
> > > > > - default n
> > > > > help
> > > > > Support library for Mellanox VDPA drivers. Provides code that
> > > > > is
> > > > > common for all types of VDPA drivers. The following drivers
> > > > > are planned:
> > > > > @@ -40,8 +39,7 @@ config MLX5_VDPA
> > > > >
> > > > > config MLX5_VDPA_NET
> > > > > tristate "vDPA driver for ConnectX devices"
> > > > > - depends on MLX5_VDPA
> > > > > - default n
> > > > > + depends on MLX5_VDPA_CORE
> > > > > help
> > > > > VDPA network driver for ConnectX6 and newer. Provides
> > > > > offloading
> > > > > of virtio net datapath such that descriptors put on the ring
> > > > > will
> > > > > diff --git a/drivers/vdpa/Makefile b/drivers/vdpa/Makefile
> > > > > index d160e9b63a66..07353bbb9f8b 100644
> > > > > --- a/drivers/vdpa/Makefile
> > > > > +++ b/drivers/vdpa/Makefile
> > > > > @@ -2,4 +2,4 @@
> > > > > obj-$(CONFIG_VDPA) += vdpa.o
> > > > > obj-$(CONFIG_VDPA_SIM) += vdpa_sim/
> > > > > obj-$(CONFIG_IFCVF)+= ifcvf/
> > > > > -obj-$(CONFIG_MLX5_VDPA) += mlx5/
> > > > > +obj-$(CONFIG_MLX5_VDPA_CORE) += mlx5/
> > > > > diff --git a/drivers/vdpa/mlx5/Makefile b/drivers/vdpa/mlx5/Makefile
> > > > > index 89a5bededc9f..9f50f7e8d889 100644
> > > > > --- a/drivers/vdpa/mlx5/Makefile
> > > > > +++ b/drivers/vdpa/mlx5/Makefile
> > > > > @@ -1,4 +1,7 @@
> > > > > subdir-ccflags-y += -I$(srctree)/drivers/vdpa/mlx5/core
> > > > >
> > > > > -obj-$(CONFIG_MLX5_VDPA_NET) += mlx5_vdpa.o
> > > > > -mlx5_vdpa-$(CONFIG_MLX5_VDPA_NET) += net/main.o net/mlx5_vnet.o
> > > > > core/resources.o core/mr.o
> > > > > +obj-$(CONFIG_MLX5_VDPA_CORE) += mlx5_vdpa_core.o
> > > > > +mlx5_vdpa_core-$(CONFIG_MLX5_VDPA_CORE) += core/resources.o
> > > > > core/mr.o core/core_main.o
> > > > > +
> > > > > +obj-$(CONFIG_MLX5_VDPA_NET) += mlx5_vdpa_net.o
> > > > > +mlx5_vdpa_net-$(CONFIG_MLX5_VDPA_NET) += net/main.o net/mlx5_vnet.o
> > > > > diff --git a/drivers/vdpa/mlx5/core/core_main.c
> > > > > b/drivers/vdpa/mlx5/core/core_main.c
> > > > > new file mode 100644
> > > > > index ..4b39b55f57ab
> > > > > --- /dev/null
> > > > > +++ b/drivers/vdpa/mlx5/core/core_main.c
> > > > > @@ -0,0 +1,20 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> > > > > +/* Copyright (c) 2020 Mellanox Technologies Ltd. */
> > > > > +
> > > > > +#include
> > > > > +
> > > > > +MODULE_AUTHOR("Eli Cohen
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin wrote:
>
> On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m@gmail.com wrote:
> > From: Tonghao Zhang
> >
> > Open vSwitch and Linux bridge will disable LRO of the interface
> > when this interface added to them. Now when disable the LRO, the
> > virtio-net csum is disable too. That drops the forwarding performance.
> >
> > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > Cc: Michael S. Tsirkin
> > Cc: Jason Wang
> > Cc: Willem de Bruijn
> > Signed-off-by: Tonghao Zhang
> > ---
> > v2:
> > * change the fix-tag
> > ---
> > drivers/net/virtio_net.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7145c83c6c8c..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > VIRTIO_NET_F_GUEST_CSUM
> > };
> >
> > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > +
>
> I think I'd rather we open-coded this, the macro is only
> used in one place ...
Yes, in this patch, it is used only in one place. But in next patch
[1], we use it twice and that make the code look a bit nicer.
Would we open-coded this in this patch ?
[1] -
http://patchwork.ozlabs.org/project/netdev/patch/20200928033915.82810-2-xiangxia.m@gmail.com/
> > struct virtnet_stat_desc {
> > char desc[ETH_GSTRING_LEN];
> > size_t offset;
> > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device
> > *dev,
> > if (features & NETIF_F_LRO)
> > offloads = vi->guest_offloads_capable;
> > else
> > - offloads = 0;
> > + offloads = vi->guest_offloads_capable &
> > +~GUEST_OFFLOAD_LRO_MASK;
> >
> > err = virtnet_set_guest_offloads(vi, offloads);
> > if (err)
>
> > --
> > 2.23.0
>
--
Best regards, Tonghao
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V1 vhost-next] vdpa/mlx5: Make vdpa core driver a distinct module
On Tue, Sep 29, 2020 at 09:57:44AM +0300, Eli Cohen wrote: > On Tue, Sep 29, 2020 at 02:51:12AM -0400, Michael S. Tsirkin wrote: > > On Tue, Sep 29, 2020 at 09:34:33AM +0300, Eli Cohen wrote: > > > On Tue, Sep 29, 2020 at 02:26:44AM -0400, Michael S. Tsirkin wrote: > > > > On Tue, Sep 29, 2020 at 09:20:26AM +0300, Eli Cohen wrote: > > > > > On Mon, Sep 28, 2020 at 03:55:09PM -0400, Michael S. Tsirkin wrote: > > > > > > On Thu, Sep 24, 2020 at 05:32:31PM +0300, Eli Cohen wrote: > > > > > > > Change core vdpa functionality into a loadbale module such that > > > > > > > upcoming > > > > > > > block implementation will be able to use it. > > > > > > > > > > > > > > Signed-off-by: Eli Cohen > > > > > > > > > > > > Why don't we merge this patch together with the block module? > > > > > > > > > > > > > > > > Since there are still not too many users of this driver, I would > > > > > prefer > > > > > to merge this as early as possible so pepole get used to the involved > > > > > modules. > > > > > > > > > > Anyways, I will send another version of the patch which makes use of > > > > > 'select' instead of 'depends'. > > > > > > > > > > Hope you agree to merge this. > > > > > > > > Are you quite sure there will be a block driver though? > > > > I'd like to avoid a situation in which we have infrastructure > > > > in place but no users. > > > > > > > > > > I know it's in our plans but I see your point. Let me know if you > > > prefer me to send the patch that fixes the 'depends' thing or defer it > > > to a later time. > > > > Not sure what's the depends thing. > > > > Use "select MLX5_CORE" > instead of "depends on MLX5_CORE" > > Wasn't this agreed upon? Hmm I don't know. I recall a similar discussion around VHOST_IOTLB. That's different ... I see [linux]$ git grep MLX5_CORE|grep depends drivers/infiniband/hw/mlx5/Kconfig: depends on NETDEVICES && ETHERNET && PCI && MLX5_CORE drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on NETDEVICES && ETHERNET && INET && PCI && MLX5_CORE drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN && RFS_ACCEL drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN && NET_SWITCHDEV drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN && DCB drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on TLS=y || MLX5_CORE=m drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on TLS=y || MLX5_CORE=m drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN drivers/net/ethernet/mellanox/mlx5/core/Kconfig:depends on MLX5_CORE_EN && MLX5_ESWITCH drivers/vdpa/Kconfig: depends on MLX5_CORE and no selects of this symbol, I guess you are saying you are changing everything else to select - is that right? Then I guess vdpa should follow suit ... > > > > > > > --- > > > > > > > V0 --> V1: > > > > > > > Removed "default n" for configu options as 'n' is the default > > > > > > > > > > > > > > drivers/vdpa/Kconfig | 8 +++- > > > > > > > drivers/vdpa/Makefile | 2 +- > > > > > > > drivers/vdpa/mlx5/Makefile | 7 +-- > > > > > > > drivers/vdpa/mlx5/core/core_main.c | 20 > > > > > > > drivers/vdpa/mlx5/core/mr.c| 3 +++ > > > > > > > drivers/vdpa/mlx5/core/resources.c | 10 ++ > > > > > > > 6 files changed, 42 insertions(+), 8 deletions(-) > > > > > > > create mode 100644 drivers/vdpa/mlx5/core/core_main.c > > > > > > > > > > > > > > diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig > > > > > > > index 4271c408103e..57ff6a7f7401 100644 > > > > > > > --- a/drivers/vdpa/Kconfig > > > > > > > +++ b/drivers/vdpa/Kconfig > > > > > > > @@ -29,10 +29,9 @@ config IFCVF > > > > > > > To compile this driver as a module, choose M here: the module > > > > > > > will > > > > > > > be called ifcvf. > > > > > > > > > > > > > > -config MLX5_VDPA > > > > > > > - bool "MLX5 VDPA support library for ConnectX devices" > > > > > > > +config MLX5_VDPA_CORE > > > > > > > + tristate "MLX5 VDPA support library for ConnectX devices" > > > > > > > depends on MLX5_CORE > > > > > > > - default n > > > > > > > help > > > > > > > Support library for Mellanox VDPA drivers. Provides code that > > > > > > > is > > > > > > >
Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
On Tue, Sep 29, 2020 at 2:22 PM Michael S. Tsirkin wrote:
>
> On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin wrote:
> > >
> > > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m@gmail.com
> > > > > wrote:
> > > > > > From: Tonghao Zhang
> > > > > >
> > > > > > Allow user configuring RXCSUM separately with ethtool -K,
> > > > > > reusing the existing virtnet_set_guest_offloads helper
> > > > > > that configures RXCSUM for XDP. This is conditional on
> > > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > >
> > > > > > Cc: Michael S. Tsirkin
> > > > > > Cc: Jason Wang
> > > > > > Signed-off-by: Tonghao Zhang
> > > > > > ---
> > > > > > drivers/net/virtio_net.c | 40
> > > > > >
> > > > > > 1 file changed, 28 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -68,6 +68,8 @@ static const unsigned long guest_offloads[] = {
> > > > > > (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > > > > (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > > >
> > > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
> > > > > > +
> > > > > > struct virtnet_stat_desc {
> > > > > > char desc[ETH_GSTRING_LEN];
> > > > > > size_t offset;
> > > > > > @@ -2526,25 +2528,37 @@ static int virtnet_set_features(struct
> > > > > > net_device *dev,
> > > > > > netdev_features_t features)
> > > > > > {
> > > > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > > > - u64 offloads;
> > > > > > + u64 offloads = vi->guest_offloads &
> > > > > > +vi->guest_offloads_capable;
> > > > > > int err;
> > > > > >
> > > > > > - if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > > - if (vi->xdp_queue_pairs)
> > > > > > - return -EBUSY;
> > > > > > + /* Don't allow configuration while XDP is active. */
> > > > > > + if (vi->xdp_queue_pairs)
> > > > > > + return -EBUSY;
> > > > > >
> > > > > > + if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > > if (features & NETIF_F_LRO)
> > > > > > - offloads = vi->guest_offloads_capable;
> > > > > > + offloads |= GUEST_OFFLOAD_LRO_MASK;
> > > > > > else
> > > > > > - offloads = vi->guest_offloads_capable &
> > > > > > -~GUEST_OFFLOAD_LRO_MASK;
> > > > > > + offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> > > > > > + }
> > > > > >
> > > > > > - err = virtnet_set_guest_offloads(vi, offloads);
> > > > > > - if (err)
> > > > > > - return err;
> > > > > > - vi->guest_offloads = offloads;
> > > > > > + if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> > > > > > + if (features & NETIF_F_RXCSUM)
> > > > > > + offloads |= GUEST_OFFLOAD_CSUM_MASK;
> > > > > > + else
> > > > > > + offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> > > > > > }
> > > > > >
> > > > > > + if (offloads == (vi->guest_offloads &
> > > > > > + vi->guest_offloads_capable))
> > > > > > + return 0;
> > > > >
> > > > > Hmm, what exactly does this do?
> > > > If the features(lro, rxcsum) we supported, are not changed, it is not
> > > > necessary to invoke virtnet_set_guest_offloads.
> > >
> > > okay, could you describe the cases where this triggers in a bit more
> > > detail pls?
> > Hi
> > As I known, when we run che commands show as below:
> > ethtool -K eth1 sg off
> > ethtool -K eth1 tso off
> >
> > In that case, we will not invoke virtnet_set_guest_offloads.
>
> How about initialization though? E.g. it looks like guest_offloads
> is 0-initialized, won't this skip the first command to disable
> offloads?
I guest you mean that: if guest_offloads == 0, and run the command
"ethtool -K eth1 sg off", that will disable offload ?
In that patch
u64 offloads = vi->guest_offloads & vi->guest_offloads_capable; // offload = 0
.
if (offloads == (vi->guest_offloads & vi->guest_offloads_capable)) //
if offload not changed, offload == 0, and (vi->guest_offloads &
vi->guest_offloads_capable) == 0.
return 0;
virtnet_set_guest_offloads // that will not be invoked, so will not
disable offload
> > > > > > +
> > > > > > + err = virtnet_set_guest_offloads(vi, offloads);
> > > > > > + if (err)
> > > > > > + return err;
> > > >
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 02:59:03PM +0800, Tonghao Zhang wrote:
> On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin wrote:
> >
> > On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m@gmail.com wrote:
> > > From: Tonghao Zhang
> > >
> > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > when this interface added to them. Now when disable the LRO, the
> > > virtio-net csum is disable too. That drops the forwarding performance.
> > >
> > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > Cc: Michael S. Tsirkin
> > > Cc: Jason Wang
> > > Cc: Willem de Bruijn
> > > Signed-off-by: Tonghao Zhang
> > > ---
> > > v2:
> > > * change the fix-tag
> > > ---
> > > drivers/net/virtio_net.c | 8 +++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 7145c83c6c8c..21b71148c532 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > > VIRTIO_NET_F_GUEST_CSUM
> > > };
> > >
> > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > +
> >
> > I think I'd rather we open-coded this, the macro is only
> > used in one place ...
> Yes, in this patch, it is used only in one place. But in next patch
> [1], we use it twice and that make the code look a bit nicer.
> Would we open-coded this in this patch ?
>
> [1] -
> http://patchwork.ozlabs.org/project/netdev/patch/20200928033915.82810-2-xiangxia.m@gmail.com/
OK then maybe keep this in a series like you did with v1.
> > > struct virtnet_stat_desc {
> > > char desc[ETH_GSTRING_LEN];
> > > size_t offset;
> > > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device
> > > *dev,
> > > if (features & NETIF_F_LRO)
> > > offloads = vi->guest_offloads_capable;
> > > else
> > > - offloads = 0;
> > > + offloads = vi->guest_offloads_capable &
> > > +~GUEST_OFFLOAD_LRO_MASK;
> > >
> > > err = virtnet_set_guest_offloads(vi, offloads);
> > > if (err)
> >
> > > --
> > > 2.23.0
> >
>
>
> --
> Best regards, Tonghao
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
On Tue, Sep 29, 2020 at 03:17:50PM +0800, Tonghao Zhang wrote:
> On Tue, Sep 29, 2020 at 2:22 PM Michael S. Tsirkin wrote:
> >
> > On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang wrote:
> > > On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > > > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin
> > > > > wrote:
> > > > > >
> > > > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m@gmail.com
> > > > > > wrote:
> > > > > > > From: Tonghao Zhang
> > > > > > >
> > > > > > > Allow user configuring RXCSUM separately with ethtool -K,
> > > > > > > reusing the existing virtnet_set_guest_offloads helper
> > > > > > > that configures RXCSUM for XDP. This is conditional on
> > > > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > > >
> > > > > > > Cc: Michael S. Tsirkin
> > > > > > > Cc: Jason Wang
> > > > > > > Signed-off-by: Tonghao Zhang
> > > > > > > ---
> > > > > > > drivers/net/virtio_net.c | 40
> > > > > > >
> > > > > > > 1 file changed, 28 insertions(+), 12 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > > > --- a/drivers/net/virtio_net.c
> > > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > > @@ -68,6 +68,8 @@ static const unsigned long guest_offloads[] = {
> > > > > > > (1ULL << VIRTIO_NET_F_GUEST_ECN) |
> > > > > > > \
> > > > > > > (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > > > >
> > > > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
> > > > > > > +
> > > > > > > struct virtnet_stat_desc {
> > > > > > > char desc[ETH_GSTRING_LEN];
> > > > > > > size_t offset;
> > > > > > > @@ -2526,25 +2528,37 @@ static int virtnet_set_features(struct
> > > > > > > net_device *dev,
> > > > > > > netdev_features_t features)
> > > > > > > {
> > > > > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > > > > - u64 offloads;
> > > > > > > + u64 offloads = vi->guest_offloads &
> > > > > > > +vi->guest_offloads_capable;
> > > > > > > int err;
> > > > > > >
> > > > > > > - if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > > > - if (vi->xdp_queue_pairs)
> > > > > > > - return -EBUSY;
> > > > > > > + /* Don't allow configuration while XDP is active. */
> > > > > > > + if (vi->xdp_queue_pairs)
> > > > > > > + return -EBUSY;
> > > > > > >
> > > > > > > + if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > > > if (features & NETIF_F_LRO)
> > > > > > > - offloads = vi->guest_offloads_capable;
> > > > > > > + offloads |= GUEST_OFFLOAD_LRO_MASK;
> > > > > > > else
> > > > > > > - offloads = vi->guest_offloads_capable &
> > > > > > > -~GUEST_OFFLOAD_LRO_MASK;
> > > > > > > + offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> > > > > > > + }
> > > > > > >
> > > > > > > - err = virtnet_set_guest_offloads(vi, offloads);
> > > > > > > - if (err)
> > > > > > > - return err;
> > > > > > > - vi->guest_offloads = offloads;
> > > > > > > + if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> > > > > > > + if (features & NETIF_F_RXCSUM)
> > > > > > > + offloads |= GUEST_OFFLOAD_CSUM_MASK;
> > > > > > > + else
> > > > > > > + offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> > > > > > > }
> > > > > > >
> > > > > > > + if (offloads == (vi->guest_offloads &
> > > > > > > + vi->guest_offloads_capable))
> > > > > > > + return 0;
> > > > > >
> > > > > > Hmm, what exactly does this do?
> > > > > If the features(lro, rxcsum) we supported, are not changed, it is not
> > > > > necessary to invoke virtnet_set_guest_offloads.
> > > >
> > > > okay, could you describe the cases where this triggers in a bit more
> > > > detail pls?
> > > Hi
> > > As I known, when we run che commands show as below:
> > > ethtool -K eth1 sg off
> > > ethtool -K eth1 tso off
> > >
> > > In that case, we will not invoke virtnet_set_guest_offloads.
> >
> > How about initialization though? E.g. it looks like guest_offloads
> > is 0-initialized, won't this skip the first command to disable
> > offloads?
> I guest you mean that: if guest_offloads == 0, and run the command
> "ethtool -K eth1 sg off", that will disable offload ?
> In that patch
> u64 offloads = vi->guest_offloads & vi->guest_offloads_capable; // offload = 0
> .
> if (offloads == (vi->guest_offloads & vi->guest_offloads_capable)) //
> if offload not changed, offload == 0, and (vi->guest_offloads
Re: [PATCH 1/2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 1:57 PM Michael S. Tsirkin wrote:
>
> On Tue, Sep 29, 2020 at 09:40:22AM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 3:21 AM Michael S. Tsirkin wrote:
> > >
> > > On Mon, Sep 28, 2020 at 11:39:14AM +0800, xiangxia.m@gmail.com wrote:
> > > > From: Tonghao Zhang
> > > >
> > > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > > when this interface added to them. Now when disable the LRO, the
> > > > virtio-net csum is disable too. That drops the forwarding performance.
> > > >
> > > > Fixes: e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> > >
> > > I am a bit confused by this tag. Did this change bring about
> > > disabling checksum when LRO is disabled? I am not sure
> > > I follow how ...
> > Hi Michael
> > It's not right fix tag.
> > The commit a02e8964eaf9 ("virtio-net: ethtool configurable LRO"),
> > disable the csum, when we disable the LRO
>
> OK then, pls send a correct Fixes tag when you repost this ...
Hi Michael
I repost this patch, please review. thanks.
http://patchwork.ozlabs.org/project/netdev/patch/20200929015806.19171-1-xiangxia.m@gmail.com/
> > > > Cc: Michael S. Tsirkin
> > > > Cc: Jason Wang
> > > > Cc: Willem de Bruijn
> > > > Signed-off-by: Tonghao Zhang
> > > > ---
> > > > drivers/net/virtio_net.c | 8 +++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 7145c83c6c8c..21b71148c532 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > > > VIRTIO_NET_F_GUEST_CSUM
> > > > };
> > > >
> > > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > > + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > > + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > +
> > > > struct virtnet_stat_desc {
> > > > char desc[ETH_GSTRING_LEN];
> > > > size_t offset;
> > > > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device
> > > > *dev,
> > > > if (features & NETIF_F_LRO)
> > > > offloads = vi->guest_offloads_capable;
> > > > else
> > > > - offloads = 0;
> > > > + offloads = vi->guest_offloads_capable &
> > > > +~GUEST_OFFLOAD_LRO_MASK;
> > > >
> > > > err = virtnet_set_guest_offloads(vi, offloads);
> > > > if (err)
> > > > --
> > > > 2.23.0
> > >
> >
> >
> > --
> > Best regards, Tonghao
>
--
Best regards, Tonghao
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V1 vhost-next] vdpa/mlx5: Make vdpa core driver a distinct module
On Tue, Sep 29, 2020 at 09:20:26AM +0300, Eli Cohen wrote:
> On Mon, Sep 28, 2020 at 03:55:09PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 24, 2020 at 05:32:31PM +0300, Eli Cohen wrote:
> > > Change core vdpa functionality into a loadbale module such that upcoming
> > > block implementation will be able to use it.
> > >
> > > Signed-off-by: Eli Cohen
> >
> > Why don't we merge this patch together with the block module?
> >
>
> Since there are still not too many users of this driver, I would prefer
> to merge this as early as possible so pepole get used to the involved
> modules.
>
> Anyways, I will send another version of the patch which makes use of
> 'select' instead of 'depends'.
>
> Hope you agree to merge this.
Are you quite sure there will be a block driver though?
I'd like to avoid a situation in which we have infrastructure
in place but no users.
> > > ---
> > > V0 --> V1:
> > > Removed "default n" for configu options as 'n' is the default
> > >
> > > drivers/vdpa/Kconfig | 8 +++-
> > > drivers/vdpa/Makefile | 2 +-
> > > drivers/vdpa/mlx5/Makefile | 7 +--
> > > drivers/vdpa/mlx5/core/core_main.c | 20
> > > drivers/vdpa/mlx5/core/mr.c| 3 +++
> > > drivers/vdpa/mlx5/core/resources.c | 10 ++
> > > 6 files changed, 42 insertions(+), 8 deletions(-)
> > > create mode 100644 drivers/vdpa/mlx5/core/core_main.c
> > >
> > > diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
> > > index 4271c408103e..57ff6a7f7401 100644
> > > --- a/drivers/vdpa/Kconfig
> > > +++ b/drivers/vdpa/Kconfig
> > > @@ -29,10 +29,9 @@ config IFCVF
> > > To compile this driver as a module, choose M here: the module will
> > > be called ifcvf.
> > >
> > > -config MLX5_VDPA
> > > - bool "MLX5 VDPA support library for ConnectX devices"
> > > +config MLX5_VDPA_CORE
> > > + tristate "MLX5 VDPA support library for ConnectX devices"
> > > depends on MLX5_CORE
> > > - default n
> > > help
> > > Support library for Mellanox VDPA drivers. Provides code that is
> > > common for all types of VDPA drivers. The following drivers are
> > > planned:
> > > @@ -40,8 +39,7 @@ config MLX5_VDPA
> > >
> > > config MLX5_VDPA_NET
> > > tristate "vDPA driver for ConnectX devices"
> > > - depends on MLX5_VDPA
> > > - default n
> > > + depends on MLX5_VDPA_CORE
> > > help
> > > VDPA network driver for ConnectX6 and newer. Provides offloading
> > > of virtio net datapath such that descriptors put on the ring will
> > > diff --git a/drivers/vdpa/Makefile b/drivers/vdpa/Makefile
> > > index d160e9b63a66..07353bbb9f8b 100644
> > > --- a/drivers/vdpa/Makefile
> > > +++ b/drivers/vdpa/Makefile
> > > @@ -2,4 +2,4 @@
> > > obj-$(CONFIG_VDPA) += vdpa.o
> > > obj-$(CONFIG_VDPA_SIM) += vdpa_sim/
> > > obj-$(CONFIG_IFCVF)+= ifcvf/
> > > -obj-$(CONFIG_MLX5_VDPA) += mlx5/
> > > +obj-$(CONFIG_MLX5_VDPA_CORE) += mlx5/
> > > diff --git a/drivers/vdpa/mlx5/Makefile b/drivers/vdpa/mlx5/Makefile
> > > index 89a5bededc9f..9f50f7e8d889 100644
> > > --- a/drivers/vdpa/mlx5/Makefile
> > > +++ b/drivers/vdpa/mlx5/Makefile
> > > @@ -1,4 +1,7 @@
> > > subdir-ccflags-y += -I$(srctree)/drivers/vdpa/mlx5/core
> > >
> > > -obj-$(CONFIG_MLX5_VDPA_NET) += mlx5_vdpa.o
> > > -mlx5_vdpa-$(CONFIG_MLX5_VDPA_NET) += net/main.o net/mlx5_vnet.o
> > > core/resources.o core/mr.o
> > > +obj-$(CONFIG_MLX5_VDPA_CORE) += mlx5_vdpa_core.o
> > > +mlx5_vdpa_core-$(CONFIG_MLX5_VDPA_CORE) += core/resources.o core/mr.o
> > > core/core_main.o
> > > +
> > > +obj-$(CONFIG_MLX5_VDPA_NET) += mlx5_vdpa_net.o
> > > +mlx5_vdpa_net-$(CONFIG_MLX5_VDPA_NET) += net/main.o net/mlx5_vnet.o
> > > diff --git a/drivers/vdpa/mlx5/core/core_main.c
> > > b/drivers/vdpa/mlx5/core/core_main.c
> > > new file mode 100644
> > > index ..4b39b55f57ab
> > > --- /dev/null
> > > +++ b/drivers/vdpa/mlx5/core/core_main.c
> > > @@ -0,0 +1,20 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> > > +/* Copyright (c) 2020 Mellanox Technologies Ltd. */
> > > +
> > > +#include
> > > +
> > > +MODULE_AUTHOR("Eli Cohen ");
> > > +MODULE_DESCRIPTION("Mellanox VDPA core driver");
> > > +MODULE_LICENSE("Dual BSD/GPL");
> > > +
> > > +static int __init mlx5_vdpa_core_init(void)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static void __exit mlx5_vdpa_core_exit(void)
> > > +{
> > > +}
> > > +
> > > +module_init(mlx5_vdpa_core_init);
> > > +module_exit(mlx5_vdpa_core_exit);
> > > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > > index ef1c550f8266..c093eab6c714 100644
> > > --- a/drivers/vdpa/mlx5/core/mr.c
> > > +++ b/drivers/vdpa/mlx5/core/mr.c
> > > @@ -434,6 +434,7 @@ int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev,
> > > struct vhost_iotlb *iotlb)
> > > mutex_unlock(>mkey_mtx);
> > > return err;
> > > }
> > > +EXPORT_SYMBOL(mlx5_vdpa_create_mr);
> > >
> > > void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> > > {
>
Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin wrote:
>
> On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin wrote:
> > >
> > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m@gmail.com wrote:
> > > > From: Tonghao Zhang
> > > >
> > > > Allow user configuring RXCSUM separately with ethtool -K,
> > > > reusing the existing virtnet_set_guest_offloads helper
> > > > that configures RXCSUM for XDP. This is conditional on
> > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > >
> > > > Cc: Michael S. Tsirkin
> > > > Cc: Jason Wang
> > > > Signed-off-by: Tonghao Zhang
> > > > ---
> > > > drivers/net/virtio_net.c | 40
> > > > 1 file changed, 28 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 21b71148c532..2e3af0b2c281 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -68,6 +68,8 @@ static const unsigned long guest_offloads[] = {
> > > > (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > > (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > >
> > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
> > > > +
> > > > struct virtnet_stat_desc {
> > > > char desc[ETH_GSTRING_LEN];
> > > > size_t offset;
> > > > @@ -2526,25 +2528,37 @@ static int virtnet_set_features(struct
> > > > net_device *dev,
> > > > netdev_features_t features)
> > > > {
> > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > - u64 offloads;
> > > > + u64 offloads = vi->guest_offloads &
> > > > +vi->guest_offloads_capable;
> > > > int err;
> > > >
> > > > - if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > - if (vi->xdp_queue_pairs)
> > > > - return -EBUSY;
> > > > + /* Don't allow configuration while XDP is active. */
> > > > + if (vi->xdp_queue_pairs)
> > > > + return -EBUSY;
> > > >
> > > > + if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > if (features & NETIF_F_LRO)
> > > > - offloads = vi->guest_offloads_capable;
> > > > + offloads |= GUEST_OFFLOAD_LRO_MASK;
> > > > else
> > > > - offloads = vi->guest_offloads_capable &
> > > > -~GUEST_OFFLOAD_LRO_MASK;
> > > > + offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> > > > + }
> > > >
> > > > - err = virtnet_set_guest_offloads(vi, offloads);
> > > > - if (err)
> > > > - return err;
> > > > - vi->guest_offloads = offloads;
> > > > + if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> > > > + if (features & NETIF_F_RXCSUM)
> > > > + offloads |= GUEST_OFFLOAD_CSUM_MASK;
> > > > + else
> > > > + offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> > > > }
> > > >
> > > > + if (offloads == (vi->guest_offloads &
> > > > + vi->guest_offloads_capable))
> > > > + return 0;
> > >
> > > Hmm, what exactly does this do?
> > If the features(lro, rxcsum) we supported, are not changed, it is not
> > necessary to invoke virtnet_set_guest_offloads.
>
> okay, could you describe the cases where this triggers in a bit more
> detail pls?
Hi
As I known, when we run che commands show as below:
ethtool -K eth1 sg off
ethtool -K eth1 tso off
In that case, we will not invoke virtnet_set_guest_offloads.
> > > > +
> > > > + err = virtnet_set_guest_offloads(vi, offloads);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + vi->guest_offloads = offloads;
> > > > return 0;
> > > > }
> > > >
> > > > @@ -3013,8 +3027,10 @@ static int virtnet_probe(struct virtio_device
> > > > *vdev)
> > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> > > > dev->features |= NETIF_F_LRO;
> > > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> > > > + dev->hw_features |= NETIF_F_RXCSUM;
> > > > dev->hw_features |= NETIF_F_LRO;
> > > > + }
> > > >
> > > > dev->vlan_features = dev->features;
> > > >
> > > > --
> > > > 2.23.0
> > >
> >
> >
> > --
> > Best regards, Tonghao
>
--
Best regards, Tonghao
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 -next] vdpa: mlx5: change Kconfig depends to fix build errors
On Tue, Sep 29, 2020 at 09:01:42AM +0300, Eli Cohen wrote: > On Fri, Sep 25, 2020 at 06:20:45AM -0400, Michael S. Tsirkin wrote: > > > > > > > > Hmm other drivers select VHOST_IOTLB, why not do the same? > > > > > > I can't see another driver doing that. > > > > Well grep VHOST_IOTLB and you will see some examples. > > $ git grep -wn VHOST_IOTLB > drivers/vhost/Kconfig:2:config VHOST_IOTLB > drivers/vhost/Kconfig:11: select VHOST_IOTLB > drivers/vhost/Kconfig:18: select VHOST_IOTLB > > What am I missing here? Nothing, there's a select here as expected. > > > Perhaps I can set dependency on > > > VHOST which by itself depends on VHOST_IOTLB? > > > > VHOST is processing virtio in the kernel. You don't really need that > > for mlx, do you? > > > > > > > > > > > > > > > > help > > > > > > Support library for Mellanox VDPA drivers. Provides code that > > > > > > is > > > > > > > > > > > > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-net: ethtool configurable RXCSUM
On Tue, Sep 29, 2020 at 02:10:56PM +0800, Tonghao Zhang wrote:
> On Tue, Sep 29, 2020 at 1:55 PM Michael S. Tsirkin wrote:
> >
> > On Tue, Sep 29, 2020 at 09:45:24AM +0800, Tonghao Zhang wrote:
> > > On Tue, Sep 29, 2020 at 3:25 AM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Mon, Sep 28, 2020 at 11:39:15AM +0800, xiangxia.m@gmail.com
> > > > wrote:
> > > > > From: Tonghao Zhang
> > > > >
> > > > > Allow user configuring RXCSUM separately with ethtool -K,
> > > > > reusing the existing virtnet_set_guest_offloads helper
> > > > > that configures RXCSUM for XDP. This is conditional on
> > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > >
> > > > > Cc: Michael S. Tsirkin
> > > > > Cc: Jason Wang
> > > > > Signed-off-by: Tonghao Zhang
> > > > > ---
> > > > > drivers/net/virtio_net.c | 40
> > > > >
> > > > > 1 file changed, 28 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 21b71148c532..2e3af0b2c281 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -68,6 +68,8 @@ static const unsigned long guest_offloads[] = {
> > > > > (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > > > (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > >
> > > > > +#define GUEST_OFFLOAD_CSUM_MASK (1ULL << VIRTIO_NET_F_GUEST_CSUM)
> > > > > +
> > > > > struct virtnet_stat_desc {
> > > > > char desc[ETH_GSTRING_LEN];
> > > > > size_t offset;
> > > > > @@ -2526,25 +2528,37 @@ static int virtnet_set_features(struct
> > > > > net_device *dev,
> > > > > netdev_features_t features)
> > > > > {
> > > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > > - u64 offloads;
> > > > > + u64 offloads = vi->guest_offloads &
> > > > > +vi->guest_offloads_capable;
> > > > > int err;
> > > > >
> > > > > - if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > - if (vi->xdp_queue_pairs)
> > > > > - return -EBUSY;
> > > > > + /* Don't allow configuration while XDP is active. */
> > > > > + if (vi->xdp_queue_pairs)
> > > > > + return -EBUSY;
> > > > >
> > > > > + if ((dev->features ^ features) & NETIF_F_LRO) {
> > > > > if (features & NETIF_F_LRO)
> > > > > - offloads = vi->guest_offloads_capable;
> > > > > + offloads |= GUEST_OFFLOAD_LRO_MASK;
> > > > > else
> > > > > - offloads = vi->guest_offloads_capable &
> > > > > -~GUEST_OFFLOAD_LRO_MASK;
> > > > > + offloads &= ~GUEST_OFFLOAD_LRO_MASK;
> > > > > + }
> > > > >
> > > > > - err = virtnet_set_guest_offloads(vi, offloads);
> > > > > - if (err)
> > > > > - return err;
> > > > > - vi->guest_offloads = offloads;
> > > > > + if ((dev->features ^ features) & NETIF_F_RXCSUM) {
> > > > > + if (features & NETIF_F_RXCSUM)
> > > > > + offloads |= GUEST_OFFLOAD_CSUM_MASK;
> > > > > + else
> > > > > + offloads &= ~GUEST_OFFLOAD_CSUM_MASK;
> > > > > }
> > > > >
> > > > > + if (offloads == (vi->guest_offloads &
> > > > > + vi->guest_offloads_capable))
> > > > > + return 0;
> > > >
> > > > Hmm, what exactly does this do?
> > > If the features(lro, rxcsum) we supported, are not changed, it is not
> > > necessary to invoke virtnet_set_guest_offloads.
> >
> > okay, could you describe the cases where this triggers in a bit more
> > detail pls?
> Hi
> As I known, when we run che commands show as below:
> ethtool -K eth1 sg off
> ethtool -K eth1 tso off
>
> In that case, we will not invoke virtnet_set_guest_offloads.
How about initialization though? E.g. it looks like guest_offloads
is 0-initialized, won't this skip the first command to disable
offloads?
> > > > > +
> > > > > + err = virtnet_set_guest_offloads(vi, offloads);
> > > > > + if (err)
> > > > > + return err;
> > > > > +
> > > > > + vi->guest_offloads = offloads;
> > > > > return 0;
> > > > > }
> > > > >
> > > > > @@ -3013,8 +3027,10 @@ static int virtnet_probe(struct virtio_device
> > > > > *vdev)
> > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> > > > > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> > > > > dev->features |= NETIF_F_LRO;
> > > > > - if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> > > > > {
> > > > > + dev->hw_features |= NETIF_F_RXCSUM;
> > > > > dev->hw_features |= NETIF_F_LRO;
> > > > > + }
> > > > >
> > > > > dev->vlan_features =
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m@gmail.com wrote:
> From: Tonghao Zhang
>
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
>
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: Michael S. Tsirkin
> Cc: Jason Wang
> Cc: Willem de Bruijn
> Signed-off-by: Tonghao Zhang
> ---
> v2:
> * change the fix-tag
> ---
> drivers/net/virtio_net.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7145c83c6c8c..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> VIRTIO_NET_F_GUEST_CSUM
> };
>
> +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
I think I'd rather we open-coded this, the macro is only
used in one place ...
> struct virtnet_stat_desc {
> char desc[ETH_GSTRING_LEN];
> size_t offset;
> @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> if (features & NETIF_F_LRO)
> offloads = vi->guest_offloads_capable;
> else
> - offloads = 0;
> + offloads = vi->guest_offloads_capable &
> +~GUEST_OFFLOAD_LRO_MASK;
>
> err = virtnet_set_guest_offloads(vi, offloads);
> if (err)
> --
> 2.23.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 9:23 AM Michael S. Tsirkin wrote:
>
> On Tue, Sep 29, 2020 at 02:59:03PM +0800, Tonghao Zhang wrote:
> > On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin wrote:
> > >
> > > On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m@gmail.com wrote:
> > > > From: Tonghao Zhang
> > > >
> > > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > > when this interface added to them. Now when disable the LRO, the
> > > > virtio-net csum is disable too. That drops the forwarding performance.
> > > >
> > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > Cc: Michael S. Tsirkin
> > > > Cc: Jason Wang
> > > > Cc: Willem de Bruijn
> > > > Signed-off-by: Tonghao Zhang
> > > > ---
> > > > v2:
> > > > * change the fix-tag
> > > > ---
> > > > drivers/net/virtio_net.c | 8 +++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 7145c83c6c8c..21b71148c532 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > > > VIRTIO_NET_F_GUEST_CSUM
> > > > };
> > > >
> > > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > > + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > > + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > +
> > >
> > > I think I'd rather we open-coded this, the macro is only
> > > used in one place ...
> > Yes, in this patch, it is used only in one place. But in next patch
> > [1], we use it twice and that make the code look a bit nicer.
> > Would we open-coded this in this patch ?
> >
> > [1] -
> > http://patchwork.ozlabs.org/project/netdev/patch/20200928033915.82810-2-xiangxia.m@gmail.com/
>
> OK then maybe keep this in a series like you did with v1.
If this is a fix it has to target net, unlike the other patch.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 4:00 AM wrote:
>
> From: Tonghao Zhang
>
> Open vSwitch and Linux bridge will disable LRO of the interface
> when this interface added to them. Now when disable the LRO, the
> virtio-net csum is disable too. That drops the forwarding performance.
I had focused on the code previously.
The s/w checksum verification cost is significant in a VM with traffic
to local destinations. A bridge does not verify transport layer
checksums OTOH?
> Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> Cc: Michael S. Tsirkin
> Cc: Jason Wang
> Cc: Willem de Bruijn
> Signed-off-by: Tonghao Zhang
> ---
> v2:
> * change the fix-tag
> ---
> drivers/net/virtio_net.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7145c83c6c8c..21b71148c532 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> VIRTIO_NET_F_GUEST_CSUM
> };
>
> +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> +
> struct virtnet_stat_desc {
> char desc[ETH_GSTRING_LEN];
> size_t offset;
> @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device *dev,
> if (features & NETIF_F_LRO)
> offloads = vi->guest_offloads_capable;
> else
> - offloads = 0;
> + offloads = vi->guest_offloads_capable &
> + ~GUEST_OFFLOAD_LRO_MASK;
>
> err = virtnet_set_guest_offloads(vi, offloads);
> if (err)
> --
> 2.23.0
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: Don't call vq_access_ok() when using IOTLB
On Mon, Sep 28, 2020 at 02:35:04PM +0200, Greg Kurz wrote:
> When the IOTLB device is enabled, the vring addresses we get from
> userspace are GIOVAs. It is thus wrong to pass them to vq_access_ok()
> which only takes HVAs. The IOTLB map is likely empty at this stage,
> so there isn't much that can be done with these GIOVAs. Access validation
> will be performed at IOTLB prefetch time anyway.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1883084
> Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
> Cc: jasow...@redhat.com
> CC: sta...@vger.kernel.org # 4.14+
> Signed-off-by: Greg Kurz
> ---
> drivers/vhost/vhost.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index b45519ca66a7..6296e33df31d 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1509,7 +1509,10 @@ static long vhost_vring_set_addr(struct vhost_dev *d,
>* If it is not, we don't as size might not have been setup.
>* We will verify when backend is configured. */
> if (vq->private_data) {
> - if (!vq_access_ok(vq, vq->num,
> + /* If an IOTLB device is present, the vring addresses are
> + * GIOVAs. Access will be validated during IOTLB prefetch. */
> + if (!vq->iotlb &&
> + !vq_access_ok(vq, vq->num,
> (void __user *)(unsigned long)a.desc_user_addr,
> (void __user *)(unsigned long)a.avail_user_addr,
> (void __user *)(unsigned long)a.used_user_addr))
OK I think you are right here.
Jason, can you ack pls?
However, I think a cleaner way to check this is by moving
the following check from vhost_vq_access_ok to vq_access_ok:
/* Access validation occurs at prefetch time with IOTLB */
if (vq->iotlb)
return true;
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 -next] vdpa: mlx5: change Kconfig depends to fix build errors
On Thu, Sep 24, 2020 at 08:47:05AM -0700, Randy Dunlap wrote: > On 9/24/20 3:24 AM, Eli Cohen wrote: > > On Thu, Sep 24, 2020 at 05:30:55AM -0400, Michael S. Tsirkin wrote: > --- linux-next-20200917.orig/drivers/vdpa/Kconfig > +++ linux-next-20200917/drivers/vdpa/Kconfig > @@ -31,7 +31,7 @@ config IFCVF > > config MLX5_VDPA > bool "MLX5 VDPA support library for ConnectX devices" > -depends on MLX5_CORE > +depends on VHOST_IOTLB && MLX5_CORE > default n > >>> > >>> While we are here, can anyone who apply this patch delete the "default n" > >>> line? > >>> It is by default "n". > > > > I can do that > > > >>> > >>> Thanks > >> > >> Hmm other drivers select VHOST_IOTLB, why not do the same? > > v1 used select, but Saeed requested use of depends instead because > select can cause problems. OK I went over the history. v1 selected VHOST, that as the issue I think. A later version switched to VHOST_IOTLB, that is ok to select. > > I can't see another driver doing that. Perhaps I can set dependency on > > VHOST which by itself depends on VHOST_IOTLB? > >> > >> > help > Support library for Mellanox VDPA drivers. Provides code that > is > > >> > > > -- > ~Randy ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[GIT PULL] virtio: last minute fixes
Unfortunately there are a couple more reported issues in vhost and vdpa, but those fixes are still being worked upon, no reason to delay those that are ready. The following changes since commit ba4f184e126b751d1bffad5897f263108befc780: Linux 5.9-rc6 (2020-09-20 16:33:55 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to a127c5bbb6a8eee851cbdec254424c480b8edd75: vhost-vdpa: fix backend feature ioctls (2020-09-24 05:54:36 -0400) virtio: last minute fixes A couple of last minute fixes Signed-off-by: Michael S. Tsirkin Eli Cohen (1): vhost: Fix documentation Jason Wang (1): vhost-vdpa: fix backend feature ioctls drivers/vhost/iotlb.c | 4 ++-- drivers/vhost/vdpa.c | 30 -- 2 files changed, 18 insertions(+), 16 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 3:32 PM Willem de Bruijn
wrote:
>
> On Tue, Sep 29, 2020 at 4:00 AM wrote:
> >
> > From: Tonghao Zhang
> >
> > Open vSwitch and Linux bridge will disable LRO of the interface
> > when this interface added to them. Now when disable the LRO, the
> > virtio-net csum is disable too. That drops the forwarding performance.
>
> I had focused on the code previously.
>
> The s/w checksum verification cost is significant in a VM with traffic
> to local destinations. A bridge does not verify transport layer
> checksums OTOH?
Hi Willem.
No, think about GRO(In the GRO we don't know packets will be forwarded
to other ports or to local). The call tree as below:
+ 5.41% secondary_startup_64
- 1.22% ret_from_fork
net_rx_action
napi_poll
virtnet_poll
virtnet_receive
napi_gro_receive
dev_gro_receive
inet_gro_receive
tcp4_gro_receive
__skb_gro_checksum_complete
skb_checksum
__skb_checksum
csum_partial
do_csum
- 1.13% do_csum
$ brctl show
bridge name bridge id STP enabled interfaces
br0 8000.001122330001 no eth1
eth2
> > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > Cc: Michael S. Tsirkin
> > Cc: Jason Wang
> > Cc: Willem de Bruijn
> > Signed-off-by: Tonghao Zhang
> > ---
> > v2:
> > * change the fix-tag
> > ---
> > drivers/net/virtio_net.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 7145c83c6c8c..21b71148c532 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > VIRTIO_NET_F_GUEST_CSUM
> > };
> >
> > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > +
> > struct virtnet_stat_desc {
> > char desc[ETH_GSTRING_LEN];
> > size_t offset;
> > @@ -2531,7 +2536,8 @@ static int virtnet_set_features(struct net_device
> > *dev,
> > if (features & NETIF_F_LRO)
> > offloads = vi->guest_offloads_capable;
> > else
> > - offloads = 0;
> > + offloads = vi->guest_offloads_capable &
> > + ~GUEST_OFFLOAD_LRO_MASK;
> >
> > err = virtnet_set_guest_offloads(vi, offloads);
> > if (err)
> > --
> > 2.23.0
> >
--
Best regards, Tonghao
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v2] virtio-net: don't disable guest csum when disable LRO
On Tue, Sep 29, 2020 at 3:29 PM Willem de Bruijn
wrote:
>
> On Tue, Sep 29, 2020 at 9:23 AM Michael S. Tsirkin wrote:
> >
> > On Tue, Sep 29, 2020 at 02:59:03PM +0800, Tonghao Zhang wrote:
> > > On Tue, Sep 29, 2020 at 2:23 PM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Tue, Sep 29, 2020 at 09:58:06AM +0800, xiangxia.m@gmail.com
> > > > wrote:
> > > > > From: Tonghao Zhang
> > > > >
> > > > > Open vSwitch and Linux bridge will disable LRO of the interface
> > > > > when this interface added to them. Now when disable the LRO, the
> > > > > virtio-net csum is disable too. That drops the forwarding performance.
> > > > >
> > > > > Fixes: a02e8964eaf9 ("virtio-net: ethtool configurable LRO")
> > > > > Cc: Michael S. Tsirkin
> > > > > Cc: Jason Wang
> > > > > Cc: Willem de Bruijn
> > > > > Signed-off-by: Tonghao Zhang
> > > > > ---
> > > > > v2:
> > > > > * change the fix-tag
> > > > > ---
> > > > > drivers/net/virtio_net.c | 8 +++-
> > > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 7145c83c6c8c..21b71148c532 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -63,6 +63,11 @@ static const unsigned long guest_offloads[] = {
> > > > > VIRTIO_NET_F_GUEST_CSUM
> > > > > };
> > > > >
> > > > > +#define GUEST_OFFLOAD_LRO_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> > > > > + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> > > > > + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> > > > > + (1ULL << VIRTIO_NET_F_GUEST_UFO))
> > > > > +
> > > >
> > > > I think I'd rather we open-coded this, the macro is only
> > > > used in one place ...
> > > Yes, in this patch, it is used only in one place. But in next patch
> > > [1], we use it twice and that make the code look a bit nicer.
> > > Would we open-coded this in this patch ?
> > >
> > > [1] -
> > > http://patchwork.ozlabs.org/project/netdev/patch/20200928033915.82810-2-xiangxia.m@gmail.com/
> >
> > OK then maybe keep this in a series like you did with v1.
>
> If this is a fix it has to target net, unlike the other patch.
Hi Willem, Michael
The first patch v2 is for -net, can we apply it?
and second patch will be sent for -net-next after discussion ? That is ok?
--
Best regards, Tonghao
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
