Re: [Qemu-devel] [RfC PATCH 06/15] virtio-gpu/2d: add virtio gpu core code

[Gerd Hoffmann <kraxel@redhat.com>]

On Do, 2015-02-26 at 11:08 -0500, Max Reitz wrote:
> On 2015-02-23 at 05:23, Gerd Hoffmann wrote:
> > This patch adds the core code for virtio gpu emulation,
> > covering 2d support.
> >
> > Written by Dave Airlie and Gerd Hoffmann.
> >
> > Signed-off-by: Dave Airlie<airlied@redhat.com>
> > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> > ---
> >   hw/display/Makefile.objs       |   2 +
> >   hw/display/virtio-gpu.c        | 903 +++++++++++++++++++++++++++++++++++++++++
> >   include/hw/virtio/virtio-gpu.h | 147 +++++++
> >   trace-events                   |  14 +
> >   4 files changed, 1066 insertions(+)
> >   create mode 100644 hw/display/virtio-gpu.c
> >   create mode 100644 include/hw/virtio/virtio-gpu.h
> 
> Again I mostly only have formal complaints...
> 
> But one non-formal question: As far as I understood virtio-gpu's mode of 
> operation from this patch, it looks like there is one resource per 
> scanout, and that resource is basically the whole screen (which can be 
> updated partially).

This is correct (for 2d mode, 3d will be different).

> If that is the case, what do we gain from being able to display a 
> resource on multiple scanouts? If we don't associate a scanout to a 
> resource with set_scanout, the resource won't ever be displayed on that 
> scanout; and if we associate it, the scanout's position and dimension 
> will be exactly the same as the resource's, so associating a resource 
> with multiple scanouts means that all those scanouts will be duplicates 
> of each other, which in turn means we can duplicate heads. But that 
> seems more like a frontend feature to me...

It's handled this way to mimic behavior of physical hardware, where you
can configure your scanouts (monitor plugs of gfx hardware) in a simliar
way.

Main advantage of taking this route is that virtual hardware and
physical hardware can be configued the same way, i.e. you can -- for
example -- setup screen mirroring with xrandr.

> > +static void update_cursor_data_simple(VirtIOGPU *g,
> > +                                      struct virtio_gpu_scanout *s,
> > +                                      uint32_t resource_id)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    uint32_t pixels;
> > +
> > +    res = virtio_gpu_find_resource(g, resource_id);
> > +    if (!res) {
> > +        return;
> > +    }
> > +
> > +    pixels = s->current_cursor->width * s->current_cursor->height;
> 
> This multiplication might overflow (but I can't imagine that one could 
> use this for an exploit).

Added checks.

> > +static struct virtio_gpu_simple_resource *
> > +virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +
> > +    QTAILQ_FOREACH(res, &g->reslist, next) {
> > +        if (res->resource_id == resource_id) {
> > +            return res;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> 
> How often do you think this function is called, and how many resources 
> do you expect there to be? Would it make sense to make this a hash table?

Not for 2d, it's only scanouts and cursors.

In 3d mode virglrenderer will manage resources, and there will be alot
more (textures, buffers, ....).

> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +
> > +    res->width = c2d.width;
> > +    res->height = c2d.height;
> > +    res->format = c2d.format;
> > +    res->resource_id = c2d.resource_id;
> 
> Considering resource_id == 0 is sometimes (apparently) used as "no 
> resource" (e.g. in transfer_to_host_2d), would it make sense to test 
> against that here?

Added check.

> > +    format = pixman_image_get_format(res->image);
> > +    bpp = (PIXMAN_FORMAT_BPP(format) + 7) / 8;
> 
> Could have used DIV_ROUND_UP(); also, I don't find it anywhere in the 
> virtio-gpu spec (at least not in the one spec I found), so will this 
> really work if PIXMAN_FORMAT_BPP(format) is not a multiple of 8? I can 
> imagine this being correct for *_BPP() == 15, but maybe not so much for 
> *_BPP() == 1; but then again, maybe you don't plan on supporting 
> sub-byte pixel formats anyway.

Yes, it's for bpp=15.

> > +    if (t2d.offset || t2d.r.x || t2d.r.y ||
> > +        t2d.r.width != pixman_image_get_width(res->image)) {
> > +        void *img_data = pixman_image_get_data(res->image);
> > +        for (h = 0; h < t2d.r.height; h++) {
> > +            src_offset = t2d.offset + stride * h;
> > +            dst_offset = (t2d.r.y + h) * stride + (t2d.r.x * bpp);
> > +
> > +            iov_to_buf(res->iov, res->iov_cnt, src_offset,
> > +                       (uint8_t *)img_data
> > +                       + dst_offset, t2d.r.width * bpp);
> > +        }
> > +    } else {
> > +        iov_to_buf(res->iov, res->iov_cnt, 0,
> > +                   pixman_image_get_data(res->image),
> > +                   pixman_image_get_stride(res->image)
> > +                   * pixman_image_get_height(res->image));
> 
> Will this work with stride != t2d.r.width * bpp?

Those cases should take the if branch above and loop over all lines to
handle it correctly.

> > +    res = virtio_gpu_find_resource(g, ss.resource_id);
> 
> If ss.resource_id == 0, the result is unused; this function call could 
> therefore be put after the next if block.

Done.

> > +    g->enable = 1;
> > +    if (ss.resource_id == 0) {
> > +        scanout = &g->scanout[ss.scanout_id];
> > +        if (g->scanout[ss.scanout_id].resource_id) {
> 
> Would be shorter as "if (scanout->resource_id)".

Yep, fixed.

> > +int virtio_gpu_create_mapping_iov(struct virtio_gpu_resource_attach_backing *ab,
> > +                                  struct virtio_gpu_ctrl_command *cmd,
> > +                                  struct iovec **iov)
> > +{
> > +    struct virtio_gpu_mem_entry *ents;
> > +    size_t esize, s;
> > +    int i;
> > +
> > +    esize = sizeof(*ents) * ab->nr_entries;
> 
> Can overflow (on 32 bit hosts).

[ ... ]

> However, I think it's best to simply limit ab->nr_entries to some sane 
> value

Done.

thanks,
  Gerd