diff mbox

[v4,1/4] x86/HVM: update the start info structure layout

Message ID 1455644269-40358-2-git-send-email-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné Feb. 16, 2016, 5:37 p.m. UTC
After some discussion around the new boot ABI consensus has been reached
about the layout and contents of the start info. The following patch updates
the layout to what has been agreed.

Also, the new layout is described in binary terms in order to avoid issues
with alignments when using C structs.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/include/xc_dom.h | 31 +++++++++++++++++++++++++
 xen/include/public/xen.h     | 55 ++++++++++++++++++++++++++++++--------------
 2 files changed, 69 insertions(+), 17 deletions(-)

Comments

Andrew Cooper Feb. 16, 2016, 7:13 p.m. UTC | #1
On 16/02/16 17:37, Roger Pau Monne wrote:
> After some discussion around the new boot ABI consensus has been reached
> about the layout and contents of the start info. The following patch updates
> the layout to what has been agreed.
>
> Also, the new layout is described in binary terms in order to avoid issues
> with alignments when using C structs.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Konrad Rzeszutek Wilk Feb. 16, 2016, 8:06 p.m. UTC | #2
On Tue, Feb 16, 2016 at 06:37:46PM +0100, Roger Pau Monne wrote:
> After some discussion around the new boot ABI consensus has been reached
> about the layout and contents of the start info. The following patch updates
> the layout to what has been agreed.

.. It would be good to have an URL to the discussion?
> 
> Also, the new layout is described in binary terms in order to avoid issues
> with alignments when using C structs.

Should the title say more about HVMLite/PVH instead of just 'HVM'?

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

You forgot Boris who is working on the Linux side. CC-ing him.

Actually please CC him on _ALL_ PVH related work.

