| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <84096c0d-975a-49c5-bf2d-42928c0854a8@wanadoo.fr>
Date: Sat, 21 Sep 2024 08:40:46 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Ryosuke Yasuoka <ryasuoka@...hat.com>, airlied@...hat.com,
kraxel@...hat.com, maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, daniel@...ll.ch, jfalempe@...hat.com
Cc: virtualization@...ts.linux.dev, spice-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v3] drm/qxl: Add drm_panic support
Le 21/09/2024 à 06:33, Ryosuke Yasuoka a écrit :
> QXL supports the drm_panic module, which displays a message to the
> screen when a kernel panic occurs.
>
> Signed-off-by: Ryosuke Yasuoka <ryasuoka@...hat.com>
> ---
Hi,
a few comments/nitpicks below, if of interest.
> diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
> index d6ea01f3797b..a3dd2ed4d9c7 100644
> --- a/drivers/gpu/drm/qxl/qxl_cmd.c
> +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
> @@ -174,6 +174,35 @@ static bool qxl_ring_pop(struct qxl_ring *ring,
> return true;
> }
>
> +/* For drm panic */
> +int
> +qxl_push_command_ring_without_release(struct qxl_device *qdev,
> + struct qxl_bo *bo, uint32_t offset)
> +{
> + struct qxl_command cmd;
> + struct qxl_ring *ring = qdev->command_ring;
> + struct qxl_ring_header *header = &(ring->ring->header);
Nitpick: I think that these () can be removed.
> + uint8_t *elt;
> + int idx;
> +
> + cmd.type = QXL_CMD_DRAW;
> + cmd.data = qxl_bo_physical_address(qdev, bo, offset);
> +
> + idx = header->prod & (ring->n_elements - 1);
> + elt = ring->ring->elements + idx * ring->element_size;
> +
> + memcpy((void *)elt, &cmd, ring->element_size);
> +
> + header->prod++;
> +
> + mb();
> +
> + if (header->prod == header->notify_on_prod)
> + outb(0, ring->prod_notify);
> +
> + return 0;
> +}
...
> +int qxl_push_command_ring_without_release(struct qxl_device *qdev,
> + struct qxl_bo *bo, uint32_t offset);
> +
> +
Nitpick: no need for 2 empty lines.
> int
> qxl_push_command_ring_release(struct qxl_device *qdev, struct qxl_release *release,
> uint32_t type, bool interruptible);
...
> +int
> +qxl_image_alloc_objects_without_release(struct qxl_device *qdev,
> + struct qxl_drm_image *image,
> + struct qxl_drm_chunk *chunk,
> + struct qxl_bo *image_bo, struct qxl_bo *chunk_bo,
> + uint8_t *surface_base, int width, int height,
> + int depth, int stride)
> +{
> + int ret;
> + unsigned int chunk_size = sizeof(struct qxl_data_chunk) + stride * height;
> +
> + INIT_LIST_HEAD(&image->chunk_list);
> + qxl_panic_bo_create(qdev, sizeof(struct qxl_image), image_bo);
> + image->bo = image_bo;
> +
> + qxl_panic_bo_create(qdev, chunk_size, chunk_bo);
> + chunk->bo = chunk_bo;
> + list_add_tail(&chunk->head, &image->chunk_list);
> +
> + ret = qxl_image_init(qdev, NULL, image, surface_base,
> + 0, 0, width, height, depth, stride);
> + return ret;
> +
Nitpick: unneeeded empty line.
> +}
> +
> int
> qxl_image_alloc_objects(struct qxl_device *qdev,
> struct qxl_release *release,
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
> index 66635c55cf85..cdba20718674 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -29,6 +29,16 @@
> #include "qxl_drv.h"
> #include "qxl_object.h"
>
> +/* For drm panic */
> +static void qxl_panic_ttm_bo_destroy(struct ttm_buffer_object *tbo)
> +{
> + struct qxl_bo *bo;
> +
> + bo = to_qxl_bo(tbo);
> + WARN_ON_ONCE(bo->map_count > 0);
> + drm_gem_object_release(&bo->tbo.base);
> +}
> +
> static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
> {
> struct qxl_bo *bo;
> @@ -101,6 +111,42 @@ static const struct drm_gem_object_funcs qxl_object_funcs = {
> .print_info = drm_gem_ttm_print_info,
> };
>
> +/* For drm panic */
> +int qxl_panic_bo_create(struct qxl_device *qdev, unsigned long size, struct qxl_bo *bo)
This function is used 3 times in this patch. Its returned value is never
checked. Should some error handling be removed, or maybe change this
function to return void?
> +{
> + u32 domain = QXL_GEM_DOMAIN_VRAM;
> + struct ttm_operation_ctx ctx = { true, false };
> + enum ttm_bo_type type;
> + int r;
> +
> + type = ttm_bo_type_device;
> +
> + size = roundup(size, PAGE_SIZE);
> + r = drm_gem_object_init(&qdev->ddev, &bo->tbo.base, size);
> + if (unlikely(r))
> + return r;
> + bo->tbo.base.funcs = &qxl_object_funcs;
> + bo->type = domain;
> + bo->surface_id = 0;
> + INIT_LIST_HEAD(&bo->list);
> +
> + qxl_ttm_placement_from_domain(bo, domain);
> +
> + bo->tbo.priority = 0;
> + r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, type,
> + &bo->placement, 0, &ctx, NULL, NULL,
> + &qxl_panic_ttm_bo_destroy);
> + if (unlikely(r != 0)) {
unlikely() is used in several places. Does it really bring any benefit?
> + if (r != -ERESTARTSYS)
> + dev_err(qdev->ddev.dev,
> + "object_init failed for (%lu, 0x%08X)\n",
> + size, domain);
> + return r;
> + }
> + ttm_bo_unreserve(&bo->tbo);
> + return 0;
> +}
...
CJ
Powered by blists - more mailing lists