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

[Uri Lublin]

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.