[Spice-devel] [win32-qxl]Do not allow duplicate IDs in video mode info buffer.

[Sandy Stutsman]

Hey.

On 9/17/2015 11:06 AM, Uri Lublin wrote:
> On 09/17/2015 02:40 PM, Christophe Fergeau wrote:
>> On Wed, Sep 16, 2015 at 03:27:49PM -0400, Sandy Stutsman wrote:
>>> Modes are returned in a buffer from qemu with IDs in increasing but not
>>> necessarily consecutive order. (Some modes may be omitted if they don't fit
>>> into the framebuffer.) Two custom modes are appended to the local buffer to
>>> support arbitrary resolutions. The first custom id must be set to +1 of the
>>> last id in the mode buffer, not to the last index +1 of the buffer.
>>>
>>> E.G. these are the last entries in the mode buffer returned from QEMU:
>>>      Index      Mode id (unfortunately named ModeIndex in the struct)
>>>      ...
>>>      124         128
>>>      125         130
>>>      126         132
>>>      127         134 (last mode returned from QEMU)
>>>
>>> Custom Modes:
>>>      128         135 (Not 128!)
>>>      129         136
>>>
>>> If the first custom mode id is set to 128 (the number of modes returned
>>> from QEMU), it will duplicate the id for the mode at index 124. That could
>>> cause the FindMode function which searches the mode list by mode id
>>> to return the wrong mode information.
>>>
>>> The custom mode id's are now set to be greater than the last mode id
>>> returned from QEMU.
>>> ---
>>>   xddm/miniport/qxl.c | 15 +++++++++++----
>>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xddm/miniport/qxl.c b/xddm/miniport/qxl.c
>>> index e0fb87f..22537e2 100644
>>> --- a/xddm/miniport/qxl.c
>>> +++ b/xddm/miniport/qxl.c
>>> @@ -596,6 +596,7 @@ VP_STATUS InitModes(QXLExtension *dev)
>>>       ULONG n_modes;
>>>       ULONG i;
>>>       VP_STATUS error;
>>> +    ULONG custom_mode_id = 0;
>>>
>>>       PAGED_CODE();
>>>       DEBUG_PRINT((dev, 0, "%s\n", __FUNCTION__));
>>> @@ -632,6 +633,11 @@ VP_STATUS InitModes(QXLExtension *dev)
>>>               return error;
>>>           }
>>>       }
>>> +    /*  Mode ids are increasing in the buffer but may not be consecutive,
>>> +     *  so set the first custom id to be 1 larger than the last id in the buffer.
>>> +     *  (ModeIndex isn't really an index, it is an ID).
>>> +     */
>>> +    custom_mode_id = modes->modes[modes->n_modes-1].id + 1;
>>>
>>>       /* 2 dummy modes for custom display resolution */
>>>       /* This is necessary to bypass Windows mode index check, that
>>> @@ -640,7 +646,7 @@ VP_STATUS InitModes(QXLExtension *dev)
>>>
>>>       for (i = dev->custom_mode; i <= dev->custom_mode + 1; ++i) {
>>>           memcpy(&modes_info[i], &modes_info[0], sizeof(VIDEO_MODE_INFORMATION));
>>> -        modes_info[i].ModeIndex = i;
>>> +        modes_info[i].ModeIndex = custom_mode_id++;
>>>       }
>>>
>>>       dev->n_modes = n_modes;
>>> @@ -1002,6 +1008,7 @@ static VP_STATUS SetCustomDisplay(QXLExtension *dev_ext, QXLEscapeSetCustomDispl
>>>       uint32_t xres = custom_display->xres;
>>>       uint32_t yres = custom_display->yres;
>>>       uint32_t bpp = custom_display->bpp;
>>> +    PVIDEO_MODE_INFORMATION pMode;
>>>
>>>       /* alternate custom mode index */
>>>       if (dev_ext->custom_mode == (dev_ext->n_modes - 1))
>>> @@ -1014,11 +1021,11 @@ static VP_STATUS SetCustomDisplay(QXLExtension *dev_ext, QXLEscapeSetCustomDispl
>>>                       __FUNCTION__, xres, yres, bpp, dev_ext->rom->surface0_area_size));
>>>           return ERROR_NOT_ENOUGH_MEMORY;
>>>       }
>>> -
>>> -    ret = FillVidModeInfo(&dev_ext->modes[dev_ext->custom_mode],
>>> +    pMode = &dev_ext->modes[dev_ext->custom_mode];
>>> +    ret = FillVidModeInfo(pMode,
>>>                             custom_display->xres, custom_display->yres,
>>>                             custom_display->bpp,
>>> -                          dev_ext->custom_mode);
>>> +                          pMode->ModeIndex);
>> This looks good to me, ACK.
>> I'd add a commit on top of that getting rid of
>> the last argument to FillVidModeInfo as all that function does with this
>> argument is set pMode->ModeIndex to be equal to its value.
> This patch is good, but I wonder if the problem is not with qemu.
> If we change in qemu/hw/display/qxl.c:
> - modes->modes[n].id          = cpu_to_le32(i);
> + modes->modes[n].id          = cpu_to_le32(n);
>
> Does that solves the problem ?
>
> Thanks,
>      Uri.
>
I bet it does and I can test it for the windows driver but I'd need help to verify it for linux :)