[Spice-devel] [PATCH qxl-wddm-dod 07/26] Do not use virtual functions for code that must not be paged
Frediano Ziglio
fziglio at redhat.com
Wed Aug 24 14:36:31 UTC 2016
> It seems that after all this patch is unnecessary at all. By default all of
> the driver code is nonpaged unless stated otherwise using pragma directives.
We are not talking about code here but vtables which are data with code pointers inside.
However even data are by default in not paged sections, at least from VS 2015 compilation. Still not clear to me in which section VS put vtables (some data are put in .text section which is supposed to contains code).
What make me worry is that the original patch came from a set of patches supposed to fix some validation tool issues. This could mean that section allocation changed from VS version to another or that verification tool detected a false positive or the this patch was not a fix to a validation issue.
Frediano
> On Thu, Aug 18, 2016 at 8:25 PM, Frediano Ziglio < fziglio at redhat.com >
> wrote:
> > > >
>
> > > > From: Sandy Stutsman < sstutsma at redhat.com >
>
> > > >
>
> > > > There is no way to guarantee that the static vtable will be in
>
> > > > non-pageable memory. This patch converts 3 virtual non-pageble
> > > > functions
>
> > > > to functions private to each device class, the parent class will make
>
> > > > an explicit call (based on a new device type initialized at object
>
> > > > creation) to the correct function.
>
> > > >
>
> > > > Fixed one misspelling: HwDeviceInterface
>
> > >
>
> > > Ok for the spelling.
>
> > > This patch is moving some code around instead of marking function
>
> > > to specific sections with #pragma code_seg.
>
> > > vtable are generated in the constant section (by default .rodata) if this
>
> > > is pageable I would rather define a section (#pragma section) and
> > > generate
>
> > > all constants in this new section (#pragma const_seg).
>
> > >
>
> > Had a look at some driver.
>
> > Looks like all main sections (.rdata, .data, etc) are not pageable.
>
> > A better check where the vtable are allocated should be done but this
>
> > looks like a false positive.
>
> > Where can I get the last repository or the binary (not sure the binary
>
> > is however enough, perhaps object files would be better) ?
>
> > Frediano
>
> > >
>
> > > > ---
>
> > > > qxldod/QxlDod.cpp | 114
>
> > > > ++++++++++++++++++++++++++++++++++++++++++------------
>
> > > > qxldod/QxlDod.h | 48 +++++++++++++++--------
>
> > > > 2 files changed, 120 insertions(+), 42 deletions(-)
>
> > > >
>
> > > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
>
> > > > index 1b3bdf1..ec960e5 100755
>
> > > > --- a/qxldod/QxlDod.cpp
>
> > > > +++ b/qxldod/QxlDod.cpp
>
> > > > @@ -2294,7 +2294,7 @@ VOID BltBits (
>
> > > > }
>
> > > > #pragma code_seg(pop) // End Non-Paged Code
>
> > > >
>
> > > > -VgaDevice::VgaDevice(_In_ QxlDod* pQxlDod)
>
> > > > +VgaDevice::VgaDevice(_In_ QxlDod* pQxlDod) :
> > > > HwDeviceInterface(pQxlDod)
>
> > > > {
>
> > > > m_pQxlDod = pQxlDod;
>
> > > > m_ModeInfo = NULL;
>
> > > > @@ -2302,6 +2302,7 @@ VgaDevice::VgaDevice(_In_ QxlDod* pQxlDod)
>
> > > > m_ModeNumbers = NULL;
>
> > > > m_CurrentMode = 0;
>
> > > > m_Id = 0;
>
> > > > + m_type = VGA_DEVICE;
>
> > > > }
>
> > > >
>
> > > > VgaDevice::~VgaDevice(void)
>
> > > > @@ -2881,18 +2882,18 @@ VOID
> > > > VgaDevice::BlackOutScreen(CURRENT_BDD_MODE*
>
> > > > pCurrentBddMod)
>
> > > > #pragma code_seg()
>
> > > > // BEGIN: Non-Paged Code Segment
>
> > > >
>
> > > > -BOOLEAN VgaDevice::InterruptRoutine(_In_ PDXGKRNL_INTERFACE
>
> > > > pDxgkInterface,
>
> > > > _In_ ULONG MessageNumber)
>
> > > > +BOOLEAN VgaDevice::HWInterruptRoutine(_In_ PDXGKRNL_INTERFACE
>
> > > > pDxgkInterface, _In_ ULONG MessageNumber)
>
> > > > {
>
> > > > UNREFERENCED_PARAMETER(pDxgkInterface);
>
> > > > UNREFERENCED_PARAMETER(MessageNumber);
>
> > > > return FALSE;
>
> > > > }
>
> > > >
>
> > > > -VOID VgaDevice::DpcRoutine(PVOID)
>
> > > > +VOID VgaDevice::HWDpcRoutine(PDXGKRNL_INTERFACE pDxgkInterface)
>
> > > > {
>
> > > > }
>
> > > >
>
> > > > -VOID VgaDevice::ResetDevice(VOID)
>
> > > > +VOID VgaDevice::HWResetDevice(VOID)
>
> > > > {
>
> > > > }
>
> > > > #pragma code_seg(pop) //end non-paged code
>
> > > > @@ -2916,7 +2917,7 @@ NTSTATUS VgaDevice::Escape(_In_ CONST
> > > > DXGKARG_ESCAPE*
>
> > > > pEscap)
>
> > > > return STATUS_NOT_IMPLEMENTED;
>
> > > > }
>
> > > >
>
> > > > -QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>
> > > > +QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod) :
> > > > HwDeviceInterface(pQxlDod)
>
> > > > {
>
> > > > m_pQxlDod = pQxlDod;
>
> > > > m_ModeInfo = NULL;
>
> > > > @@ -2926,6 +2927,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>
> > > > m_CustomMode = 0;
>
> > > > m_FreeOutputs = 0;
>
> > > > m_Pending = 0;
>
> > > > + m_type = QXL_DEVICE;
>
> > > > }
>
> > > >
>
> > > > QxlDevice::~QxlDevice(void)
>
> > > > @@ -3152,7 +3154,7 @@ NTSTATUS QxlDevice::SetPowerState(_In_
>
> > > > DEVICE_POWER_STATE DevicePowerState, DXGK
>
> > > > NTSTATUS QxlDevice::HWInit(PCM_RESOURCE_LIST pResList,
>
> > > > DXGK_DISPLAY_INFORMATION* pDispInfo)
>
> > > > {
>
> > > > DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>
> > > > - PDXGKRNL_INTERFACE pDxgkInterface = m_pQxlDod->GetDxgkInterrface();
>
> > > > + PDXGKRNL_INTERFACE pDxgkInterface = m_pQxlDod->GetDxgkInterface();
>
> > > > UINT pci_range = QXL_RAM_RANGE_INDEX;
>
> > > > for (ULONG i = 0; i < pResList->Count; ++i)
>
> > > > {
>
> > > > @@ -3329,7 +3331,7 @@ void QxlDevice::QxlClose()
>
> > > >
>
> > > > void QxlDevice::UnmapMemory(void)
>
> > > > {
>
> > > > - PDXGKRNL_INTERFACE pDxgkInterface = m_pQxlDod->GetDxgkInterrface();
>
> > > > + PDXGKRNL_INTERFACE pDxgkInterface = m_pQxlDod->GetDxgkInterface();
>
> > > > if (m_IoMapped && m_IoBase)
>
> > > > {
>
> > > > pDxgkInterface->DxgkCbUnmapMemory( pDxgkInterface->DeviceHandle,
>
> > > > &m_IoBase);
>
> > > > @@ -4498,7 +4500,8 @@ VOID QxlDevice::PushCursor(VOID)
>
> > > > #pragma code_seg(push)
>
> > > > #pragma code_seg()
>
> > > > // BEGIN: Non-Paged Code Segment
>
> > > > -BOOLEAN QxlDevice::InterruptRoutine(_In_ PDXGKRNL_INTERFACE
>
> > > > pDxgkInterface,
>
> > > > _In_ ULONG MessageNumber)
>
> > > > +
>
> > > > +BOOLEAN QxlDevice::HWInterruptRoutine(_In_ PDXGKRNL_INTERFACE
>
> > > > pDxgkInterface, _In_ ULONG MessageNumber)
>
> > > > {
>
> > > > UNREFERENCED_PARAMETER(MessageNumber);
>
> > > > DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>
> > > > @@ -4523,10 +4526,8 @@ BOOLEAN QxlDevice::InterruptRoutine(_In_
>
> > > > PDXGKRNL_INTERFACE pDxgkInterface, _In_
>
> > > > return TRUE;
>
> > > > }
>
> > > >
>
> > > > -VOID QxlDevice::DpcRoutine(PVOID ptr)
>
> > > > +VOID QxlDevice::HWDpcRoutine(PDXGKRNL_INTERFACE pDxgkInterface)
>
> > > > {
>
> > > > - PDXGKRNL_INTERFACE pDxgkInterface = (PDXGKRNL_INTERFACE)ptr;
>
> > > > -
>
> > > > DbgPrint(TRACE_LEVEL_INFORMATION, ("---> %s\n", __FUNCTION__));
>
> > > > DPC_CB_CONTEXT ctx;
>
> > > > BOOLEAN dummy;
>
> > > > @@ -4557,13 +4558,89 @@ VOID QxlDevice::DpcRoutine(PVOID ptr)
>
> > > > DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__));
>
> > > > }
>
> > > >
>
> > > > -void QxlDevice::ResetDevice(void)
>
> > > > +void QxlDevice::HWResetDevice(void)
>
> > > > {
>
> > > > DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>
> > > > m_RamHdr->int_mask = ~0;
>
> > > > WRITE_PORT_UCHAR(m_IoBase + QXL_IO_MEMSLOT_ADD, 0);
>
> > > > DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
>
> > > > }
>
> > > > +
>
> > > > +// Given bits per pixel, return the pixel format at the same bpp
>
> > > > +D3DDDIFORMAT PixelFormatFromBPP(UINT BPP)
>
> > > > +{
>
> > > > + switch (BPP)
>
> > > > + {
>
> > > > + case 8: return D3DDDIFMT_P8;
>
> > > > + case 16: return D3DDDIFMT_R5G6B5;
>
> > > > + case 24: return D3DDDIFMT_R8G8B8;
>
> > > > + case 32: return D3DDDIFMT_X8R8G8B8;
>
> > > > + default: QXL_LOG_ASSERTION1("A bit per pixel of 0x%I64x is not
>
> > > > supported.", BPP); return D3DDDIFMT_UNKNOWN;
>
> > > > + }
>
> > > > +}
>
> > > > +
>
> > > > +
>
> > > > +BOOLEAN HwDeviceInterface::InterruptRoutine(_In_ PDXGKRNL_INTERFACE
>
> > > > pDxgkInterface, _In_ ULONG MessageNumber)
>
> > > > +{
>
> > > > + QxlDevice * qxl_device;
>
> > > > + VgaDevice * vga_device;
>
> > > > +
>
> > > > + switch (m_type) {
>
> > > > + case QXL_DEVICE:
>
> > > > + qxl_device = (QxlDevice *)this;
>
> > > > + return qxl_device->HWInterruptRoutine(pDxgkInterface,
>
> > > > MessageNumber);
>
> > > > + case VGA_DEVICE:
>
> > > > + vga_device = (VgaDevice *)this;
>
> > > > + return vga_device->HWInterruptRoutine(pDxgkInterface,
>
> > > > MessageNumber);
>
> > > > + default:
>
> > > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s called with invalid device type
>
> > > > of
>
> > > > %d\n",
>
> > > > + __FUNCTION__, m_type));
>
> > > > + return FALSE;
>
> > > > + }
>
> > > > +}
>
> > > > +
>
> > > > +VOID HwDeviceInterface::DpcRoutine(PDXGKRNL_INTERFACE pDxgkInterface)
>
> > > > +{
>
> > > > + QxlDevice * qxl_device;
>
> > > > + VgaDevice * vga_device;
>
> > > > +
>
> > > > + switch (m_type) {
>
> > > > + case QXL_DEVICE:
>
> > > > + qxl_device = (QxlDevice *)this;
>
> > > > + qxl_device->HWDpcRoutine(pDxgkInterface);
>
> > > > + return;
>
> > > > + case VGA_DEVICE:
>
> > > > + vga_device = (VgaDevice *)this;
>
> > > > + vga_device->HWDpcRoutine(pDxgkInterface);
>
> > > > + return;
>
> > > > + default:
>
> > > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s called with invalid device type
>
> > > > of
>
> > > > %d\n",
>
> > > > + __FUNCTION__, m_type));
>
> > > > + return;
>
> > > > + }
>
> > > > +}
>
> > > > +
>
> > > > +VOID HwDeviceInterface::ResetDevice(void)
>
> > > > +{
>
> > > > + QxlDevice * qxl_device;
>
> > > > + VgaDevice * vga_device;
>
> > > > +
>
> > > > + switch (m_type) {
>
> > > > + case QXL_DEVICE:
>
> > > > + qxl_device = (QxlDevice *)this;
>
> > > > + qxl_device->HWResetDevice();
>
> > > > + return;
>
> > > > + case VGA_DEVICE:
>
> > > > + vga_device = (VgaDevice *)this;
>
> > > > + vga_device->HWResetDevice();
>
> > > > + return;
>
> > > > + default:
>
> > > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s called with invalid device type
>
> > > > of
>
> > > > %d\n",
>
> > > > + __FUNCTION__, m_type));
>
> > > > + return;
>
> > > > + }
>
> > > > +}
>
> > > > +
>
> > > > #pragma code_seg(pop) //end non-paged code
>
> > > >
>
> > > > VOID QxlDevice::UpdateArea(CONST RECT* area, UINT32 surface_id)
>
> > > > @@ -4592,19 +4669,6 @@ VOID QxlDevice::DpcCallback(PDPC_CB_CONTEXT ctx)
>
> > > >
>
> > > > }
>
> > > >
>
> > > > -// Given bits per pixel, return the pixel format at the same bpp
>
> > > > -D3DDDIFORMAT PixelFormatFromBPP(UINT BPP)
>
> > > > -{
>
> > > > - switch (BPP)
>
> > > > - {
>
> > > > - case 8: return D3DDDIFMT_P8;
>
> > > > - case 16: return D3DDDIFMT_R5G6B5;
>
> > > > - case 24: return D3DDDIFMT_R8G8B8;
>
> > > > - case 32: return D3DDDIFMT_X8R8G8B8;
>
> > > > - default: QXL_LOG_ASSERTION1("A bit per pixel of 0x%I64x is not
>
> > > > supported.", BPP); return D3DDDIFMT_UNKNOWN;
>
> > > > - }
>
> > > > -}
>
> > > > -
>
> > > > UINT SpiceFromPixelFormat(D3DDDIFORMAT Format)
>
> > > > {
>
> > > > switch (Format)
>
> > > > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
>
> > > > index eb9bea7..db343e9 100755
>
> > > > --- a/qxldod/QxlDod.h
>
> > > > +++ b/qxldod/QxlDod.h
>
> > > > @@ -212,21 +212,34 @@ typedef struct _CURRENT_BDD_MODE
>
> > > >
>
> > > > class QxlDod;
>
> > > >
>
> > > > -class HwDeviceIntrface {
>
> > > > +//
>
> > > > +// Can't use virtual for functions that are non-pageable because there
> > > > is
>
> > > > no guarantee
>
> > > > +// that the vtable will not be paged out. Will test for type in the
>
> > > > parent
>
> > > > call and
>
> > > > +// cast to call in the correct device class.
>
> > > > +//
>
> > > > +
>
> > > > +typedef enum {
>
> > > > + QXL_DEVICE,
>
> > > > + VGA_DEVICE,
>
> > > > + INVALID_DEVICE,
>
> > > > +}WIN_QXL_DEVICE_TYPE;
>
> > > > +
>
> > > > +class HwDeviceInterface {
>
> > > > public:
>
> > > > -// HwDeviceIntrface(_In_ QxlDod* pQxlDod) {;}
>
> > > > - virtual ~HwDeviceIntrface() {;}
>
> > > > + HwDeviceInterface(_In_ QxlDod* pQxlDod) {m_type = INVALID_DEVICE;}
>
> > > > + virtual ~HwDeviceInterface() {;}
>
> > > > virtual NTSTATUS QueryCurrentMode(PVIDEO_MODE RequestedMode) = 0;
>
> > > > virtual NTSTATUS SetCurrentMode(ULONG Mode) = 0;
>
> > > > virtual NTSTATUS GetCurrentMode(ULONG* Mode) = 0;
>
> > > > virtual NTSTATUS SetPowerState(DEVICE_POWER_STATE DevicePowerState,
>
> > > > DXGK_DISPLAY_INFORMATION* pDispInfo) = 0;
>
> > > > virtual NTSTATUS HWInit(PCM_RESOURCE_LIST pResList,
>
> > > > DXGK_DISPLAY_INFORMATION* pDispInfo) = 0;
>
> > > > virtual NTSTATUS HWClose(void) = 0;
>
> > > > - virtual BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE
>
> > > > pDxgkInterface,
>
> > > > _In_ ULONG MessageNumber) = 0;
>
> > > > - virtual VOID DpcRoutine(PVOID) = 0;
>
> > > > - virtual VOID ResetDevice(void) = 0;
>
> > > > -
>
> > > > virtual ULONG GetModeCount(void) = 0;
>
> > > > +
>
> > > > + BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface, _In_
>
> > > > ULONG MessageNumber);;
>
> > > > + VOID DpcRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface);
>
> > > > + VOID ResetDevice(void);;
>
> > > > +
>
> > > > PVIDEO_MODE_INFORMATION GetModeInfo(UINT idx) {return
>
> > > > &m_ModeInfo[idx];}
>
> > > > USHORT GetModeNumber(USHORT idx) {return m_ModeNumbers[idx];}
>
> > > > USHORT GetCurrentModeIndex(void) {return m_CurrentMode;}
>
> > > > @@ -259,10 +272,11 @@ protected:
>
> > > > USHORT m_CurrentMode;
>
> > > > USHORT m_CustomMode;
>
> > > > ULONG m_Id;
>
> > > > + WIN_QXL_DEVICE_TYPE m_type;
>
> > > > };
>
> > > >
>
> > > > class VgaDevice :
>
> > > > - public HwDeviceIntrface
>
> > > > + public HwDeviceInterface
>
> > > > {
>
> > > > public:
>
> > > > VgaDevice(_In_ QxlDod* pQxlDod);
>
> > > > @@ -287,9 +301,9 @@ public:
>
> > > > _In_ D3DKMDT_VIDPN_PRESENT_PATH_ROTATION
>
> > > > Rotation,
>
> > > > _In_ const CURRENT_BDD_MODE* pModeCur);
>
> > > > VOID BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod);
>
> > > > - BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface, _In_
>
> > > > ULONG MessageNumber);
>
> > > > - VOID DpcRoutine(PVOID);
>
> > > > - VOID ResetDevice(VOID);
>
> > > > + BOOLEAN HWInterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface,
>
> > > > _In_
>
> > > > ULONG MessageNumber);
>
> > > > + VOID HWDpcRoutine(PDXGKRNL_INTERFACE pDxgkInterface);
>
> > > > + VOID HWResetDevice(VOID);
>
> > > > NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
>
> > > > pSetPointerShape);
>
> > > > NTSTATUS SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION*
>
> > > > pSetPointerPosition);
>
> > > > NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap);
>
> > > > @@ -435,7 +449,7 @@ typedef struct DpcCbContext {
>
> > > > #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
>
> > > >
>
> > > > class QxlDevice :
>
> > > > - public HwDeviceIntrface
>
> > > > + public HwDeviceInterface
>
> > > > {
>
> > > > public:
>
> > > > QxlDevice(_In_ QxlDod* pQxlDod);
>
> > > > @@ -460,9 +474,9 @@ public:
>
> > > > _In_ D3DKMDT_VIDPN_PRESENT_PATH_ROTATION Rotation,
>
> > > > _In_ const CURRENT_BDD_MODE* pModeCur);
>
> > > > VOID BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod);
>
> > > > - BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface, _In_
>
> > > > ULONG MessageNumber);
>
> > > > - VOID DpcRoutine(PVOID);
>
> > > > - VOID ResetDevice(VOID);
>
> > > > + BOOLEAN HWInterruptRoutine(_In_ PDXGKRNL_INTERFACE pDxgkInterface,
>
> > > > _In_
>
> > > > ULONG MessageNumber);
>
> > > > + VOID HWDpcRoutine(PDXGKRNL_INTERFACE pDxgkInterface);
>
> > > > + VOID HWResetDevice(VOID);
>
> > > > NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
>
> > > > pSetPointerShape);
>
> > > > NTSTATUS SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION*
>
> > > > pSetPointerPosition);
>
> > > > NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap);
>
> > > > @@ -592,7 +606,7 @@ private:
>
> > > >
>
> > > > D3DDDI_VIDEO_PRESENT_SOURCE_ID m_SystemDisplaySourceId;
>
> > > > DXGKARG_SETPOINTERSHAPE m_PointerShape;
>
> > > > - HwDeviceIntrface* m_pHWDevice;
>
> > > > + HwDeviceInterface* m_pHWDevice;
>
> > > > public:
>
> > > > QxlDod(_In_ DEVICE_OBJECT* pPhysicalDeviceObject);
>
> > > > ~QxlDod(void);
>
> > > > @@ -685,7 +699,7 @@ public:
>
> > > > _In_
>
> > > > UINT
>
> > > > SourceStride,
>
> > > > _In_
>
> > > > INT
>
> > > > PositionX,
>
> > > > _In_
>
> > > > INT
>
> > > > PositionY);
>
> > > > - PDXGKRNL_INTERFACE GetDxgkInterrface(void) { return
> > > > &m_DxgkInterface;}
>
> > > > + PDXGKRNL_INTERFACE GetDxgkInterface(void) { return &m_DxgkInterface;}
>
> > > > private:
>
> > > > VOID CleanUp(VOID);
>
> > > > NTSTATUS CheckHardware();
>
> > > _______________________________________________
>
> > > Spice-devel mailing list
>
> > > Spice-devel at lists.freedesktop.org
>
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> > >
>
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Junior Software Engineer @ Daynix .
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160824/141802f9/attachment-0001.html>
More information about the Spice-devel
mailing list