[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
Thu Aug 18 17:25:17 UTC 2016


> > 
> > 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
> 


More information about the Spice-devel mailing list