Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 31.08.17 at 15:21,  wrote:
> So, will it be acceptable to use my approach with that union?

As per Ian's reply, go with just the containerized uint8_t[].

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Ian Jackson]

Volodymyr Babchuk writes ("Re: [PATCH v4 03/11] public: xen.h: add definitions 
for UUID handling"):
> Do you have any ideas how to indicate endianess of the fields, then? I 
> can just write it in the comments. But I fear of misuse.

I definitely prefer your approach of providing only an array.  (It
needs to be in a struct for typesafety, as discussed.)  At least that
way the semantics are clear.

No-one is likely to want to access these individual fields.  If they
do so they are doing something very wrong.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

On 31.08.17 15:53, Jan Beulich wrote:

On 31.08.17 at 14:24,  wrote:

Hi Jan,

On 31.08.17 10:34, Jan Beulich wrote:

On 30.08.17 at 18:20,  wrote:

My first intention was to declare union with all possible
representations, so it would be possible to access the same UUID as an
array of bytes or, for example, as Microsoft GUID. Like this:

typedef union {
   /* UUID represented as a 128-bit object */
   uint8_t obj[16];

   /* Representation according to RFC 4122 */
   struct {
   __be32  time_low;
   __be16  time_mid;
   __be16  time_hi_and_version;
   __u8clock_seq_hi_and_reserved;
   __u8clock_seq_low;
   __u8node[6];
   } rfc4122;

   /* Microsoft/Intel style GUID representation */
   struct {
   __le32  Data1;
   __le16  Data2;
   __le16  Data3;
   __u8Data4[8];
   } guid;

   /* SMCCC compatible format */
   struct {
   __le32 r0;
   __le32 r1;
   __le32 r2;
   __le32 r3;
   } smccc;
} xen_uuid_t;


But looks like we can't use something like __packed or
__attribute__((__packed__)) in the public header. This means that we
can't rely on right overlapping and users of this union should take care
to read and write only to one chosen substructure.


I don't see any use of that attribute in the structure definition
above, nor any need to add one - all fields are suitably aligned
anyway. You can't use __be* and __le* types in the public
headers, though - these will need to be uint*_t.

This is a public header. As I understand it can be used by different
compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to
padding or alignment of fields in structures. No one can guarantee that

sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) ==
sizeof(xen_uuid_t.smccc)  == 16

On all platforms. Using any compiler. With any compiler options.

This is implementation defined ([1]). Standard says "This should present
no problem unless binary data written by one implementation are read by
another.". But in case of public headers, this structures can be written
by one implementation and read by another.


My reference to C89 was to tell you what language constructs
you're allowed to use. For binary layout, conventions also
matter (like gABI and processor specific ABIs). Without that we
wouldn't be able to write any C header in compatible manner.
What helps us greatly is that we're not needing interfaces for
cross-host communication - the entire public interface assumes
that producer and consumer run on the same physical system
(or, for the parts concerning migration, on similar ones).


So, will it be acceptable to use my approach with that union?

Do you have any ideas how to indicate endianess of the fields, then? I 
can just write it in the comments. But I fear of misuse.



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 31.08.17 at 14:24,  wrote:
> Hi Jan,
> 
> On 31.08.17 10:34, Jan Beulich wrote:
> On 30.08.17 at 18:20,  wrote:
>>> My first intention was to declare union with all possible
>>> representations, so it would be possible to access the same UUID as an
>>> array of bytes or, for example, as Microsoft GUID. Like this:
>>>
>>> typedef union {
>>>   /* UUID represented as a 128-bit object */
>>>   uint8_t obj[16];
>>>
>>>   /* Representation according to RFC 4122 */
>>>   struct {
>>>   __be32  time_low;
>>>   __be16  time_mid;
>>>   __be16  time_hi_and_version;
>>>   __u8clock_seq_hi_and_reserved;
>>>   __u8clock_seq_low;
>>>   __u8node[6];
>>>   } rfc4122;
>>>
>>>   /* Microsoft/Intel style GUID representation */
>>>   struct {
>>>   __le32  Data1;
>>>   __le16  Data2;
>>>   __le16  Data3;
>>>   __u8Data4[8];
>>>   } guid;
>>>
>>>   /* SMCCC compatible format */
>>>   struct {
>>>   __le32 r0;
>>>   __le32 r1;
>>>   __le32 r2;
>>>   __le32 r3;
>>>   } smccc;
>>> } xen_uuid_t;
>>>
>>>
>>> But looks like we can't use something like __packed or
>>> __attribute__((__packed__)) in the public header. This means that we
>>> can't rely on right overlapping and users of this union should take care
>>> to read and write only to one chosen substructure.
>> 
>> I don't see any use of that attribute in the structure definition
>> above, nor any need to add one - all fields are suitably aligned
>> anyway. You can't use __be* and __le* types in the public
>> headers, though - these will need to be uint*_t.
> This is a public header. As I understand it can be used by different 
> compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to 
> padding or alignment of fields in structures. No one can guarantee that
> 
> sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) == 
> sizeof(xen_uuid_t.smccc)  == 16
> 
> On all platforms. Using any compiler. With any compiler options.
> 
> This is implementation defined ([1]). Standard says "This should present 
> no problem unless binary data written by one implementation are read by 
> another.". But in case of public headers, this structures can be written 
> by one implementation and read by another.

