[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/7] sm501: Replace hand written implementation with pixma
From: |
Gerd Hoffmann |
Subject: |
Re: [PATCH v2 5/7] sm501: Replace hand written implementation with pixman where possible |
Date: |
Tue, 26 May 2020 12:43:18 +0200 |
On Thu, May 21, 2020 at 09:39:44PM +0200, BALATON Zoltan wrote:
> Besides being faster this should also prevent malicious guests to
> abuse 2D engine to overwrite data or cause a crash.
> uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> - uint8_t *src = s->local_mem + src_base;
> - val = *(_pixel_type *)&src[index_s];
> \
Well, the advantage of *not* using pixman is that you can easily switch
the code to use offsets instead of pointers, then apply the mask to the
*final* offset to avoid oob data access:
val = *(_pixel_type*)(&s->local_mem[(s->twoD_source_base + index_s) &
0x03FFFFFF]);
> + if ((rop_mode && rop == 0x5) || (!rop_mode && rop == 0x55)) {
> + /* Invert dest, is there a way to do this with pixman? */
PIXMAN_OP_XOR maybe?
> + if (rtl && ((db >= sb && db <= se) || (de >= sb && de <= se))) {
> + /* regions may overlap: copy via temporary */
The usual way for a hardware blitter is to have a direction bit, i.e.
the guest os can ask to blit in top->bottom or bottom->top scanline
ordering. The guest can use that to make sure the blit does not
overwrite things. But note the guest can also intentionally use
overlapping regions, i.e. memset(0) the first scanline, then use a blit
with overlap to clear the whole screen. The later will surely break if
you blit via temporary image ...
> + pixman_blt((uint32_t *)&s->local_mem[src_base],
> + (uint32_t *)&s->local_mem[dst_base],
> + src_pitch * (1 << format) / sizeof(uint32_t),
> + dst_pitch * (1 << format) / sizeof(uint32_t),
> + 8 * (1 << format), 8 * (1 << format),
> + src_x, src_y, dst_x, dst_y, width, height);
See above, i'm not convinced pixman is the best way here.
When using pixman I'd suggest:
(1) src = pixman_image_create_bits_no_clear(...);
(2) dst = pixman_image_create_bits_no_clear(...);
(3) pixman_image_composite(PIXMAN_OP_SRC, src, NULL, dst, ...);
(4) pixman_image_unref(src);
(5) pixman_image_unref(dst);
pixman_blt() is probably doing basically the same. The advantage of not
using pixman_blt() is that
(a) you can also use pixman ops other than PIXMAN_OP_SRC, and
(b) you can have a helper function for (1)+(2) which very carefully
applies sanity checks to make sure the pixman image created stays
completely inside s->local_mem.
(c) you have the option to completely rearrange the code flow, for
example update the src pixman image whenever the guest touches
src_base or src_pitch or format instead of having a
create/op/unref cycle on every blitter op.
> + pixman_fill((uint32_t *)&s->local_mem[dst_base],
> + dst_pitch * (1 << format) / sizeof(uint32_t),
> + 8 * (1 << format), dst_x, dst_y, width, height, color);
(1) src = pixman_image_create_solid(...), otherwise same as above ;)
take care,
Gerd
[PATCH v2 4/7] sm501: Clean up local variables in sm501_2d_operation, BALATON Zoltan, 2020/05/21
[PATCH v2 6/7] sm501: Optimize small overlapping blits, BALATON Zoltan, 2020/05/21
[PATCH v2 1/7] sm501: Convert printf + abort to qemu_log_mask, BALATON Zoltan, 2020/05/21
Re: [PATCH v2 0/7] Misc display/sm501 clean ups and fixes, no-reply, 2020/05/21
Re: [PATCH v2 0/7] Misc display/sm501 clean ups and fixes, Gerd Hoffmann, 2020/05/28