[PATCH] x86/HVM: rewrite the start info structure definition in binary form

[PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Roger Pau Monne]

This will prevent alignments from getting in the way. It's not safe to
define this memory structures using C anyway, since the ABI depends on the
bitness, while our protocol does not.

Also add a command line parameter to each module, and a reserved field in
order to have the layout aligned. Note that the current implementation in
libxc doesn't make use of the module command line at all.

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 | 28 ++++++++++++++++++++++++++++
 xen/include/public/xen.h     | 42 ++++++++++++++++++++++++++++--------------
 2 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index cac4698..e5ab56c 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -216,6 +216,34 @@ 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 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.                        */
+} __attribute__((packed));
+
+struct hvm_modlist_entry {
+    uint32_t paddr;             /* Physical address of the module.           */
+    uint32_t size;              /* Size of the module in bytes.              */
+    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
+    uint32_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..e1350d0 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -790,22 +790,36 @@ typedef struct start_info start_info_t;
  * 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 +----------------+
+ *    | flags          | SIF_xxx flags.
+ *  8 +----------------+
+ *    | cmdline_paddr  | Physical address of the command line,
+ *    |                | a zero-terminated ASCII string.
+ * 12 +----------------+
+ *    | nr_modules     | Number of modules passed to the kernel.
+ * 16 +----------------+
+ *    | modlist_paddr  | Physical address of an array of modules
+ *    |                | (layout of the structure below).
+ * 20 +----------------+
+ *
+ * The layout of each entry in the module structure is the following:
+ *
+ *  0 +----------------+
+ *    | paddr          | Physical address of the module.
+ *  4 +----------------+
+ *    | size           | Size of the module in bytes.
+ *  8 +----------------+
+ *    | cmdline_paddr  | Physical address of the command line,
+ *    |                | a zero-terminated ASCII string.
+ * 12 +----------------+
+ *    | reserved       |
+ * 16 +----------------+
  */
-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

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Jan Beulich]

>>> On 05.02.16 at 13:28, <roger.pau@citrix.com> wrote:
> This will prevent alignments from getting in the way. It's not safe to
> define this memory structures using C anyway, since the ABI depends on the
> bitness, while our protocol does not.
> 
> Also add a command line parameter to each module, and a reserved field in
> order to have the layout aligned. Note that the current implementation in
> libxc doesn't make use of the module command line at all.

Which would seem wrong then - what use is the field if it doesn't
get filled? Or is that because it has nowhere to come from? But
even then - wouldn't what I've read on the other thread mean
at least the filename should be put there (as kind of the first
command line element)?

Jan

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Roger Pau Monné]

El 5/2/16 a les 14:13, Jan Beulich ha escrit:
>>>> On 05.02.16 at 13:28, <roger.pau@citrix.com> wrote:
>> This will prevent alignments from getting in the way. It's not safe to
>> define this memory structures using C anyway, since the ABI depends on the
>> bitness, while our protocol does not.
>>
>> Also add a command line parameter to each module, and a reserved field in
>> order to have the layout aligned. Note that the current implementation in
>> libxc doesn't make use of the module command line at all.
> 
> Which would seem wrong then - what use is the field if it doesn't
> get filled? Or is that because it has nowhere to come from?

Right now it has nowhere to come from as you say.

Once we enable this ABI for Dom0 boot we are just going to copy what's
in the "string" field defined in the multiboot spec for each module
structure that's passed to Dom0. That's the primary use I can see for
this ATM.

It was requested by Samuel, and I think it's fine to add it to the spec,
even if we are not going to use it right now. Samuel requires something
like this in order to properly pass parameters to boot modules in gnumach.

> But
> even then - wouldn't what I've read on the other thread mean
> at least the filename should be put there (as kind of the first
> command line element)?

Really? I didn't get that impression at all, what's the filename useful
for anyway?

IMHO this would need plumbing through libxl/xl in order to be able to
specify command line arguments for modules, which is something that we
don't support at the moment.

Roger.

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Jan Beulich]

>>> On 05.02.16 at 16:45, <roger.pau@citrix.com> wrote:
> El 5/2/16 a les 14:13, Jan Beulich ha escrit:
>> But
>> even then - wouldn't what I've read on the other thread mean
>> at least the filename should be put there (as kind of the first
>> command line element)?
> 
> Really? I didn't get that impression at all, what's the filename useful
> for anyway?

This largely depends on whether you mean to mimic multiboot1 or
multiboot2. Remember the placeholder people have to add to
certain grub2 invocation lines? I think that's a result of their
attempt to had through a file name. But indeed - I've never
understood what it's good for until this request came about: How
else would the consumer know which module is which if there's no
signature or alike inside the file? Iirc someone suggested (in the
context of the discussion here) that Linux might look for "initrd"
in the filename to identify that one. Seems fragile to me, but may
be the only alternative if not demanding multiple modules to be
ordered in a certain way.

Jan

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Samuel Thibault]

Jan Beulich, on Tue 09 Feb 2016 01:14:13 -0700, wrote:
> >>> On 05.02.16 at 16:45, <roger.pau@citrix.com> wrote:
> > El 5/2/16 a les 14:13, Jan Beulich ha escrit:
> >> But
> >> even then - wouldn't what I've read on the other thread mean
> >> at least the filename should be put there (as kind of the first
> >> command line element)?
> > 
> > Really? I didn't get that impression at all, what's the filename useful
> > for anyway?
> 
> This largely depends on whether you mean to mimic multiboot1 or
> multiboot2. Remember the placeholder people have to add to
> certain grub2 invocation lines? I think that's a result of their
> attempt to had through a file name.