> ---
>  tools/libxc/include/xc_dom.h | 31 +++++++++++++++++++++++++
>  xen/include/public/xen.h     | 55 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index cac4698..6ebe946 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -216,6 +216,37 @@ struct xc_dom_image {
>      struct xc_hvm_firmware_module smbios_module;
>  };
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +/* C representation of the x86/HVM start info layout.
> + *
> + * The canonical definition of this layout resides in public/xen.h, this
> + * is just a way to represent the layout described there using C types.
> + *
> + * NB: the packed attribute is not really needed, but it helps us enforce
> + * the fact this this is just a representation, and it might indeed
> + * be required in the future if there are alignment changes.
> + */
> +struct hvm_start_info {
> +    uint32_t magic;             /* Contains the magic value 0x336ec578       */
> +                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
> +    uint32_t version;           /* Version of this structure.                */
> +    uint32_t flags;             /* SIF_xxx flags.                            */
> +    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
> +    uint32_t modlist_paddr;     /* Physical address of an array of           */
> +                                /* hvm_modlist_entry.                        */
> +    uint32_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
> +                                /* structure.                                */
> +} __attribute__((packed));
> +
> +struct hvm_modlist_entry {
> +    uint64_t paddr;             /* Physical address of the module.           */
> +    uint64_t size;              /* Size of the module in bytes.              */
> +    uint64_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint64_t reserved;
> +} __attribute__((packed));
> +#endif /* x86 */
> +
>  /* --- pluggable kernel loader ------------------------------------- */
>  
>  struct xc_dom_loader {
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 7b629b1..6ba060f 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>  /*
>   * Start of day structure passed to PVH guests in %ebx.
>   *
> - * NOTE: nothing will be loaded at physical address 0, so
> - * a 0 value in any of the address fields should be treated
> - * as not present.
> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> + * of the address fields should be treated as not present.
> + *
> + *  0 +----------------+
> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
> + *  4 +----------------+
> + *    | version        | Version of this structure. Current version is 0. New
> + *    |                | versions are guaranteed to be backwards-compatible.
> + *  8 +----------------+
> + *    | flags          | SIF_xxx flags.
> + * 12 +----------------+
> + *    | cmdline_paddr  | Physical address of the command line,
> + *    |                | a zero-terminated ASCII string.
> + * 16 +----------------+
> + *    | nr_modules     | Number of modules passed to the kernel.
> + * 20 +----------------+
> + *    | modlist_paddr  | Physical address of an array of modules
> + *    |                | (layout of the structure below).
> + * 24 +----------------+
> + *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
> + * 28 +----------------+
> + *
> + * The layout of each entry in the module structure is the following:
> + *
> + *  0 +----------------+
> + *    | paddr          | Physical address of the module.
> + *  8 +----------------+
> + *    | size           | Size of the module in bytes.
> + * 16 +----------------+
> + *    | cmdline_paddr  | Physical address of the command line,
> + *    |                | a zero-terminated ASCII string.
> + * 24 +----------------+
> + *    | reserved       |
> + * 32 +----------------+
> + *
> + * The address and size of the modules is a 64bit unsigned integer. However
> + * Xen will always try to place all modules below the 4GiB boundary.
>   */
> -struct hvm_start_info {
>  #define HVM_START_MAGIC_VALUE 0x336ec578
> -    uint32_t magic;             /* Contains the magic value 0x336ec578       */
> -                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
> -    uint32_t flags;             /* SIF_xxx flags.                            */
> -    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
> -    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
> -    uint32_t modlist_paddr;     /* Physical address of an array of           */
> -                                /* hvm_modlist_entry.                        */
> -};
> -
> -struct hvm_modlist_entry {
> -    uint32_t paddr;             /* Physical address of the module.           */
> -    uint32_t size;              /* Size of the module in bytes.              */
> -};
>  
>  /* New console union for dom0 introduced in 0x00030203. */
>  #if __XEN_INTERFACE_VERSION__ < 0x00030203
> -- 
> 2.5.4 (Apple Git-61)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Boris Ostrovsky Feb. 16, 2016, 9:26 p.m. UTC | #3
On 02/16/2016 12:37 PM, Roger Pau Monne wrote:
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 7b629b1..6ba060f 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>   /*
>    * Start of day structure passed to PVH guests in %ebx.
>    *
> - * NOTE: nothing will be loaded at physical address 0, so
> - * a 0 value in any of the address fields should be treated
> - * as not present.
> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> + * of the address fields should be treated as not present.
> + *
> + *  0 +----------------+
> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
> + *  4 +----------------+
> + *    | version        | Version of this structure. Current version is 0. New
> + *    |                | versions are guaranteed to be backwards-compatible.

#define XEN_HVM_START_INFO_VERSION   0

is needed then?
Jan Beulich Feb. 17, 2016, 9:58 a.m. UTC | #4
>>> On 16.02.16 at 22:26, <boris.ostrovsky@oracle.com> wrote:
> On 02/16/2016 12:37 PM, Roger Pau Monne wrote:
>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>> index 7b629b1..6ba060f 100644
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>>   /*
>>    * Start of day structure passed to PVH guests in %ebx.
>>    *
>> - * NOTE: nothing will be loaded at physical address 0, so
>> - * a 0 value in any of the address fields should be treated
>> - * as not present.
>> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
>> + * of the address fields should be treated as not present.
>> + *
>> + *  0 +----------------+
>> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
>> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>> + *  4 +----------------+
>> + *    | version        | Version of this structure. Current version is 0. New
>> + *    |                | versions are guaranteed to be backwards-compatible.
> 
> #define XEN_HVM_START_INFO_VERSION   0

What would that buy us? Once it gets bumped to 1, consumers
would need to check against literal zero anyway.

Jan
Roger Pau Monné Feb. 17, 2016, 10:01 a.m. UTC | #5
El 16/2/16 a les 21:06, Konrad Rzeszutek Wilk ha escrit:
> On Tue, Feb 16, 2016 at 06:37:46PM +0100, Roger Pau Monne wrote:
>> After some discussion around the new boot ABI consensus has been reached
>> about the layout and contents of the start info. The following patch updates
>> the layout to what has been agreed.
> 
> .. It would be good to have an URL to the discussion?
>>
>> Also, the new layout is described in binary terms in order to avoid issues
>> with alignments when using C structs.
> 
> Should the title say more about HVMLite/PVH instead of just 'HVM'?

I guess x86/PVHv2 would be a better tag.

>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> You forgot Boris who is working on the Linux side. CC-ing him.
> 
> Actually please CC him on _ALL_ PVH related work.

Sure, I should have CC'ed him, David and yourself on this one, sorry.

Roger.
Roger Pau Monné Feb. 17, 2016, 10:05 a.m. UTC | #6
El 17/2/16 a les 10:58, Jan Beulich ha escrit:
>>>> On 16.02.16 at 22:26, <boris.ostrovsky@oracle.com> wrote:
>> On 02/16/2016 12:37 PM, Roger Pau Monne wrote:
>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>> index 7b629b1..6ba060f 100644
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>>>   /*
>>>    * Start of day structure passed to PVH guests in %ebx.
>>>    *
>>> - * NOTE: nothing will be loaded at physical address 0, so
>>> - * a 0 value in any of the address fields should be treated
>>> - * as not present.
>>> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
>>> + * of the address fields should be treated as not present.
>>> + *
>>> + *  0 +----------------+
>>> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
>>> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>> + *  4 +----------------+
>>> + *    | version        | Version of this structure. Current version is 0. New
>>> + *    |                | versions are guaranteed to be backwards-compatible.
>>
>> #define XEN_HVM_START_INFO_VERSION   0
> 
> What would that buy us? Once it gets bumped to 1, consumers
> would need to check against literal zero anyway.

I agree with Jan that this doesn't seem very useful, the headers don't
have to be in sync with the underlying hypervisor, so it's better to
just use literals instead of defines.

Roger.
Samuel Thibault Feb. 17, 2016, 10:45 a.m. UTC | #7
Roger Pau Monne, on Tue 16 Feb 2016 18:37:46 +0100, wrote:
> After some discussion around the new boot ABI consensus has been reached
> about the layout and contents of the start info. The following patch updates
> the layout to what has been agreed.
> 
> Also, the new layout is described in binary terms in order to avoid issues
> with alignments when using C structs.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/include/xc_dom.h | 31 +++++++++++++++++++++++++
>  xen/include/public/xen.h     | 55 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index cac4698..6ebe946 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -216,6 +216,37 @@ struct xc_dom_image {
>      struct xc_hvm_firmware_module smbios_module;
>  };
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +/* C representation of the x86/HVM start info layout.
> + *
> + * The canonical definition of this layout resides in public/xen.h, this
> + * is just a way to represent the layout described there using C types.
> + *
> + * NB: the packed attribute is not really needed, but it helps us enforce
> + * the fact this this is just a representation, and it might indeed
> + * be required in the future if there are alignment changes.
> + */
> +struct hvm_start_info {
> +    uint32_t magic;             /* Contains the magic value 0x336ec578       */
> +                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
> +    uint32_t version;           /* Version of this structure.                */
> +    uint32_t flags;             /* SIF_xxx flags.                            */
> +    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
> +    uint32_t modlist_paddr;     /* Physical address of an array of           */
> +                                /* hvm_modlist_entry.                        */
> +    uint32_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
> +                                /* structure.                                */
> +} __attribute__((packed));
> +
> +struct hvm_modlist_entry {
> +    uint64_t paddr;             /* Physical address of the module.           */
> +    uint64_t size;              /* Size of the module in bytes.              */
> +    uint64_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint64_t reserved;
> +} __attribute__((packed));
> +#endif /* x86 */
> +
>  /* --- pluggable kernel loader ------------------------------------- */
>  
>  struct xc_dom_loader {
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 7b629b1..6ba060f 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>  /*
>   * Start of day structure passed to PVH guests in %ebx.
>   *
> - * NOTE: nothing will be loaded at physical address 0, so
> - * a 0 value in any of the address fields should be treated
> - * as not present.
> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> + * of the address fields should be treated as not present.
> + *
> + *  0 +----------------+
> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
> + *  4 +----------------+
> + *    | version        | Version of this structure. Current version is 0. New
> + *    |                | versions are guaranteed to be backwards-compatible.
> + *  8 +----------------+
> + *    | flags          | SIF_xxx flags.
> + * 12 +----------------+
> + *    | cmdline_paddr  | Physical address of the command line,
> + *    |                | a zero-terminated ASCII string.
> + * 16 +----------------+
> + *    | nr_modules     | Number of modules passed to the kernel.
> + * 20 +----------------+
> + *    | modlist_paddr  | Physical address of an array of modules
> + *    |                | (layout of the structure below).
> + * 24 +----------------+
> + *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
> + * 28 +----------------+
> + *
> + * The layout of each entry in the module structure is the following:
> + *
> + *  0 +----------------+
> + *    | paddr          | Physical address of the module.
> + *  8 +----------------+
> + *    | size           | Size of the module in bytes.
> + * 16 +----------------+
> + *    | cmdline_paddr  | Physical address of the command line,
> + *    |                | a zero-terminated ASCII string.
> + * 24 +----------------+
> + *    | reserved       |
> + * 32 +----------------+
> + *
> + * The address and size of the modules is a 64bit unsigned integer. However
> + * Xen will always try to place all modules below the 4GiB boundary.
>   */
> -struct hvm_start_info {
>  #define HVM_START_MAGIC_VALUE 0x336ec578
> -    uint32_t magic;             /* Contains the magic value 0x336ec578       */
> -                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
> -    uint32_t flags;             /* SIF_xxx flags.                            */
> -    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
> -    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
> -    uint32_t modlist_paddr;     /* Physical address of an array of           */
> -                                /* hvm_modlist_entry.                        */
> -};
> -
> -struct hvm_modlist_entry {
> -    uint32_t paddr;             /* Physical address of the module.           */
> -    uint32_t size;              /* Size of the module in bytes.              */
> -};
>  
>  /* New console union for dom0 introduced in 0x00030203. */
>  #if __XEN_INTERFACE_VERSION__ < 0x00030203
> -- 
> 2.5.4 (Apple Git-61)
>
Jan Beulich Feb. 17, 2016, 1 p.m. UTC | #8
>>> On 16.02.16 at 18:37, <roger.pau@citrix.com> wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>  /*
>   * Start of day structure passed to PVH guests in %ebx.
>   *
> - * NOTE: nothing will be loaded at physical address 0, so
> - * a 0 value in any of the address fields should be treated
> - * as not present.
> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> + * of the address fields should be treated as not present.
> + *
> + *  0 +----------------+
> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
> + *  4 +----------------+
> + *    | version        | Version of this structure. Current version is 0. New
> + *    |                | versions are guaranteed to be backwards-compatible.
> + *  8 +----------------+
> + *    | flags          | SIF_xxx flags.
> + * 12 +----------------+
> + *    | cmdline_paddr  | Physical address of the command line,
> + *    |                | a zero-terminated ASCII string.
> + * 16 +----------------+
> + *    | nr_modules     | Number of modules passed to the kernel.
> + * 20 +----------------+
> + *    | modlist_paddr  | Physical address of an array of modules
> + *    |                | (layout of the structure below).
> + * 24 +----------------+
> + *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
> + * 28 +----------------+
> + *
> + * The layout of each entry in the module structure is the following:
> + *
> + *  0 +----------------+
> + *    | paddr          | Physical address of the module.
> + *  8 +----------------+
> + *    | size           | Size of the module in bytes.
> + * 16 +----------------+
> + *    | cmdline_paddr  | Physical address of the command line,
> + *    |                | a zero-terminated ASCII string.
> + * 24 +----------------+
> + *    | reserved       |
> + * 32 +----------------+
> + *
> + * The address and size of the modules is a 64bit unsigned integer. However
> + * Xen will always try to place all modules below the 4GiB boundary.
>   */
> -struct hvm_start_info {
>  #define HVM_START_MAGIC_VALUE 0x336ec578
> -    uint32_t magic;             /* Contains the magic value 0x336ec578       */

I would have wanted to take the opportunity and prefix this
constant with XEN_, but I see that the tools already use it. May
I please ask for a follow-up patch to correct this name space
issue?

Jan
Boris Ostrovsky Feb. 17, 2016, 2:39 p.m. UTC | #9
On 02/17/2016 05:05 AM, Roger Pau Monné wrote:
> El 17/2/16 a les 10:58, Jan Beulich ha escrit:
>>>>> On 16.02.16 at 22:26, <boris.ostrovsky@oracle.com> wrote:
>>> On 02/16/2016 12:37 PM, Roger Pau Monne wrote:
>>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>>> index 7b629b1..6ba060f 100644
>>>> --- a/xen/include/public/xen.h
>>>> +++ b/xen/include/public/xen.h
>>>> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>>>>    /*
>>>>     * Start of day structure passed to PVH guests in %ebx.
>>>>     *
>>>> - * NOTE: nothing will be loaded at physical address 0, so
>>>> - * a 0 value in any of the address fields should be treated
>>>> - * as not present.
>>>> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
>>>> + * of the address fields should be treated as not present.
>>>> + *
>>>> + *  0 +----------------+
>>>> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
>>>> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>>> + *  4 +----------------+
>>>> + *    | version        | Version of this structure. Current version is 0. New
>>>> + *    |                | versions are guaranteed to be backwards-compatible.
>>> #define XEN_HVM_START_INFO_VERSION   0
>> What would that buy us? Once it gets bumped to 1, consumers
>> would need to check against literal zero anyway.

Consumers would need to check against what their header file's version 
is, not necessarily zero. And they, for example, may decide not to run 
if the version provided by the structure is smaller than what they support.

-boris

> I agree with Jan that this doesn't seem very useful, the headers don't
> have to be in sync with the underlying hypervisor, so it's better to
> just use literals instead of defines.
>
> Roger.
Jan Beulich Feb. 17, 2016, 2:54 p.m. UTC | #10
>>> On 17.02.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> On 02/17/2016 05:05 AM, Roger Pau Monné wrote:
>> El 17/2/16 a les 10:58, Jan Beulich ha escrit:
>>>>>> On 16.02.16 at 22:26, <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/16/2016 12:37 PM, Roger Pau Monne wrote:
>>>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>>>> index 7b629b1..6ba060f 100644
>>>>> --- a/xen/include/public/xen.h
>>>>> +++ b/xen/include/public/xen.h
>>>>> @@ -787,25 +787,46 @@ typedef struct start_info start_info_t;
>>>>>    /*
>>>>>     * Start of day structure passed to PVH guests in %ebx.
>>>>>     *
>>>>> - * NOTE: nothing will be loaded at physical address 0, so
>>>>> - * a 0 value in any of the address fields should be treated
>>>>> - * as not present.
>>>>> + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
>>>>> + * of the address fields should be treated as not present.
>>>>> + *
>>>>> + *  0 +----------------+
>>>>> + *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
>>>>> + *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>>>> + *  4 +----------------+
>>>>> + *    | version        | Version of this structure. Current version is 0. 
> New
>>>>> + *    |                | versions are guaranteed to be backwards-compatible.
>>>> #define XEN_HVM_START_INFO_VERSION   0
>>> What would that buy us? Once it gets bumped to 1, consumers
>>> would need to check against literal zero anyway.
> 
> Consumers would need to check against what their header file's version 
> is, not necessarily zero.

Only if they aren't capable to deal with more than one version. Plus -
an update to the header would then go unnoticed, breaking the code.

> And they, for example, may decide not to run 
> if the version provided by the structure is smaller than what they support.

Achievable by doing checks against literal numbers.

Jan
diff mbox

Patch

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index cac4698..6ebe946 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -216,6 +216,37 @@  struct xc_dom_image {
     struct xc_hvm_firmware_module smbios_module;
 };
 
+#if defined(__i386__) || defined(__x86_64__)
+/* C representation of the x86/HVM start info layout.
+ *
+ * The canonical definition of this layout resides in public/xen.h, this
+ * is just a way to represent the layout described there using C types.
+ *
+ * NB: the packed attribute is not really needed, but it helps us enforce
+ * the fact this this is just a representation, and it might indeed
+ * be required in the future if there are alignment changes.
+ */
+struct hvm_start_info {
+    uint32_t magic;             /* Contains the magic value 0x336ec578       */
+                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
+    uint32_t version;           /* Version of this structure.                */
+    uint32_t flags;             /* SIF_xxx flags.                            */
+    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
+    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
+    uint32_t modlist_paddr;     /* Physical address of an array of           */
+                                /* hvm_modlist_entry.                        */
+    uint32_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
+                                /* structure.                                */
+} __attribute__((packed));
+
+struct hvm_modlist_entry {
+    uint64_t paddr;             /* Physical address of the module.           */
+    uint64_t size;              /* Size of the module in bytes.              */
+    uint64_t cmdline_paddr;     /* Physical address of the command line.     */
+    uint64_t reserved;
+} __attribute__((packed));
+#endif /* x86 */
+
 /* --- pluggable kernel loader ------------------------------------- */
 
 struct xc_dom_loader {
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 7b629b1..6ba060f 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -787,25 +787,46 @@  typedef struct start_info start_info_t;
 /*
  * Start of day structure passed to PVH guests in %ebx.
  *
- * NOTE: nothing will be loaded at physical address 0, so
- * a 0 value in any of the address fields should be treated
- * as not present.
+ * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
+ * of the address fields should be treated as not present.
+ *
+ *  0 +----------------+
+ *    | magic          | Contains the magic value HVM_START_MAGIC_VALUE
+ *    |                | ("xEn3" with the 0x80 bit of the "E" set).
+ *  4 +----------------+
+ *    | version        | Version of this structure. Current version is 0. New
+ *    |                | versions are guaranteed to be backwards-compatible.
+ *  8 +----------------+
+ *    | flags          | SIF_xxx flags.
+ * 12 +----------------+
+ *    | cmdline_paddr  | Physical address of the command line,
+ *    |                | a zero-terminated ASCII string.
+ * 16 +----------------+
+ *    | nr_modules     | Number of modules passed to the kernel.
+ * 20 +----------------+
+ *    | modlist_paddr  | Physical address of an array of modules
+ *    |                | (layout of the structure below).
+ * 24 +----------------+
+ *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
+ * 28 +----------------+
+ *
+ * The layout of each entry in the module structure is the following:
+ *
+ *  0 +----------------+
+ *    | paddr          | Physical address of the module.
+ *  8 +----------------+
+ *    | size           | Size of the module in bytes.
+ * 16 +----------------+
+ *    | cmdline_paddr  | Physical address of the command line,
+ *    |                | a zero-terminated ASCII string.
+ * 24 +----------------+
+ *    | reserved       |
+ * 32 +----------------+
+ *
+ * The address and size of the modules is a 64bit unsigned integer. However
+ * Xen will always try to place all modules below the 4GiB boundary.
  */
-struct hvm_start_info {
 #define HVM_START_MAGIC_VALUE 0x336ec578
-    uint32_t magic;             /* Contains the magic value 0x336ec578       */
-                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
-    uint32_t flags;             /* SIF_xxx flags.                            */
-    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
-    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
-    uint32_t modlist_paddr;     /* Physical address of an array of           */
-                                /* hvm_modlist_entry.                        */
-};
-
-struct hvm_modlist_entry {
-    uint32_t paddr;             /* Physical address of the module.           */
-    uint32_t size;              /* Size of the module in bytes.              */
-};
 
 /* New console union for dom0 introduced in 0x00030203. */
 #if __XEN_INTERFACE_VERSION__ < 0x00030203