[ofw] WDK build environment migration thoughts

[Alex Naslednikov]

Good questions !
Q1. Unless CONCAT is used elsewhere, I would not add that #define.  It's
too generic of a name.
I put #ifndef around this macro for the case it had been defined, but
it's possible to rename it too, say, CONCAT_STR 
Please, see my next mail with the example of a patch
 
Q2.Does TO_LONG_PTR work for both big endian and little endian systems?
TO_LONG_PTR works only with little endian, because we do not support PPC
platform 
Currently, there's no need to extend it to support big endian
 
Q3. How is the padded space initialized?
Inside sdp, all padded space was initialized by class constructor
(except one specific case)
In all other code, that not C++, padded space was initialized by
cl_memclr before setting the field and before calling to ioctl procedure
Please, see my next mail with examples from the code
 
Q4. WDK migration
Code can be compiled both in WDK and DDK. Of course, we carefully check
it with our regression system for all combination of platforms (x86/x64)
and environments
 
XaleX
 

________________________________

From: Sean Hefty [mailto:sean.hefty at intel.com] 
Sent: Monday, April 21, 2008 8:09 PM
To: Alex Naslednikov; Smith, Stan; Ishai Rabinovitz
Cc: ofw at lists.openfabrics.org
Subject: RE: [ofw] WDK build environment migration thoughts



1. __ptr64 problem

Briefly speaking, this problem arises when copying 32bit len pointer
into 64bit len pointer. In this case, signed pointer extension will take
place.

How it's applicable to WinOF ?  A lot of pointer were declared to be
__ptr64 (i.e., to be always "long", even in 32bit kernel systems),
that's to preserve on unique size of structs used in IOCTL calls.  The
main problem it will cause is between 32bit user applications and 64bit
kernel application.

When user code do operation like 

s_ptr = &my_struct;

my_type* __ptr64 ptr = s_ptr;

Than kernel will receive ptr with invalid upper bits data (4 bytes FF).

To avoid signed pointer extension, PtrToPtr64() function should be used.

Also, I found some other places where dangerous signed pointer extension
took place, even on 32bit kernel.

Yet another problem that arises with __ptr64 attribute is internal
compiler error (C1001)  in WDK when using __ptr64 pointer to function
(callback)

This problem was described in ofw discussion, you can see also :

http://blogs.msdn.com/texblog/archive/2005/10/31/487436.aspx
<http://blogs.msdn.com/texblog/archive/2005/10/31/487436.aspx> 

http://lists.openfabrics.org/pipermail/ofw/2007-July/001613.html
<http://lists.openfabrics.org/pipermail/ofw/2007-July/001613.html>
(posted by Jan from OFW)

Our solution:

1. Initially, we decided to remove all __ptr64 attributes except those
ones inside IOCTL structures. After, put PtrToPtr64() conversion on
every assignment to long pointer.

(like my_type* __ptr64 ptr = PtrToPtr64(s_ptr);  )

During this solution, we changed a huge amount of code, so patch became
unreadable. And it was difficult to validate that all long pointer (with
__ptr64 attribute) were used in a proper manner

2. So, we decided about another solution:

 All __ptr64 occurrences were replaced by either:

 i) TO_LONG_PTR(type, field) macro, when occurred inside structure

ii) VOID_PTR64 macro otherwise (defined as void macro)

#define CONCAT(str1, str2) str1##str2

#define TO_LONG_PTR(type,member_name) \

    union { type member_name;  uint64_t CONCAT(member_name,_padding) ; }

 

Unless CONCAT is used elsewhere, I would not add that #define.  It's too
generic of a name.

Does TO_LONG_PTR work for both big endian and little endian systems?
How is the padded space initialized?

 

Thus, we can both preserve on a uniform shapes of structs in user and
kernel and to avoid unsafe pointer arithmetic !

The patch now is much more readable, but it sill consist of thousands
lines.

 

IBAL uses typedefs everywhere.  Why couldn't the typedefs be redefined
as 'UINT64' instead of struct _blah __ptr64?  Compiled code shouldn't
notice the difference, and we'd get compiler warnings for any casting
errors, that we could then clean up.  It doesn't seem like this would
produce a patch that's any bigger or more difficult to read, but the
resulting code would be cleaner than introducing unnamed unions with
potential padding that requires additional initialization.

 

2. Migration to WDK

Main issue here was to preserve on backward compatibility with DDK

 

Personally, I don't see the need to be backward compatible with the DDK.
Let's just require the latest WDK to build the drivers.

- Sean

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080421/844e662a/attachment.html>