Re: [PATCH v10] Implementing WDDM interface to support multiple monitors and arbitrary resolution

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 18, 2016 at 6:48 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
> The Direct3D 9 API operates on either the Windows XP display driver
> model (XPDM) or the Windows Vista display driver model (WDDM), depending
> on the operating system installed.
>
> This patch implements the WDDM interface while using the CCD API to do
> so. Moreover it introduces multiple monitors support and arbitrary
> resolution for Windows 10 while preserving backward compatiblity with
> previous versions of Windows.
>
> Based on a patch by Sandy Stutsman <sstutsma at redhat.com>
>
> Signed-off-by: Dmitry Fleytman <dfleytma at redhat.com>
> Signed-off-by: Sameeh Jubran <sameeh@xxxxxxxxxx>
> ---
>  vdagent/display_configuration.cpp | 346
>  +++++++++++++++++++++++++++++++++++++-
>  vdagent/display_configuration.h   |  47 ++++++
>  2 files changed, 392 insertions(+), 1 deletion(-)
>

Missing changes however looking at the code I understood that
now the ABI is as was before and you are using some malloc/free.
So the protocol change is not needed.
I prefer the version before you decided to introduce the protocol
change.

Can you please provide me the patch number that you preferred?
Beside the malloc/free, what's wrong with this version?

I personally like v7 version, I would just add this patch


diff --git a/vdagent/display_configuration.cpp b/vdagent/display_configuration.cpp
index 1504a60..9cc39ae 100644
--- a/vdagent/display_configuration.cpp
+++ b/vdagent/display_configuration.cpp
@@ -17,6 +17,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 #include "display_configuration.h"
 #include <winternl.h>
