Re: [PATCH 4/5] xen: Add V4V implementation

[Attilio Rao <attilio.rao@citrix.com>]

On 19/07/12 11:06, Jean Guyader wrote:
> On 19 July 2012 10:54, Attilio Rao<attilio.rao@citrix.com>  wrote:
>    
>> On 19/07/12 10:58, Jean Guyader wrote:
>>      
>>> On 19 July 2012 10:34, Andrew Cooper<andrew.cooper3@citrix.com>   wrote:
>>>
>>>        
>>>> On 18/07/12 21:09, Jean Guyader wrote:
>>>>
>>>>          
>>>>> On 29 June 2012 11:36, Jan Beulich<JBeulich@suse.com>   wrote:
>>>>>
>>>>>            
>>>>>>>>> On 29.06.12 at 12:03, Jean Guyader<jean.guyader@gmail.com>   wrote:
>>>>>>>>>
>>>>>>>>>                    
>>>>>>> On 29 June 2012 09:33, Jan Beulich<JBeulich@suse.com>   wrote:
>>>>>>>
>>>>>>>                
>>>>>>>>>>> On 28.06.12 at 18:26, Jean Guyader<jean.guyader@citrix.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>                        
>>>>>>>>> +typedef struct v4v_ring_id
>>>>>>>>> +{
>>>>>>>>> +    struct v4v_addr addr;
>>>>>>>>> +    domid_t partner;
>>>>>>>>> +} V4V_PACKED v4v_ring_id_t;
>>>>>>>>> +
>>>>>>>>>
>>>>>>>>>                    
>>>>>>> This structure is really the one that cause trouble. domid_t is 16b
>>>>>>> and v4v_addr_t is used
>>>>>>> inside v4v_ring_t. I would like the structure to remind as close as we
>>>>>>> can from the original version
>>>>>>> as we already versions in the field. Having explicit padding will make
>>>>>>> all the structures different
>>>>>>> which will make much harder to write a driver that will support the
>>>>>>> two versions of the API.
>>>>>>>
>>>>>>>                
>>>>>> Oh, I see, "partner" would end up on a different offset if the
>>>>>> packed attribute was removed from v4v_addr_t. But that
>>>>>> could still be solved by making this type a union:
>>>>>>
>>>>>> typedef union v4v_ring_id
>>>>>> {
>>>>>>       struct v4v_addr addr;
>>>>>>       struct {
>>>>>>           uint32_t port;
>>>>>>           domid_t domain;
>>>>>>           domid_t partner;
>>>>>>       } full;
>>>>>> } v4v_ring_id_t;
>>>>>>
>>>>>> That would guarantee binary compatibility. And you could even
>>>>>> achieve source compatibility for gcc users by making the naming
>>>>>> of the second structure conditional upon __GNUC__ being
>>>>>> undefined (or adding a second instance of the same, just
>>>>>> unnamed structure within a respective #ifdef - that would make
>>>>>> it possible to write code that can be compiled by both gcc and
>>>>>> non-gcc, yet existing gcc-only code would need changing).
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> Also most all the consumer of those headers will have to rewrite the
>>>>>>> structure anyway, for instance
>>>>>>> the Linux kernel have it's own naming convention, macros definitions
>>>>>>> which are different, etc..
>>>>>>>
>>>>>>>                
>>>>>> Such can usually be done via scripts, so having a fully defined
>>>>>> public header is still worthwhile.
>>>>>>
>>>>>>
>>>>>>              
>>>>> Hi,
>>>>>
>>>>> I've been working on this and it work for most of it apart from one
>>>>> case.
>>>>> Let's take this structure:
>>>>>
>>>>> struct a
>>>>> {
>>>>>       uint64_t a;
>>>>>       uint32_t b;
>>>>>
>>>>>            
>>>>       uint32_t _pad0;
>>>>
>>>>          
>>>>>       uint16_t c;
>>>>>       uint16_t d;
>>>>>       uint32_t e;
>>>>>       uint32_t f;
>>>>>       uint32_t g;
>>>>>       uint8_t  h[32];
>>>>>       uint8_t  q[0];
>>>>> };
>>>>>
>>>>>            
>>>> Manually padding so the alignment is the same on 32 and 64 bit is the
>>>> only way to do this in the public headers, which cant have gcc'isms for
>>>> compatibility reasons with other compilers.
>>>>
>>>>
>>>>          
>>> The problem isn't with the individual fields (they are all correctly
>>> aligned) it is
>>> the the overall structure size which is 64 even so offset of q is 60
>>> (and sizeof q
>>> should be 0).
>>>
>>> I think there is no way around it. The structure I have should be
>>> aligned on 64b anyway.
>>>
>>>
>>>        
>>
>> Can you use gcc __attribute__((aligned(64))) for this? Or we try to avoid
>> gcc-ism at all?
>>
>>      
> I'm trying to avoid any compiler specific statement. I'll added some
> manual padding
> to have it explicitly aligned to 64b.
>
>    
Ideally though you can have an intermediate header where you workout 
compiler specific code (in particular if you are almost sure your code 
will always be likely compiled with gcc).
So, you can have an header which does the following:

#ifdef __GNUC__
#define _compiler_align(x) __attribute__((__aligned__(x)))
#elif __INTEL_COMPILER
#define _compiler_align(x) foo
#else
#define _compiler_align(x)
#endif

Probabily for something like that you want to use some header that is 
known to be included in a lot of files.

Attilio