My reference to C89 was to tell you what language constructs
you're allowed to use. For binary layout, conventions also
matter (like gABI and processor specific ABIs). Without that we
wouldn't be able to write any C header in compatible manner.
What helps us greatly is that we're not needing interfaces for
cross-host communication - the entire public interface assumes
that producer and consumer run on the same physical system
(or, for the parts concerning migration, on similar ones).

Jan



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hi Jan,

On 31.08.17 10:34, Jan Beulich wrote:

On 30.08.17 at 18:20,  wrote:

My first intention was to declare union with all possible
representations, so it would be possible to access the same UUID as an
array of bytes or, for example, as Microsoft GUID. Like this:

typedef union {
  /* UUID represented as a 128-bit object */
  uint8_t obj[16];

  /* Representation according to RFC 4122 */
  struct {
  __be32  time_low;
  __be16  time_mid;
  __be16  time_hi_and_version;
  __u8clock_seq_hi_and_reserved;
  __u8clock_seq_low;
  __u8node[6];
  } rfc4122;

  /* Microsoft/Intel style GUID representation */
  struct {
  __le32  Data1;
  __le16  Data2;
  __le16  Data3;
  __u8Data4[8];
  } guid;

  /* SMCCC compatible format */
  struct {
  __le32 r0;
  __le32 r1;
  __le32 r2;
  __le32 r3;
  } smccc;
} xen_uuid_t;


But looks like we can't use something like __packed or
__attribute__((__packed__)) in the public header. This means that we
can't rely on right overlapping and users of this union should take care
to read and write only to one chosen substructure.


I don't see any use of that attribute in the structure definition
above, nor any need to add one - all fields are suitably aligned
anyway. You can't use __be* and __le* types in the public
headers, though - these will need to be uint*_t.
This is a public header. As I understand it can be used by different 
compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to 
padding or alignment of fields in structures. No one can guarantee that


sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) == 
sizeof(xen_uuid_t.smccc)  == 16


On all platforms. Using any compiler. With any compiler options.

This is implementation defined ([1]). Standard says "This should present 
no problem unless binary data written by one implementation are read by 
another.". But in case of public headers, this structures can be written 
by one implementation and read by another.



BTW, I'm very interested how it can be guaranteed that structures
defined in xen.h will have the same size and alignment on both sides of
communication channel, taking into account, then we rely only on C89
standard.


I don't understand this comment.
See my reply above. There absolutely no guarantees, that MSVC compiler 
will not add extra padding somewhere. This will impair interoperability.
I think, this is why (amongst other reasons) WinAPI structures has 
dwSize as the first field. And this is why _IO* macros in Linux kernel 
use sizeof() to create ioctl number.

But, as I can see, XEN code checks only magic or version.