Yes. For gnumach we've been adding the file name by hand in the command
line. Otherwise the resulting tasks would have at best their first
parameter as task name, since gnumach has no idea what these modules...

Samuel

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Roger Pau Monné]

El 9/2/16 a les 9:14, Jan Beulich ha escrit:
>>>> On 05.02.16 at 16:45, <roger.pau@citrix.com> wrote:
>> El 5/2/16 a les 14:13, Jan Beulich ha escrit:
>>> But
>>> even then - wouldn't what I've read on the other thread mean
>>> at least the filename should be put there (as kind of the first
>>> command line element)?
>>
>> Really? I didn't get that impression at all, what's the filename useful
>> for anyway?
> 
> This largely depends on whether you mean to mimic multiboot1 or
> multiboot2. Remember the placeholder people have to add to
> certain grub2 invocation lines? I think that's a result of their
> attempt to had through a file name. But indeed - I've never
> understood what it's good for until this request came about: How
> else would the consumer know which module is which if there's no
> signature or alike inside the file? Iirc someone suggested (in the
> context of the discussion here) that Linux might look for "initrd"
> in the filename to identify that one. Seems fragile to me, but may
> be the only alternative if not demanding multiple modules to be
> ordered in a certain way.

OK, right now xl/libxl only allows passing one module that's considered
the initram/ramdisk from Linux PoV. Other OSes that use the pv loader
don't pass any module at all AFAIK. The problem is that there's no way
to specify a command line parameter for modules, and I think in order to
make use of this we should add such option, instead of hard coding
"initrd" as the command line of the first module.

However, I don't think any of this should prevent this patch from being
accepted. If someone has interest in implementing a way to pass module
command line arguments please feel free to do it, this patch just sets
the right ABI for doing so, although it's unused at the moment.

Roger.

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Samuel Thibault]

Roger Pau Monné, on Tue 09 Feb 2016 11:38:43 +0100, wrote:
> Other OSes that use the pv loader don't pass any module at all AFAIK.

GNU Mach does need several modules.

Samuel

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Roger Pau Monné]

El 9/2/16 a les 11:41, Samuel Thibault ha escrit:
> Roger Pau Monné, on Tue 09 Feb 2016 11:38:43 +0100, wrote:
>> Other OSes that use the pv loader don't pass any module at all AFAIK.
> 
> GNU Mach does need several modules.

How do you usually pass multiple modules from an xl configuration file
at the moment?

Roger.

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Samuel Thibault]

Roger Pau Monné, on Tue 09 Feb 2016 11:45:01 +0100, wrote:
> El 9/2/16 a les 11:41, Samuel Thibault ha escrit:
> > Roger Pau Monné, on Tue 09 Feb 2016 11:38:43 +0100, wrote:
> >> Other OSes that use the pv loader don't pass any module at all AFAIK.
> > 
> > GNU Mach does need several modules.
> 
> How do you usually pass multiple modules from an xl configuration file
> at the moment?

We pack the modules and command lines in one big blob, which we really
don't like.

Or we just use pv-grub / pv-grub2.

Samuel

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Roger Pau Monné]

El 9/2/16 a les 11:49, Samuel Thibault ha escrit:
> Roger Pau Monné, on Tue 09 Feb 2016 11:45:01 +0100, wrote:
>> El 9/2/16 a les 11:41, Samuel Thibault ha escrit:
>>> Roger Pau Monné, on Tue 09 Feb 2016 11:38:43 +0100, wrote:
>>>> Other OSes that use the pv loader don't pass any module at all AFAIK.
>>>
>>> GNU Mach does need several modules.
>>
>> How do you usually pass multiple modules from an xl configuration file
>> at the moment?
> 
> We pack the modules and command lines in one big blob, which we really
> don't like.

Right, this should allow you to pass multiple modules, but someone needs
to implement the xl/libxl/libxc side in order to do it. As said, I don't
think this should block it from being accepted.

Roger.

Re: [PATCH] x86/HVM: rewrite the start info structure definition in binary form

[Andrew Cooper]

On 09/02/16 10:56, Roger Pau Monné wrote:
> El 9/2/16 a les 11:49, Samuel Thibault ha escrit:
>> Roger Pau Monné, on Tue 09 Feb 2016 11:45:01 +0100, wrote:
>>> El 9/2/16 a les 11:41, Samuel Thibault ha escrit:
>>>> Roger Pau Monné, on Tue 09 Feb 2016 11:38:43 +0100, wrote:
>>>>> Other OSes that use the pv loader don't pass any module at all AFAIK.
>>>> GNU Mach does need several modules.
>>> How do you usually pass multiple modules from an xl configuration file
>>> at the moment?
>> We pack the modules and command lines in one big blob, which we really
>> don't like.
> Right, this should allow you to pass multiple modules, but someone needs
> to implement the xl/libxl/libxc side in order to do it. As said, I don't
> think this should block it from being accepted.

+1 on both counts.

~Andrew