linux-kernel - Re: [PATCH v4] drm/virtio: Add drm

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4ba9da71-535d-4579-907f-8ff2ccc505e5@collabora.com>
Date: Mon, 25 Nov 2024 05:27:00 +0300
From: Dmitry Osipenko <dmitry.osipenko@...labora.com>
To: Jocelyn Falempe <jfalempe@...hat.com>,
 Ryosuke Yasuoka <ryasuoka@...hat.com>, airlied@...hat.com,
 kraxel@...hat.com, gurchetansingh@...omium.org, olvaffe@...il.com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 simona@...ll.ch
Cc: dri-devel@...ts.freedesktop.org, virtualization@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] drm/virtio: Add drm_panic support

On 11/19/24 13:21, Jocelyn Falempe wrote:
...
>> In the virtio_panic_flush() below,
>> virtio_gpu_panic_cmd_transfer_to_host_2d() is invoked only for dumb BOs.
>> Thus, only dumb BO supports panic output and should be accepted by
>> get_scanout_buffer(), other should be rejected with ENODEV here, AFAICT.
>> Correct?
> 
> Yes, it's correct

Please fix in v5.

...
>>> +struct virtio_gpu_panic_object_array {
>>> +    struct ww_acquire_ctx ticket;
>>> +    struct list_head next;
>>> +    u32 nents, total;
>>> +    struct drm_gem_object *objs;
>>> +};
>>
>> This virtio_gpu_panic_object_array struct is a hack, use
>> virtio_gpu_array_alloc(). Maybe add atomic variant of the array_alloc().
> 
> We can't allocate memory in the panic handler, but maybe it can be pre-
> allocated, like the virtio_panic_buffer ?

We certainly can allocate memory from any context using GFP_ATOMIC flag
for the allocation. You already doing that in
virtio_gpu_panic_queue_ctrl_sgs().

>>> +static void virtio_gpu_panic_put_vbuf(struct virtqueue *vq,
>>> +                      struct virtio_gpu_vbuffer *vbuf,
>>> +                      struct virtio_gpu_object_array *objs)
>>> +{
>>> +    unsigned int len;
>>> +    int i;
>>> +
>>> +    /* waiting vbuf to be used */
>>> +    for (i = 0; i < 500; i++) {
>>> +        if (vbuf == virtqueue_get_buf(vq, &len)) {
>>
>> Is it guaranteed that virtio_gpu_dequeue_ctrl_func() never runs in
>> parallel here?
> 
> Yes, in the panic handler, only one CPU remains active, and no other
> task can be scheduled.

Awesome. thanks for the clarification.

...
>>> +    ret = virtio_gpu_panic_resource_flush(plane, vbuf_resource_flush,
>>> +                          plane->state->src_x >> 16,
>>> +                          plane->state->src_y >> 16,
>>> +                          plane->state->src_w >> 16,
>>> +                          plane->state->src_h >> 16);
>>> +    if (ret) {
>>> +        virtio_gpu_panic_put_vbuf(vgdev->ctrlq.vq,
>>> +                      vbuf_dumb_bo,
>>> +                      (struct virtio_gpu_object_array *)&objs);
>>
>> The virtio_gpu_panic_notify() hasn't been invoked here, thus this
>> put_vbuf should always time out because vq hasn't been kicked. Again,
>> best to leak resources on error than to have broken/untested error
>> handling code paths.
> 
> agreed

Please change it in v5.

...
>> If there is more than one virtio-gpu display, then this panic buffer is
>> reused by other displays. It seems to work okay, but potential
>> implications are unclear. Won't it be more robust to have a panic buffer
>> per CRTC?
> 
> The drm panic handler call each plane sequentially, so it's not a
> problem. Having one buffer per CRTC would just add more complexity.

Please add a clarifying comment for that to the code.

-- 
Best regards,
Dmitry