[1] http://port70.net/~nsz/c/c89/c89-draft.html#A.6.3.8

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 30.08.17 at 18:20,  wrote:
> My first intention was to declare union with all possible 
> representations, so it would be possible to access the same UUID as an 
> array of bytes or, for example, as Microsoft GUID. Like this:
> 
> typedef union {
>  /* UUID represented as a 128-bit object */
>  uint8_t obj[16];
> 
>  /* Representation according to RFC 4122 */
>  struct {
>  __be32  time_low;
>  __be16  time_mid;
>  __be16  time_hi_and_version;
>  __u8clock_seq_hi_and_reserved;
>  __u8clock_seq_low;
>  __u8node[6];
>  } rfc4122;
> 
>  /* Microsoft/Intel style GUID representation */
>  struct {
>  __le32  Data1;
>  __le16  Data2;
>  __le16  Data3;
>  __u8Data4[8];
>  } guid;
> 
>  /* SMCCC compatible format */
>  struct {
>  __le32 r0;
>  __le32 r1;
>  __le32 r2;
>  __le32 r3;
>  } smccc;
> } xen_uuid_t;
> 
> 
> But looks like we can't use something like __packed or 
> __attribute__((__packed__)) in the public header. This means that we 
> can't rely on right overlapping and users of this union should take care 
> to read and write only to one chosen substructure.

I don't see any use of that attribute in the structure definition
above, nor any need to add one - all fields are suitably aligned
anyway. You can't use __be* and __le* types in the public
headers, though - these will need to be uint*_t.

> BTW, I'm very interested how it can be guaranteed that structures 
> defined in xen.h will have the same size and alignment on both sides of 
> communication channel, taking into account, then we rely only on C89 
> standard.

I don't understand this comment.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hi Jan,

On 23.08.17 14:29, Jan Beulich wrote:

On 23.08.17 at 13:08,  wrote:

On 23.08.17 11:10, Jan Beulich wrote:

On 22.08.17 at 16:37,  wrote:

I can't see why you want to map UUID to a certain structure.


This is so that the type cannot mistakenly be passed to a function
taking unsigned char *, or be assigned to a variable of that type.

Right, I see the point there.


Please see our TYPE_SAFE() macro which we use to specifically
enclose certain scalar types in a structure to that they won't be
compatible with other types deriving from the same scalar base type.

I see. So what about

struct xen_uuid_t
{
   uint8_t a[16];
};

then?


Yes, that's what I had asked for as the minimal solution. That
would be in line with (but better than) xen_domain_handle_t,
which I've just realized we also have.


One can convert it to union with different representations (array,
RFC4122 struct, etc) later if there will be need for this.


Well, why don't you make it a union but stick to just the array
for now if you dislike making it similar to the EFI one? That way
we can add further representations if needed/desired without
breaking existing consumers.
My first intention was to declare union with all possible 
representations, so it would be possible to access the same UUID as an 
array of bytes or, for example, as Microsoft GUID. Like this:


typedef union {
/* UUID represented as a 128-bit object */
uint8_t obj[16];

/* Representation according to RFC 4122 */
struct {
__be32  time_low;
__be16  time_mid;
__be16  time_hi_and_version;
__u8clock_seq_hi_and_reserved;
__u8clock_seq_low;
__u8node[6];
} rfc4122;

/* Microsoft/Intel style GUID representation */
struct {
__le32  Data1;
__le16  Data2;
__le16  Data3;
__u8Data4[8];
} guid;

/* SMCCC compatible format */
struct {
__le32 r0;
__le32 r1;
__le32 r2;
__le32 r3;
} smccc;
} xen_uuid_t;


But looks like we can't use something like __packed or 
__attribute__((__packed__)) in the public header. This means that we 
can't rely on right overlapping and users of this union should take care 
to read and write only to one chosen substructure. I think, this is 
error-prone, so it is better to stick to


typedef struct
{
   uint8_t a[16];
} xen_uuid_t;

BTW, I'm very interested how it can be guaranteed that structures 
defined in xen.h will have the same size and alignment on both sides of 
communication channel, taking into account, then we rely only on C89 
standard.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 23.08.17 at 13:08,  wrote:
> On 23.08.17 11:10, Jan Beulich wrote:
> On 22.08.17 at 16:37,  wrote:
>>> I can't see why you want to map UUID to a certain structure.
>> 
>> This is so that the type cannot mistakenly be passed to a function
>> taking unsigned char *, or be assigned to a variable of that type.
> Right, I see the point there.
> 
>> Please see our TYPE_SAFE() macro which we use to specifically
>> enclose certain scalar types in a structure to that they won't be
>> compatible with other types deriving from the same scalar base type.
> I see. So what about
> 
> struct xen_uuid_t
> {
>   uint8_t a[16];
> };
> 
> then?

Yes, that's what I had asked for as the minimal solution. That
would be in line with (but better than) xen_domain_handle_t,
which I've just realized we also have.

> One can convert it to union with different representations (array, 
> RFC4122 struct, etc) later if there will be need for this.