+#include <spice/macros.h>
 
 /* The following definitions and structures are taken
 from the wine project repository and can be found
@@ -631,7 +632,7 @@ D3D_HANDLE WDDMInterface::handle_from_GDI_name(LPCTSTR adapter_name)
     NTSTATUS status;
 
     ZeroMemory(&gdi_display_name, sizeof(gdi_display_name));
-    wcsncpy(gdi_display_name.DeviceName, adapter_name, sizeof(TCHAR)* CCHDEVICENAME);
+    wcsncpy(gdi_display_name.DeviceName, adapter_name, SPICE_N_ELEMENTS(gdi_display_name.DeviceName));
 
     if (NT_SUCCESS(status = _pfnOpen_adapter_gdi_name(&gdi_display_name))) {
         return  gdi_display_name.hAdapter;

which fixes a potential buffer overflow (wcsncpy require number of characters, not bytes)


Other versions rely on QXL_ESCAPE which was removed or v10 which is the latest.
I don't like malloc/free as they does not fit with the current style, are not exception safe and
are heavy for small static structure allocations.

Frediano

Frediano

> diff --git a/vdagent/display_configuration.cpp
> b/vdagent/display_configuration.cpp
> index 01fdbb0..bab4c95 100644
> --- a/vdagent/display_configuration.cpp
> +++ b/vdagent/display_configuration.cpp
> @@ -153,6 +153,56 @@ struct DISPLAYCONFIG_PATH_INFO {
>      UINT32 flags;
>  };
>
> +/* The following definitions and structures are taken
> + * from here
> https://github.com/notr1ch/DWMCapture/blob/master/DWMCaptureSource.cpp */
> +
> +enum D3DKMT_ESCAPETYPE {
> +    D3DKMT_ESCAPE_DRIVERPRIVATE = 0
> +};
> +
> +struct D3DDDI_ESCAPEFLAGS {
> +    union {
> +        struct {
> +            UINT    Reserved : 31;
> +        };
> +        UINT        Value;
> +    };
> +};
> +
> +struct D3DKMT_ESCAPE {
> +    D3D_HANDLE hAdapter;
> +    D3D_HANDLE hDevice;
> +    D3DKMT_ESCAPETYPE Type;
> +    D3DDDI_ESCAPEFLAGS Flags;
> +    VOID* pPrivateDriverData;
> +    UINT PrivateDriverDataSize;
> +    D3D_HANDLE hContext;
> +};
> +
> +struct D3DKMT_OPENADAPTERFROMHDC {
> +    HDC hDc;
> +    D3D_HANDLE hAdapter;
> +    LUID AdapterLuid;
> +    UINT VidPnSourceId;
> +};
> +
> +struct D3DKMT_CLOSEADAPTER {
> +    D3D_HANDLE hAdapter;
> +};
> +
> +struct D3DKMT_OPENADAPTERFROMDEVICENAME {
> +    const WCHAR *pDeviceName;
> +    D3D_HANDLE hAdapter;
> +    LUID AdapterLuid;
> +};
> +
> +struct D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME {
> +    WCHAR DeviceName[32];
> +    D3D_HANDLE hAdapter;
> +    LUID AdapterLuid;
> +    UINT VidPnSourceId;
> +};
> +
>  struct QXLMonitorEscape {
>      QXLMonitorEscape(DEVMODE* dev_mode)
>      {
> @@ -178,7 +228,14 @@ struct QxlCustomEscapeObj : public
> QXLEscapeSetCustomDisplay {
>  DisplayConfig* DisplayConfig::create_config()
>  {
>      DisplayConfig* new_interface;
> -    new_interface = new XPDMInterface();
> +    /* Try to open a WDDM adapter.
> +    If that failed, assume we have an XPDM driver */
> +    try {
> +        new_interface = new  WDDMInterface();
> +    }
> +    catch (std::exception& e) {
> +        new_interface = new XPDMInterface();
> +    }
>      return new_interface;
>  }
>
> @@ -328,6 +385,293 @@ bool XPDMInterface::find_best_mode(LPCTSTR Device,
> DEVMODE* dev_mode)
>      return NT_SUCCESS(status);
>  }
>
> +WDDMInterface::WDDMInterface()
> +    : _pfnOpen_adapter_hdc(NULL)
> +    , _pfnClose_adapter(NULL)
> +    , _pfnEscape(NULL)
> +    , _pfnOpen_adapter_device_name(NULL)
> +    , _pfnOpen_adapter_gdi_name(NULL)
> +{
> +    LONG error(0);
> +    //Can we find the D3D calls we need?
> +    if (!init_d3d_api()) {
> +        throw std::exception();
> +    }
> +
> +    //Initialize  CCD path stuff
> +    if (!_ccd.query_display_config()) {
> +        throw std::exception();
> +    }
> +
> +    if (!_ccd.set_display_config(error)) {
> +        throw std::exception();
> +    }
> +}
> +
> +bool WDDMInterface::is_attached(DISPLAY_DEVICE* dev_info)
> +{
> +    return _ccd.is_attached(dev_info->DeviceName);
> +}
> +
> +bool WDDMInterface::set_monitor_state(LPCTSTR device_name, DEVMODE*
> dev_mode, MONITOR_STATE state)
> +{
> +   return  _ccd.set_path_state(device_name, state);
> +}
> +
> +bool WDDMInterface::custom_display_escape(LPCTSTR device_name, DEVMODE*
> dev_mode)
> +{
> +    DISPLAYCONFIG_MODE_INFO* mode = _ccd.get_active_mode(device_name,
> false);
> +    if (!mode) {
> +        return false;
> +    }
> +
> +    //Don't bother if we are already set to the new resolution
> +    if (mode->sourceMode.width == dev_mode->dmPelsWidth &&
> +        mode->sourceMode.height == dev_mode->dmPelsHeight) {
> +        return true;
> +    }
> +
> +    vd_printf("%s: updating %S resolution\n", __FUNCTION__, device_name);
> +
> +    size_t data_size = sizeof(QXLEscape) +
> sizeof(QXLEscapeSetCustomDisplay);
> +    QXLEscape * qxl_escape = (QXLEscape *) malloc(data_size);
> +    qxl_escape->ioctl = QXL_ESCAPE_SET_CUSTOM_DISPLAY;
> +    QXLEscapeSetCustomDisplay * wddm_escape = (QXLEscapeSetCustomDisplay
> *)qxl_escape->data;
> +    wddm_escape->bpp = dev_mode->dmBitsPerPel;
> +    wddm_escape->xres = dev_mode->dmPelsWidth;
> +    wddm_escape->yres = dev_mode->dmPelsHeight;
> +    bool escape_result = escape(device_name, qxl_escape, data_size);
> +    free(qxl_escape);
> +    if (escape_result) {
> +        return _ccd.update_mode_size(device_name, dev_mode);
> +    }
> +
> +    vd_printf("%s: (%dx%d)", __FUNCTION__, mode->sourceMode.width,
> mode->sourceMode.height);
> +    return false;
> +}
> +
> +bool WDDMInterface::update_monitor_config(LPCTSTR device_name, DisplayMode*
> display_mode,
> +                                           DEVMODE* dev_mode)
> +{
> +    if (!display_mode || !display_mode->get_attached()) {
> +        return false;
> +    }
> +    DISPLAYCONFIG_MODE_INFO* mode = _ccd.get_active_mode(device_name,
> false);
> +    if (!mode || !_send_monitors_config)
> +        return false;
> +
> +    size_t data_size = sizeof(QXLEscape) + sizeof(QXLHead);
> +    QXLEscape * qxl_escape = (QXLEscape *) malloc(data_size);
> +    qxl_escape->ioctl = QXL_ESCAPE_MONITOR_CONFIG;
> +    QXLHead * wddm_escape = (QXLHead *) qxl_escape->data;
> +    wddm_escape->id = wddm_escape->surface_id = 0;
> +    wddm_escape->x = display_mode->get_pos_x();
> +    wddm_escape->y = display_mode->get_pos_y();
> +    wddm_escape->width = display_mode->get_width();
> +    wddm_escape->height = display_mode->get_height();
> +    bool escape_result = escape(device_name, qxl_escape, data_size);
> +    free(qxl_escape);
> +    if (escape_result) {
> +        //Update the path position
> +        return _ccd.update_mode_position(device_name, dev_mode);
> +    }
> +
> +    vd_printf("%s: %S failed", __FUNCTION__, device_name);
> +    return false;
> +
> +}
> +
> +LONG WDDMInterface::update_display_settings()
> +{
> +    LONG error(0);
> +    //If we removed the primary monitor since the last call, we need to
> +    //reorder the other monitors, making the leftmost one the primary
> +    _ccd.verify_primary_position();
> +    _ccd.set_display_config(error);
> +    return error;
> +}
> +
> +void WDDMInterface::update_config_path()
> +{
> +    _ccd.query_display_config();
> +}
> +
> +bool WDDMInterface::update_dev_mode_position(LPCTSTR device_name, DEVMODE*
> dev_mode,
> +                                             LONG x, LONG y)
> +{
> +    dev_mode->dmPosition.x = x;
> +    dev_mode->dmPosition.y = y;
> +    return _ccd.update_mode_position(device_name, dev_mode);
> +}
> +
> +bool WDDMInterface::init_d3d_api()
> +{
> +    HMODULE hModule = GetModuleHandle(L"gdi32.dll");
> +
> +    //Look for the gdi32 functions we need to perform driver escapes
> +    if (!hModule) {
> +        vd_printf("%s something wildly wrong as we can't open gdi32.dll",
> __FUNCTION__);
> +        return false;
> +    }
> +
> +    do {
> +        _pfnClose_adapter = (PFND3DKMT_CLOSEADAPTER)
> +            GetProcAddress(hModule, "D3DKMTCloseAdapter");
> +        if (!_pfnClose_adapter) {
> +            break;
> +        }
> +
> +        _pfnEscape = (PFND3DKMT_ESCAPE) GetProcAddress(hModule,
> "D3DKMTEscape");
> +        if (!_pfnEscape) {
> +            break;
> +        }
> +
> +        _pfnOpen_adapter_hdc = (PFND3DKMT_OPENADAPTERFROMHDC)
> +            GetProcAddress(hModule, "D3DKMTOpenAdapterFromHdc");
> +        if (!_pfnOpen_adapter_hdc) {
> +            break;
> +        }
> +
> +        _pfnOpen_adapter_device_name = (PFND3DKMT_OPENADAPTERFROMDEVICENAME)
> +            GetProcAddress(hModule, "D3DKMTOpenAdapterFromDeviceName");
> +        if (!_pfnOpen_adapter_device_name) {
> +            break;
> +        }
> +
> +        _pfnOpen_adapter_gdi_name =
> (PFND3DKMT_OPENADAPTERFROMGDIDISPLAYNAME)
> +            GetProcAddress(hModule, "D3DKMTOpenAdapterFromGdiDisplayName");
> +        if (!_pfnOpen_adapter_gdi_name) {
> +            break;
> +        }
> +
> +    }
> +    while(0);
> +
> +    //Did we get them ?
> +    if (!_pfnClose_adapter || !_pfnOpen_adapter_hdc || !_pfnEscape)  {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +D3D_HANDLE WDDMInterface::adapter_handle(LPCTSTR device_name)
> +{
> +    D3D_HANDLE hAdapter(0);
> +
> +    //For some reason, this call will occasionally fail.
> +    if ((hAdapter = handle_from_DC(device_name))) {
> +        return hAdapter;
> +    }
> +        //So try other available methods.
> +    if (_pfnOpen_adapter_device_name && (hAdapter =
> handle_from_device_name(device_name))) {
> +        return hAdapter;
> +    }
> +    //One last chance to open this guy
> +    if (_pfnOpen_adapter_gdi_name) {
> +        hAdapter = handle_from_GDI_name(device_name);
> +    }
> +
> +    if (!hAdapter) {
> +        vd_printf("%s: failed to open adapter %S", __FUNCTION__,
> device_name);
> +    }
> +
> +    return hAdapter;
> +}
> +
> +D3D_HANDLE WDDMInterface::handle_from_DC(LPCTSTR adapter_name)
> +{
> +    NTSTATUS status;
> +    D3DKMT_OPENADAPTERFROMHDC open_data;
> +    HDC hDc(CreateDC(adapter_name, NULL, NULL, NULL));
> +
> +    if (!hDc) {
> +        vd_printf("%s: %S CreateDC failed with %lu", __FUNCTION__,
> adapter_name, GetLastError());
> +        return 0;
> +    }
> +
> +    ZeroMemory(&open_data, sizeof(D3DKMT_OPENADAPTERFROMHDC));
> +    open_data.hDc = hDc;
> +
> +    if (!NT_SUCCESS(status = _pfnOpen_adapter_hdc(&open_data))) {
> +        vd_printf("%s: %S open adapter from hdc failed with %lu",
> __FUNCTION__, adapter_name,
> +            status);
> +        open_data.hAdapter = 0;
> +    }
> +
> +    DeleteDC(hDc);
> +    return open_data.hAdapter;
> +}
> +
> +D3D_HANDLE WDDMInterface::handle_from_device_name(LPCTSTR adapter_name)
> +{
> +    D3DKMT_OPENADAPTERFROMDEVICENAME display_name_data;
> +    NTSTATUS  status;
> +
> +    ZeroMemory(&display_name_data, sizeof(display_name_data));
> +    display_name_data.pDeviceName = adapter_name;
> +
> +    if (NT_SUCCESS(status =
> _pfnOpen_adapter_device_name(&display_name_data))) {
> +        return display_name_data.hAdapter;
> +    }
> +
> +    vd_printf("%s %S failed with 0x%lx", __FUNCTION__, adapter_name,
> status);
> +    return 0;
> +}
> +
> +D3D_HANDLE WDDMInterface::handle_from_GDI_name(LPCTSTR adapter_name)
> +{
> +    D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME gdi_display_name;
> +    NTSTATUS status;
> +
> +    ZeroMemory(&gdi_display_name, sizeof(gdi_display_name));
> +    wcsncpy(gdi_display_name.DeviceName, adapter_name, sizeof(TCHAR)*
> CCHDEVICENAME);
> +
> +    if (NT_SUCCESS(status = _pfnOpen_adapter_gdi_name(&gdi_display_name))) {
> +        return  gdi_display_name.hAdapter;
> +    }
> +
> +    vd_printf("%s: %S aurrrgghh nothing works..error  is 0x%lx",
> __FUNCTION__, adapter_name,
> +            status);
> +    return 0;
> +}
> +
> +void WDDMInterface::close_adapter(D3D_HANDLE handle)
> +{
> +    D3DKMT_CLOSEADAPTER closeData;
> +    if (handle) {
> +        closeData.hAdapter = handle;
> +        _pfnClose_adapter(&closeData);
> +    }
> +}
> +
> +bool WDDMInterface::escape(LPCTSTR device_name, void* data, UINT size_data)
> +{
> +    D3DKMT_ESCAPE   escapeData;
> +    NTSTATUS        status;
> +    D3D_HANDLE   hAdapter(0);
> +
> +    if (!(hAdapter = adapter_handle(device_name)))
> +        return false;
> +
> +    escapeData.hAdapter = hAdapter;
> +    escapeData.hDevice = 0;
> +    escapeData.hContext = 0;
> +    escapeData.Type = D3DKMT_ESCAPE_DRIVERPRIVATE;
> +    escapeData.Flags.Value = 0;
> +    escapeData.pPrivateDriverData = data;
> +    escapeData.PrivateDriverDataSize = size_data;
> +
> +    status = _pfnEscape(&escapeData);
> +
> +    if (!NT_SUCCESS(status)) {
> +        vd_printf("%s: this should never happen. Status is 0x%lx",
> __FUNCTION__, status);
> +    }
> +
> +    //Close the handle to this device
> +    close_adapter(hAdapter);
> +    return NT_SUCCESS(status);
> +}
> +
>  CCD::CCD()
>      :_numPathElements(0)
>      ,_numModeElements(0)
> diff --git a/vdagent/display_configuration.h
> b/vdagent/display_configuration.h
> index 388ac9d..7abf526 100644
> --- a/vdagent/display_configuration.h
> +++ b/vdagent/display_configuration.h
> @@ -125,4 +125,51 @@ private:
>      bool find_best_mode(LPCTSTR Device, DEVMODE* dev_mode);
>  };
>
> +//DisplayConfig implementation for guest with WDDM graphics drivers
> +typedef UINT D3D_HANDLE;
> +
> +struct D3DKMT_ESCAPE;
> +struct D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME;
> +struct D3DKMT_OPENADAPTERFROMDEVICENAME;
> +struct D3DKMT_CLOSEADAPTER;
> +struct D3DKMT_OPENADAPTERFROMHDC;
> +
> +typedef NTSTATUS(APIENTRY* PFND3DKMT_ESCAPE)(CONST D3DKMT_ESCAPE*);
> +typedef NTSTATUS(APIENTRY*
> PFND3DKMT_OPENADAPTERFROMGDIDISPLAYNAME)(D3DKMT_OPENADAPTERFROMGDIDISPLAYNAME*);
> +typedef NTSTATUS(APIENTRY*
> PFND3DKMT_OPENADAPTERFROMDEVICENAME)(D3DKMT_OPENADAPTERFROMDEVICENAME*);
> +typedef NTSTATUS(APIENTRY* PFND3DKMT_CLOSEADAPTER)(D3DKMT_CLOSEADAPTER*);
> +typedef NTSTATUS(APIENTRY*
> PFND3DKMT_OPENADAPTERFROMHDC)(D3DKMT_OPENADAPTERFROMHDC*);
> +
> +class WDDMInterface : public DisplayConfig {
> +public:
> +    WDDMInterface();
> +    bool is_attached(DISPLAY_DEVICE* dev_info);
> +    bool set_monitor_state(LPCTSTR device_name, DEVMODE* dev_mode,
> MONITOR_STATE state);
> +    LONG update_display_settings();
> +    bool custom_display_escape(LPCTSTR device_name, DEVMODE* dev_mode);
> +    bool update_monitor_config(LPCTSTR device_name, DisplayMode* mode,
> DEVMODE* dev_mode);
> +    bool update_dev_mode_position(LPCTSTR device_name, DEVMODE * dev_mode,
> LONG x, LONG y);
> +    void update_config_path();
> +
> +private:
> +    bool init_d3d_api();
> +    D3D_HANDLE adapter_handle(LPCTSTR device_name);
> +    D3D_HANDLE handle_from_DC(LPCTSTR adapter_name);
> +    D3D_HANDLE handle_from_device_name(LPCTSTR adapter_name);
> +    D3D_HANDLE handle_from_GDI_name(LPCTSTR adapter_name);
> +
> +    void close_adapter(D3D_HANDLE handle);
> +    bool escape(LPCTSTR device_name, void* data, UINT sizeData);
> +
> +    //GDI Function pointers
> +    PFND3DKMT_OPENADAPTERFROMHDC _pfnOpen_adapter_hdc;
> +    PFND3DKMT_CLOSEADAPTER  _pfnClose_adapter;
> +    PFND3DKMT_ESCAPE _pfnEscape;
> +    PFND3DKMT_OPENADAPTERFROMDEVICENAME _pfnOpen_adapter_device_name;
> +    PFND3DKMT_OPENADAPTERFROMGDIDISPLAYNAME _pfnOpen_adapter_gdi_name;
> +
> +    //object handles the CCD API
> +    CCD _ccd;
> +};
> +
>  #endif
> \ No newline at end of file



--
Respectfully,
Sameeh Jubran
Junior Software Engineer @ Daynix.

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]