Re: [PATCH v4 3/9] tools/libxc: Stream specification and some common code

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/05/14 12:57, Ian Campbell wrote:
> On Wed, 2014-04-30 at 19:36 +0100, Andrew Cooper wrote:
>> diff --git a/tools/libxc/saverestore/stream_format.h b/tools/libxc/saverestore/stream_format.h
>> new file mode 100644
>> index 0000000..efcca60
>> --- /dev/null
>> +++ b/tools/libxc/saverestore/stream_format.h
> Reference to the spec (which doesn't appear to be in this series
> anywhere BTW)

The spec pdf is referenced in the header.  I suppose it should move into
docs/.

>
>> @@ -0,0 +1,158 @@
>> +#ifndef __STREAM_FORMAT__H
>> +#define __STREAM_FORMAT__H
>> +
>> +#include <inttypes.h>
>> +
>> +/* TODO - find somewhere more appropriate. rec_tsc_info needs it for 64bit */
> You seem to use it on all of them though. Can we not use explicit
> padding, as you seem to do for many of the other structs?

The issue with rec_tsc_info is its trailing alignment.  i.e.
sizeof(rec_tsc_info) is different between 32 and 64 bit builds, yet all
fields are at the same offset from the beginning.  Putting explicit
trailing padding would invalidate the current code writing
sizeof(rec_tsc_info) bytes into the stream.

Inside Xen when I was sorting out the __attribute__((packed))
consistency, we did agree on __packed for binary interfaces even when
not strictly necessary.

>
>> +#ifndef __packed
>> +#define __packed __attribute__((packed))
>> +#endif
>> +
>> +/*
>> + * Image Header
>> + */
>> +struct __packed ihdr
> So long as this remains confined to libxc/saverestore/*.[ch] we can
> avoid namespacing/prefixing. I suppose that is a safe assumption.

Hmm yes.  (This I suppose is still "v1 code hacking to make something work")

Probably do want some namespacing even if it does only live inside this
code, although an obvious prefix doesn't jump to mind.

>
>> +{
>> +    uint64_t marker;
>> +    uint32_t id;
>> +    uint32_t version;
>> +    uint16_t options;
>> +    uint16_t _res1;
>> +    uint32_t _res2;
>> +};
>> +
>> +#define IHDR_MARKER  0xffffffffffffffffULL
>> +#define IHDR_ID      0x58454E46U
>> +#define IHDR_VERSION 1
>> +
>> +#define _IHDR_OPT_ENDIAN 0
>> +#define IHDR_OPT_LITTLE_ENDIAN (0 << _IHDR_OPT_ENDIAN)
>> +#define IHDR_OPT_BIG_ENDIAN    (1 << _IHDR_OPT_ENDIAN)
>> +
>> +/*
>> + * Domain Header
>> + */
>> +struct __packed dhdr
>> +{
>> +    uint32_t type;
>> +    uint16_t page_shift;
>> +    uint16_t _res1;
>> +    uint32_t xen_major;
>> +    uint32_t xen_minor;
>> +};
>> +
>> +#define DHDR_TYPE_x86_pv  0x00000001U
>> +#define DHDR_TYPE_x86_hvm 0x00000002U
>> +#define DHDR_TYPE_x86_pvh 0x00000003U
>> +#define DHDR_TYPE_arm     0x00000004U
> That capitalisation here (and elsewhere) seems rather unconventional. 
>
> BTW IHDR_OPT is also inconsistent with the pattern you've established.
> TBH I'd prefer more conventional upper case #defines. I suppose you want
> to avoid enums for the ABI-confusing properties they have.
>
>

This particular bit is to match the spec, but I see what you mean.  I
shall make it consistently upper case.

I hadn't actually considered an enum.  It might be ok as each type field
is passed as a uint32_t, although changing it would be faff for no
appreciable gain.

~Andrew