Well, why don't you make it a union but stick to just the array
for now if you dislike making it similar to the EFI one? That way
we can add further representations if needed/desired without
breaking existing consumers.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hello Jan

On 23.08.17 11:10, Jan Beulich wrote:

On 22.08.17 at 16:37,  wrote:

I can't see why you want to map UUID to a certain structure.


This is so that the type cannot mistakenly be passed to a function
taking unsigned char *, or be assigned to a variable of that type.

Right, I see the point there.


Please see our TYPE_SAFE() macro which we use to specifically
enclose certain scalar types in a structure to that they won't be
compatible with other types deriving from the same scalar base type.

I see. So what about

struct xen_uuid_t
{
 uint8_t a[16];
};

then?

One can convert it to union with different representations (array, 
RFC4122 struct, etc) later if there will be need for this.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 22.08.17 at 16:37,  wrote:
> I can't see why you want to map UUID to a certain structure.

This is so that the type cannot mistakenly be passed to a function
taking unsigned char *, or be assigned to a variable of that type.
Please see our TYPE_SAFE() macro which we use to specifically
enclose certain scalar types in a structure to that they won't be
compatible with other types deriving from the same scalar base type.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hi Jan,

On 22.08.17 10:26, Jan Beulich wrote:

On 21.08.17 at 22:27,  wrote:

--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
  __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
  __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
  
+typedef uint8_t xen_uuid_t[16];


As expressed before, I'm opposed to this being a plain array. I've
pointed you at the EFI representation as an example; at the very
least I'd expect a wrapper structure around the array (which is
_not_ to say that I would ack such a patch, but at least I wouldn't
nak it).


EFI code uses GUID, which is product of Microsoft's NIH syndrome.

Let me cite some parts of RFC 4122:

4.1.  Format

   *The UUID format is 16 octets*; some bits of the eight octet variant
   field specified below determine finer structure.
.

4.1.2.  Layout and Byte Order
.
   In the absence of explicit application or presentation protocol
   specification to the contrary, a UUID is encoded as a 128-bit object,
   as follows:

   The fields are encoded as 16 octets, with the sizes and order of the
   fields defined above, and with each field encoded with the Most
   Significant Byte first (known as network byte order).  Note that the
   field names, particularly for multiplexed fields, follow historical
   practice.

   0   1   2   3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  time_low |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |   time_mid| time_hi_and_version   |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |clk_seq_hi_res |  clk_seq_low  | node (0-1)|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   | node (2-5)|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
.


BTW, GUID handling is incompatible with this RFC, because in GUID
first three fields are stored in LE format, while other fields are 
stored in BE format.


I can't see why you want to map UUID to a certain structure. I can 
create such wrapper, but it will be just dead code, because there are no 
users for it. Frankly, I can't imagine why someone will want to read, 
say, clk_seq_hi_res field of UUID.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 21.08.17 at 22:27,  wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
>  __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
>  __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
>  
> +typedef uint8_t xen_uuid_t[16];

As expressed before, I'm opposed to this being a plain array. I've
pointed you at the EFI representation as an example; at the very
least I'd expect a wrapper structure around the array (which is
_not_ to say that I would ack such a patch, but at least I wouldn't
nak it).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Added type xen_uuid_t. This type represents UUID as an array of 16
bytes in big endian format.

Added macro XEN_DEFINE_UUID that constructs UUID in the usual way:

 XEN_DEFINE_UUID(00112233, 4455, 6677, 8899, aabbccddeeff)

will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as
 {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88,
  0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}

NB: This is compatible with Linux kernel and with libuuid, but it is not
compatible with Microsoft, as they use mixed-endian encoding (some
components are little-endian, some are big-endian).

Signed-off-by: Volodymyr Babchuk 
---
 xen/include/public/xen.h | 9 +
 1 file changed, 9 insertions(+)

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2ac6b1e..d1a4765 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
 __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
 __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
 
+typedef uint8_t xen_uuid_t[16];
+#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \
+{((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF,\
+ ((a) >>  8) & 0xFF, ((a) >>  0) & 0xFF,\
+ ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,\
+ ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,\
+ ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,\
+e1, e2, e3, e4, e5, e6}
+
 #endif /* !__ASSEMBLY__ */
 
 /* Default definitions for macros used by domctl/sysctl. */
-- 
2.7.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel