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