[PATCH 0/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[PATCH 0/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

Here is the patch for updating the canonical definition of the
hvm_start_info struct corresponding to the discussion happening on the
linux-kernel and kvm mailing lists regarding Qemu/KVM use of the PVH
entry point:

   KVM: x86: Allow Qemu/KVM to use PVH entry point
   https://lkml.org/lkml/2018/2/28/1121

Maran Wilson (1):
      x86/PVHv2: Add memory map pointer to hvm_start_info struct

 xen/include/public/arch-x86/hvm/start_info.h | 51 +++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

The start info structure that is defined as part of the x86/HVM direct boot
ABI and used for starting Xen PVH guests would be more versatile if it also
included a way to pass information about the memory map to the guest. This
would allow KVM guests to share the same entry point.

Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
---
 xen/include/public/arch-x86/hvm/start_info.h | 51 +++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
index 6484159..ae8dac8 100644
--- a/xen/include/public/arch-x86/hvm/start_info.h
+++ b/xen/include/public/arch-x86/hvm/start_info.h
@@ -33,8 +33,9 @@
  *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
  *    |                | ("xEn3" with the 0x80 bit of the "E" set).
  *  4 +----------------+
- *    | version        | Version of this structure. Current version is 0. New
+ *    | version        | Version of this structure. Current version is 1. New
  *    |                | versions are guaranteed to be backwards-compatible.
+ *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
  *  8 +----------------+
  *    | flags          | SIF_xxx flags.
  * 12 +----------------+
@@ -48,6 +49,15 @@
  * 32 +----------------+
  *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
  * 40 +----------------+
+ *    | memmap_paddr   | Physical address of the (optional) memory map. Only
+ *    |                | present in version 1 and newer of the structure.
+ * 48 +----------------+
+ *    | memmap_entries | Number of entries in the memory map table. Only
+ *    |                | present in version 1 and newer of the structure.
+ *    |                | Zero if there is no memory map being provided.
+ * 52 +----------------+
+ *    | reserved       | Version 1 and newer only.
+ * 56 +----------------+
  *
  * The layout of each entry in the module structure is the following:
  *
@@ -62,10 +72,34 @@
  *    | reserved       |
  * 32 +----------------+
  *
+ * The layout of each entry in the memory map table is as follows:
+ *
+ *  0 +----------------+
+ *    | addr           | Base address
+ *  8 +----------------+
+ *    | size           | Size of mapping in bytes
+ * 16 +----------------+
+ *    | type           | Type of mapping as defined between the hypervisor
+ *    |                | and guest it's starting. E820_TYPE_xxx, for example.
+ * 20 +----------------|
+ *    | reserved       |
+ * 24 +----------------+
+ *
  * The address and sizes are always a 64bit little endian unsigned integer.
  *
  * NB: Xen on x86 will always try to place all the data below the 4GiB
  * boundary.
+ *
+ * Version numbers of the hvm_start_info structure have evolved like this:
+ *
+ * Version 0:
+ *
+ * Version 1:	Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
+ *		padding) to the end of the hvm_start_info struct. These new
+ *		fields can be used to pass a memory map to the guest. The
+ *		memory map is optional and so guests that understand version 1
+ *		of the structure must check that memmap_entries is non-zero
+ *		before trying to read the memory map.
  */
 #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
 
@@ -86,6 +120,14 @@ struct hvm_start_info {
     uint64_t cmdline_paddr;     /* Physical address of the command line.     */
     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
                                 /* structure.                                */
+    uint64_t memmap_paddr;	/* Physical address of an array of           */
+				/* hvm_memmap_table_entry. Only present in   */
+				/* version 1 and newer of the structure      */
+    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
+				/* Only present in version 1 and newer of    */
+				/* the structure. Value will be zero if      */
+				/* there is no memory map being provided.    */
+    uint32_t reserved;
 };
 
 struct hvm_modlist_entry {
@@ -95,4 +137,11 @@ struct hvm_modlist_entry {
     uint64_t reserved;
 };
 
+struct hvm_memmap_table_entry {
+    uint64_t addr;		/* Base address of the memory region         */
+    uint64_t size;		/* Size of the memory region in bytes        */
+    uint32_t type;		/* Mapping type                              */
+    uint32_t reserved;
+};
+
 #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Konrad Rzeszutek Wilk]

On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.

Would it be better if there was an tag/length as well? And maybe more dynamic
so that if you want to add more structures you can identify them tags?
Like what Multiboot2 has?

> 
> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> ---
>  xen/include/public/arch-x86/hvm/start_info.h | 51 +++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
> index 6484159..ae8dac8 100644
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,8 +33,9 @@
>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>   *  4 +----------------+
> - *    | version        | Version of this structure. Current version is 0. New
> + *    | version        | Version of this structure. Current version is 1. New
>   *    |                | versions are guaranteed to be backwards-compatible.
> + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
>   *  8 +----------------+
>   *    | flags          | SIF_xxx flags.
>   * 12 +----------------+
> @@ -48,6 +49,15 @@
>   * 32 +----------------+
>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>   * 40 +----------------+
> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 48 +----------------+
> + *    | memmap_entries | Number of entries in the memory map table. Only
> + *    |                | present in version 1 and newer of the structure.
> + *    |                | Zero if there is no memory map being provided.
> + * 52 +----------------+
> + *    | reserved       | Version 1 and newer only.
> + * 56 +----------------+
>   *
>   * The layout of each entry in the module structure is the following:
>   *
> @@ -62,10 +72,34 @@
>   *    | reserved       |
>   * 32 +----------------+
>   *
> + * The layout of each entry in the memory map table is as follows:
> + *
> + *  0 +----------------+
> + *    | addr           | Base address
> + *  8 +----------------+
> + *    | size           | Size of mapping in bytes
> + * 16 +----------------+
> + *    | type           | Type of mapping as defined between the hypervisor
> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
> + * 20 +----------------|
> + *    | reserved       |
> + * 24 +----------------+
> + *
>   * The address and sizes are always a 64bit little endian unsigned integer.
>   *
>   * NB: Xen on x86 will always try to place all the data below the 4GiB
>   * boundary.
> + *
> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:
> + *
> + * Version 1:	Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
> + *		padding) to the end of the hvm_start_info struct. These new
> + *		fields can be used to pass a memory map to the guest. The
> + *		memory map is optional and so guests that understand version 1
> + *		of the structure must check that memmap_entries is non-zero
> + *		before trying to read the memory map.
>   */
>  #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>  
> @@ -86,6 +120,14 @@ struct hvm_start_info {
>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>                                  /* structure.                                */
> +    uint64_t memmap_paddr;	/* Physical address of an array of           */
> +				/* hvm_memmap_table_entry. Only present in   */
> +				/* version 1 and newer of the structure      */
> +    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
> +				/* Only present in version 1 and newer of    */
> +				/* the structure. Value will be zero if      */
> +				/* there is no memory map being provided.    */
> +    uint32_t reserved;
>  };
>  
>  struct hvm_modlist_entry {
> @@ -95,4 +137,11 @@ struct hvm_modlist_entry {
>      uint64_t reserved;
>  };
>  
> +struct hvm_memmap_table_entry {
> +    uint64_t addr;		/* Base address of the memory region         */
> +    uint64_t size;		/* Size of the memory region in bytes        */
> +    uint32_t type;		/* Mapping type                              */
> +    uint32_t reserved;
> +};
> +
>  #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

On 3/2/2018 1:20 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
>> The start info structure that is defined as part of the x86/HVM direct boot
>> ABI and used for starting Xen PVH guests would be more versatile if it also
>> included a way to pass information about the memory map to the guest. This
>> would allow KVM guests to share the same entry point.
> Would it be better if there was an tag/length as well? And maybe more dynamic
> so that if you want to add more structures you can identify them tags?
> Like what Multiboot2 has?

That sounds like a decent idea if we expect this structure to continue 
to grow and expand in the future. But I'd be hesitant to make it part of 
this patch series. Mostly because it doesn't add value to the existing 
use case(s) and there's a risk we end up going down a less than ideal 
path trying to design for anticipated (but presently unknown) use cases.

I don't think the currently proposed changes would prevent us from doing 
something like you describe in the future, so I guess I'd prefer to 
leave that discussion for if/when we run into additional use cases that 
require new structures. But if there is overwhelming support for the 
idea, I can work on drafting up a proposal for what that would look like.

Thanks,
-Maran

>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
>> ---
>>   xen/include/public/arch-x86/hvm/start_info.h | 51 +++++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
>> index 6484159..ae8dac8 100644
>> --- a/xen/include/public/arch-x86/hvm/start_info.h
>> +++ b/xen/include/public/arch-x86/hvm/start_info.h
>> @@ -33,8 +33,9 @@
>>    *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>>    *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>    *  4 +----------------+
>> - *    | version        | Version of this structure. Current version is 0. New
>> + *    | version        | Version of this structure. Current version is 1. New
>>    *    |                | versions are guaranteed to be backwards-compatible.
>> + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
>>    *  8 +----------------+
>>    *    | flags          | SIF_xxx flags.
>>    * 12 +----------------+
>> @@ -48,6 +49,15 @@
>>    * 32 +----------------+
>>    *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>    * 40 +----------------+
>> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 48 +----------------+
>> + *    | memmap_entries | Number of entries in the memory map table. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + *    |                | Zero if there is no memory map being provided.
>> + * 52 +----------------+
>> + *    | reserved       | Version 1 and newer only.
>> + * 56 +----------------+
>>    *
>>    * The layout of each entry in the module structure is the following:
>>    *
>> @@ -62,10 +72,34 @@
>>    *    | reserved       |
>>    * 32 +----------------+
>>    *
>> + * The layout of each entry in the memory map table is as follows:
>> + *
>> + *  0 +----------------+
>> + *    | addr           | Base address
>> + *  8 +----------------+
>> + *    | size           | Size of mapping in bytes
>> + * 16 +----------------+
>> + *    | type           | Type of mapping as defined between the hypervisor
>> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
>> + * 20 +----------------|
>> + *    | reserved       |
>> + * 24 +----------------+
>> + *
>>    * The address and sizes are always a 64bit little endian unsigned integer.
>>    *
>>    * NB: Xen on x86 will always try to place all the data below the 4GiB
>>    * boundary.
>> + *
>> + * Version numbers of the hvm_start_info structure have evolved like this:
>> + *
>> + * Version 0:
>> + *
>> + * Version 1:	Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
>> + *		padding) to the end of the hvm_start_info struct. These new
>> + *		fields can be used to pass a memory map to the guest. The
>> + *		memory map is optional and so guests that understand version 1
>> + *		of the structure must check that memmap_entries is non-zero
>> + *		before trying to read the memory map.
>>    */
>>   #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>>   
>> @@ -86,6 +120,14 @@ struct hvm_start_info {
>>       uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>>       uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>>                                   /* structure.                                */
>> +    uint64_t memmap_paddr;	/* Physical address of an array of           */
>> +				/* hvm_memmap_table_entry. Only present in   */
>> +				/* version 1 and newer of the structure      */
>> +    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
>> +				/* Only present in version 1 and newer of    */
>> +				/* the structure. Value will be zero if      */
>> +				/* there is no memory map being provided.    */
>> +    uint32_t reserved;
>>   };
>>   
>>   struct hvm_modlist_entry {
>> @@ -95,4 +137,11 @@ struct hvm_modlist_entry {
>>       uint64_t reserved;
>>   };
>>   
>> +struct hvm_memmap_table_entry {
>> +    uint64_t addr;		/* Base address of the memory region         */
>> +    uint64_t size;		/* Size of the memory region in bytes        */
>> +    uint32_t type;		/* Mapping type                              */
>> +    uint32_t reserved;
>> +};
>> +
>>   #endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */
>> -- 
>> 1.8.3.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xenproject.org
>> https://lists.xenproject.org/mailman/listinfo/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 02.03.18 at 21:54, <maran.wilson@oracle.com> wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.

So is there no use for this with Xen at all? If so, I remain unconvinced
(and will probably defer to others). If not, I think adding the population
of the new data (under whatever conditions) should be part of this
(then) series, demonstrating that this isn't dead code.

> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:

Perhaps "Initial implementation" or some such? Leaving it completely
empty make it look a little odd as an item.

> @@ -86,6 +120,14 @@ struct hvm_start_info {
>      uint64_t cmdline_paddr;     /* Physical address of the command line.     */
>      uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI data    */
>                                  /* structure.                                */
> +    uint64_t memmap_paddr;	/* Physical address of an array of           */
> +				/* hvm_memmap_table_entry. Only present in   */
> +				/* version 1 and newer of the structure      */
> +    uint32_t memmap_entries;	/* Number of entries in the memmap table.    */
> +				/* Only present in version 1 and newer of    */
> +				/* the structure. Value will be zero if      */
> +				/* there is no memory map being provided.    */
> +    uint32_t reserved;

For both new reserved fields, please state in at least one of
formal definition and C implementation that they're required to be
set to zero in version 1. That'll open the road to assign meaning
to these fields perhaps without having to bump the version number.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Daniel Kiper]

On Fri, Mar 02, 2018 at 02:29:29PM -0800, Maran Wilson wrote:
> On 3/2/2018 1:20 PM, Konrad Rzeszutek Wilk wrote:
> >On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
> >>The start info structure that is defined as part of the x86/HVM direct boot
> >>ABI and used for starting Xen PVH guests would be more versatile if it also
> >>included a way to pass information about the memory map to the guest. This
> >>would allow KVM guests to share the same entry point.
> >Would it be better if there was an tag/length as well? And maybe more dynamic
> >so that if you want to add more structures you can identify them tags?
> >Like what Multiboot2 has?
>
> That sounds like a decent idea if we expect this structure to
> continue to grow and expand in the future. But I'd be hesitant to
> make it part of this patch series. Mostly because it doesn't add
> value to the existing use case(s) and there's a risk we end up going
> down a less than ideal path trying to design for anticipated (but
> presently unknown) use cases.
>
> I don't think the currently proposed changes would prevent us from
> doing something like you describe in the future, so I guess I'd
> prefer to leave that discussion for if/when we run into additional
> use cases that require new structures. But if there is overwhelming
> support for the idea, I can work on drafting up a proposal for what
> that would look like.

Granted! However, if you change your mind or circumstances have changed
just take a look at Multiboot2 spec. There is a chance that you can use
it as is or if it is not possible you can add what you need. Or in the
worst case you can steal the idea.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.

I would also like to see Xen modified to make use of this new
memmap_paddr feature. See bootlate_hvm in tools/libxc/xc_dom_x86.c, it
should be quite trivial to add the memmap to the hvm_start_info
crafted there.

> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
> ---
>  xen/include/public/arch-x86/hvm/start_info.h | 51 +++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
> index 6484159..ae8dac8 100644
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,8 +33,9 @@
>   *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>   *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>   *  4 +----------------+
> - *    | version        | Version of this structure. Current version is 0. New
> + *    | version        | Version of this structure. Current version is 1. New
>   *    |                | versions are guaranteed to be backwards-compatible.
> + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
>   *  8 +----------------+
>   *    | flags          | SIF_xxx flags.
>   * 12 +----------------+
> @@ -48,6 +49,15 @@
>   * 32 +----------------+
>   *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>   * 40 +----------------+
> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
> + *    |                | present in version 1 and newer of the structure.
> + * 48 +----------------+
> + *    | memmap_entries | Number of entries in the memory map table. Only
> + *    |                | present in version 1 and newer of the structure.
> + *    |                | Zero if there is no memory map being provided.

Can you place the "present in version 1 and newer" at the end of the
text block?

IMHO setting memmap_paddr to 0 should be the way to signal that
there's no memory map (like it's done for rsdp_paddr), and then the
value of _entries is irrelevant. At which point the "Zero if there is
no memory map being provided" is wrong.

> + * 52 +----------------+
> + *    | reserved       | Version 1 and newer only.
> + * 56 +----------------+
>   *
>   * The layout of each entry in the module structure is the following:
>   *
> @@ -62,10 +72,34 @@
>   *    | reserved       |
>   * 32 +----------------+
>   *
> + * The layout of each entry in the memory map table is as follows:
> + *
> + *  0 +----------------+
> + *    | addr           | Base address
> + *  8 +----------------+
> + *    | size           | Size of mapping in bytes
> + * 16 +----------------+
> + *    | type           | Type of mapping as defined between the hypervisor
> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.

This needs a link to the expected type values (or a reference). Or you
need to spell out the relation between the values and the memory types.

> + * 20 +----------------|
> + *    | reserved       |
> + * 24 +----------------+
> + *
>   * The address and sizes are always a 64bit little endian unsigned integer.
>   *
>   * NB: Xen on x86 will always try to place all the data below the 4GiB
>   * boundary.
> + *
> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:
> + *
> + * Version 1:	Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
> + *		padding) to the end of the hvm_start_info struct. These new
> + *		fields can be used to pass a memory map to the guest. The
> + *		memory map is optional and so guests that understand version 1
> + *		of the structure must check that memmap_entries is non-zero
> + *		before trying to read the memory map.

No hard tabs please.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
> On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
>> The start info structure that is defined as part of the x86/HVM direct boot
>> ABI and used for starting Xen PVH guests would be more versatile if it also
>> included a way to pass information about the memory map to the guest. This
>> would allow KVM guests to share the same entry point.
> I would also like to see Xen modified to make use of this new
> memmap_paddr feature. See bootlate_hvm in tools/libxc/xc_dom_x86.c, it
> should be quite trivial to add the memmap to the hvm_start_info
> crafted there.

Yes, that is being worked on as we speak. Should have a new set of 
patches out shortly.

>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
>> ---
>>   xen/include/public/arch-x86/hvm/start_info.h | 51 +++++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/arch-x86/hvm/start_info.h b/xen/include/public/arch-x86/hvm/start_info.h
>> index 6484159..ae8dac8 100644
>> --- a/xen/include/public/arch-x86/hvm/start_info.h
>> +++ b/xen/include/public/arch-x86/hvm/start_info.h
>> @@ -33,8 +33,9 @@
>>    *    | magic          | Contains the magic value XEN_HVM_START_MAGIC_VALUE
>>    *    |                | ("xEn3" with the 0x80 bit of the "E" set).
>>    *  4 +----------------+
>> - *    | version        | Version of this structure. Current version is 0. New
>> + *    | version        | Version of this structure. Current version is 1. New
>>    *    |                | versions are guaranteed to be backwards-compatible.
>> + *    |                | For PV guests only 0 allowed, for PVH 0 or 1 allowed.
>>    *  8 +----------------+
>>    *    | flags          | SIF_xxx flags.
>>    * 12 +----------------+
>> @@ -48,6 +49,15 @@
>>    * 32 +----------------+
>>    *    | rsdp_paddr     | Physical address of the RSDP ACPI data structure.
>>    * 40 +----------------+
>> + *    | memmap_paddr   | Physical address of the (optional) memory map. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + * 48 +----------------+
>> + *    | memmap_entries | Number of entries in the memory map table. Only
>> + *    |                | present in version 1 and newer of the structure.
>> + *    |                | Zero if there is no memory map being provided.
> Can you place the "present in version 1 and newer" at the end of the
> text block?

Sure, I'll do that.

> IMHO setting memmap_paddr to 0 should be the way to signal that
> there's no memory map (like it's done for rsdp_paddr), and then the
> value of _entries is irrelevant. At which point the "Zero if there is
> no memory map being provided" is wrong.
>
>> + * 52 +----------------+
>> + *    | reserved       | Version 1 and newer only.
>> + * 56 +----------------+
>>    *
>>    * The layout of each entry in the module structure is the following:
>>    *
>> @@ -62,10 +72,34 @@
>>    *    | reserved       |
>>    * 32 +----------------+
>>    *
>> + * The layout of each entry in the memory map table is as follows:
>> + *
>> + *  0 +----------------+
>> + *    | addr           | Base address
>> + *  8 +----------------+
>> + *    | size           | Size of mapping in bytes
>> + * 16 +----------------+
>> + *    | type           | Type of mapping as defined between the hypervisor
>> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
> This needs a link to the expected type values (or a reference). Or you
> need to spell out the relation between the values and the memory types.

This field was discussed a good deal in v2 of the linux patches. I had 
originally defined this to be a specific type field, matching the 
x86/Linux definition for e820 memory mapping types. But Jan Beulich 
successfully argued that we should keep the definition of this 
particular interface agnostic to architecture and OS and not limit the 
field to specific values. I believe the central idea behind Jan's 
argument was to keep the interface x86-agnostic as well as preserving 
the option to add additional memory mapping types in the future without 
them being sanctioned by whoever maintains E820 type assignments.

That's why I changed the comment wording to what it is now. Basically 
spelling out the fact that this field simply needs to be agreed upon 
between the producer and the consumer since a hypervisor should 
generally know what type of guest it is starting. And I mentioned 
e820_type_xxx as the *example* of one such implementation, since that is 
the most obvious use case and the e820 types are part of the ACPI 
standard (and thus easy to find/reference).

>> + * 20 +----------------|
>> + *    | reserved       |
>> + * 24 +----------------+
>> + *
>>    * The address and sizes are always a 64bit little endian unsigned integer.
>>    *
>>    * NB: Xen on x86 will always try to place all the data below the 4GiB
>>    * boundary.
>> + *
>> + * Version numbers of the hvm_start_info structure have evolved like this:
>> + *
>> + * Version 0:
>> + *
>> + * Version 1:	Added the memmap_paddr/memmap_entries fields (plus 4 bytes of
>> + *		padding) to the end of the hvm_start_info struct. These new
>> + *		fields can be used to pass a memory map to the guest. The
>> + *		memory map is optional and so guests that understand version 1
>> + *		of the structure must check that memmap_entries is non-zero
>> + *		before trying to read the memory map.
> No hard tabs please.

OK, will clean that up.

Thanks,
-Maran

>
> Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 13.03.18 at 17:20, <maran.wilson@oracle.com> wrote:
> On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
>> On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
>>> @@ -62,10 +72,34 @@
>>>    *    | reserved       |
>>>    * 32 +----------------+
>>>    *
>>> + * The layout of each entry in the memory map table is as follows:
>>> + *
>>> + *  0 +----------------+
>>> + *    | addr           | Base address
>>> + *  8 +----------------+
>>> + *    | size           | Size of mapping in bytes
>>> + * 16 +----------------+
>>> + *    | type           | Type of mapping as defined between the hypervisor
>>> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
>> This needs a link to the expected type values (or a reference). Or you
>> need to spell out the relation between the values and the memory types.
> 
> This field was discussed a good deal in v2 of the linux patches. I had 
> originally defined this to be a specific type field, matching the 
> x86/Linux definition for e820 memory mapping types. But Jan Beulich 
> successfully argued that we should keep the definition of this 
> particular interface agnostic to architecture and OS and not limit the 
> field to specific values. I believe the central idea behind Jan's 
> argument was to keep the interface x86-agnostic as well as preserving 
> the option to add additional memory mapping types in the future without 
> them being sanctioned by whoever maintains E820 type assignments.
> 
> That's why I changed the comment wording to what it is now. Basically 
> spelling out the fact that this field simply needs to be agreed upon 
> between the producer and the consumer since a hypervisor should 
> generally know what type of guest it is starting. And I mentioned 
> e820_type_xxx as the *example* of one such implementation, since that is 
> the most obvious use case and the e820 types are part of the ACPI 
> standard (and thus easy to find/reference).

But Roger makes a valid remark here. Statements like
"E820_TYPE_xxx, for example" are simply to vague for a stable public
interface.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Tue, Mar 13, 2018 at 09:20:09AM -0700, Maran Wilson wrote:
> On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
> > On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
> > > + * The layout of each entry in the memory map table is as follows:
> > > + *
> > > + *  0 +----------------+
> > > + *    | addr           | Base address
> > > + *  8 +----------------+
> > > + *    | size           | Size of mapping in bytes
> > > + * 16 +----------------+
> > > + *    | type           | Type of mapping as defined between the hypervisor
> > > + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
> > This needs a link to the expected type values (or a reference). Or you
> > need to spell out the relation between the values and the memory types.
> 
> This field was discussed a good deal in v2 of the linux patches. I had
> originally defined this to be a specific type field, matching the x86/Linux
> definition for e820 memory mapping types. But Jan Beulich successfully
> argued that we should keep the definition of this particular interface
> agnostic to architecture and OS and not limit the field to specific values.
> I believe the central idea behind Jan's argument was to keep the interface
> x86-agnostic as well as preserving the option to add additional memory
> mapping types in the future without them being sanctioned by whoever
> maintains E820 type assignments.
> 
> That's why I changed the comment wording to what it is now. Basically
> spelling out the fact that this field simply needs to be agreed upon between
> the producer and the consumer since a hypervisor should generally know what
> type of guest it is starting. And I mentioned e820_type_xxx as the *example*
> of one such implementation, since that is the most obvious use case and the
> e820 types are part of the ACPI standard (and thus easy to find/reference).

IMO we should provide a list of matching values and types here, or
else how is a consumer supposed to use this?

This is the ABI used by Xen in order to boot guests, and as such
consumers need to know how to parse the type values given in the
memory map table. The types that you define should be labeled as x86
specific, but they must be written down somewhere.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

On 3/13/2018 9:34 AM, Jan Beulich wrote:
>>>> On 13.03.18 at 17:20, <maran.wilson@oracle.com> wrote:
>> On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
>>> On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
>>>> @@ -62,10 +72,34 @@
>>>>     *    | reserved       |
>>>>     * 32 +----------------+
>>>>     *
>>>> + * The layout of each entry in the memory map table is as follows:
>>>> + *
>>>> + *  0 +----------------+
>>>> + *    | addr           | Base address
>>>> + *  8 +----------------+
>>>> + *    | size           | Size of mapping in bytes
>>>> + * 16 +----------------+
>>>> + *    | type           | Type of mapping as defined between the hypervisor
>>>> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
>>> This needs a link to the expected type values (or a reference). Or you
>>> need to spell out the relation between the values and the memory types.
>> This field was discussed a good deal in v2 of the linux patches. I had
>> originally defined this to be a specific type field, matching the
>> x86/Linux definition for e820 memory mapping types. But Jan Beulich
>> successfully argued that we should keep the definition of this
>> particular interface agnostic to architecture and OS and not limit the
>> field to specific values. I believe the central idea behind Jan's
>> argument was to keep the interface x86-agnostic as well as preserving
>> the option to add additional memory mapping types in the future without
>> them being sanctioned by whoever maintains E820 type assignments.
>>
>> That's why I changed the comment wording to what it is now. Basically
>> spelling out the fact that this field simply needs to be agreed upon
>> between the producer and the consumer since a hypervisor should
>> generally know what type of guest it is starting. And I mentioned
>> e820_type_xxx as the *example* of one such implementation, since that is
>> the most obvious use case and the e820 types are part of the ACPI
>> standard (and thus easy to find/reference).
> But Roger makes a valid remark here. Statements like
> "E820_TYPE_xxx, for example" are simply to vague for a stable public
> interface.

How about "For example, E820 types like E820_RAM, E820_ACPI, etc as 
defined in xen/include/asm-x86/e820.h of the Xen tree" ?

Thanks,
-Maran

>
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Roger Pau Monné]

On Tue, Mar 13, 2018 at 09:55:20AM -0700, Maran Wilson wrote:
> On 3/13/2018 9:34 AM, Jan Beulich wrote:
> > > > > On 13.03.18 at 17:20, <maran.wilson@oracle.com> wrote:
> > > On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
> > > > On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
> > > > > @@ -62,10 +72,34 @@
> > > > >     *    | reserved       |
> > > > >     * 32 +----------------+
> > > > >     *
> > > > > + * The layout of each entry in the memory map table is as follows:
> > > > > + *
> > > > > + *  0 +----------------+
> > > > > + *    | addr           | Base address
> > > > > + *  8 +----------------+
> > > > > + *    | size           | Size of mapping in bytes
> > > > > + * 16 +----------------+
> > > > > + *    | type           | Type of mapping as defined between the hypervisor
> > > > > + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
> > > > This needs a link to the expected type values (or a reference). Or you
> > > > need to spell out the relation between the values and the memory types.
> > > This field was discussed a good deal in v2 of the linux patches. I had
> > > originally defined this to be a specific type field, matching the
> > > x86/Linux definition for e820 memory mapping types. But Jan Beulich
> > > successfully argued that we should keep the definition of this
> > > particular interface agnostic to architecture and OS and not limit the
> > > field to specific values. I believe the central idea behind Jan's
> > > argument was to keep the interface x86-agnostic as well as preserving
> > > the option to add additional memory mapping types in the future without
> > > them being sanctioned by whoever maintains E820 type assignments.
> > > 
> > > That's why I changed the comment wording to what it is now. Basically
> > > spelling out the fact that this field simply needs to be agreed upon
> > > between the producer and the consumer since a hypervisor should
> > > generally know what type of guest it is starting. And I mentioned
> > > e820_type_xxx as the *example* of one such implementation, since that is
> > > the most obvious use case and the e820 types are part of the ACPI
> > > standard (and thus easy to find/reference).
> > But Roger makes a valid remark here. Statements like
> > "E820_TYPE_xxx, for example" are simply to vague for a stable public
> > interface.
> 
> How about "For example, E820 types like E820_RAM, E820_ACPI, etc as defined
> in xen/include/asm-x86/e820.h of the Xen tree" ?

No, it needs to be in a public header, e820.h is private to Xen.

I would recommend that you list the types in this header, specifying
that the 'type' values are arch-specific, and that this is the x86
specific interface. You likely also want to reference the section of
the ACPI spec where those types are defined, so that the reader can
figure out it's exact meaning.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson]

On 3/13/2018 10:16 AM, Roger Pau Monné wrote:
> On Tue, Mar 13, 2018 at 09:55:20AM -0700, Maran Wilson wrote:
>> On 3/13/2018 9:34 AM, Jan Beulich wrote:
>>>>>> On 13.03.18 at 17:20, <maran.wilson@oracle.com> wrote:
>>>> On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
>>>>> On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
>>>>>> @@ -62,10 +72,34 @@
>>>>>>      *    | reserved       |
>>>>>>      * 32 +----------------+
>>>>>>      *
>>>>>> + * The layout of each entry in the memory map table is as follows:
>>>>>> + *
>>>>>> + *  0 +----------------+
>>>>>> + *    | addr           | Base address
>>>>>> + *  8 +----------------+
>>>>>> + *    | size           | Size of mapping in bytes
>>>>>> + * 16 +----------------+
>>>>>> + *    | type           | Type of mapping as defined between the hypervisor
>>>>>> + *    |                | and guest it's starting. E820_TYPE_xxx, for example.
>>>>> This needs a link to the expected type values (or a reference). Or you
>>>>> need to spell out the relation between the values and the memory types.
>>>> This field was discussed a good deal in v2 of the linux patches. I had
>>>> originally defined this to be a specific type field, matching the
>>>> x86/Linux definition for e820 memory mapping types. But Jan Beulich
>>>> successfully argued that we should keep the definition of this
>>>> particular interface agnostic to architecture and OS and not limit the
>>>> field to specific values. I believe the central idea behind Jan's
>>>> argument was to keep the interface x86-agnostic as well as preserving
>>>> the option to add additional memory mapping types in the future without
>>>> them being sanctioned by whoever maintains E820 type assignments.
>>>>
>>>> That's why I changed the comment wording to what it is now. Basically
>>>> spelling out the fact that this field simply needs to be agreed upon
>>>> between the producer and the consumer since a hypervisor should
>>>> generally know what type of guest it is starting. And I mentioned
>>>> e820_type_xxx as the *example* of one such implementation, since that is
>>>> the most obvious use case and the e820 types are part of the ACPI
>>>> standard (and thus easy to find/reference).
>>> But Roger makes a valid remark here. Statements like
>>> "E820_TYPE_xxx, for example" are simply to vague for a stable public
>>> interface.
>> How about "For example, E820 types like E820_RAM, E820_ACPI, etc as defined
>> in xen/include/asm-x86/e820.h of the Xen tree" ?
> No, it needs to be in a public header, e820.h is private to Xen.
>
> I would recommend that you list the types in this header, specifying
> that the 'type' values are arch-specific, and that this is the x86
> specific interface.

Can I provide that list in a comment block? Or are you saying you want 
me to create new #define values in this header file to enumerate the 
possible range of "type" values for x86 guests?

I'd prefer to avoid the latter since I would be redefining values that 
most certainly are already defined in every source tree where this 
header file is likely to show up. But if folks feel it is necessary, 
I'll add the symbols here.

> You likely also want to reference the section of
> the ACPI spec where those types are defined, so that the reader can
> figure out it's exact meaning.

Sure, I can add that. I'm thinking something like:

    For x86 guests, please see "Address Range Types" as defined in 
section 15 (System Address Map Interfaces) of the ACPI Specification 
(http://uefi.org/specifications)

Thanks,
-Maran

>
> Thanks, Roger.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 13.03.18 at 17:55, <maran.wilson@oracle.com> wrote:
> On 3/13/2018 9:34 AM, Jan Beulich wrote:
>>>>> On 13.03.18 at 17:20, <maran.wilson@oracle.com> wrote:
>>> On 3/13/2018 3:50 AM, Roger Pau Monné wrote:
>>>> On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
>>>>> @@ -62,10 +72,34 @@
>>>>>     *    | reserved       |
>>>>>     * 32 +----------------+
>>>>>     *
>>>>> + * The layout of each entry in the memory map table is as follows:
>>>>> + *
>>>>> + *  0 +----------------+
>>>>> + *    | addr           | Base address
>>>>> + *  8 +----------------+
>>>>> + *    | size           | Size of mapping in bytes
>>>>> + * 16 +----------------+
>>>>> + *    | type           | Type of mapping as defined between the hypervisor
>>>>> + *    |                | and guest it's starting. E820_TYPE_xxx, for 
> example.
>>>> This needs a link to the expected type values (or a reference). Or you
>>>> need to spell out the relation between the values and the memory types.
>>> This field was discussed a good deal in v2 of the linux patches. I had
>>> originally defined this to be a specific type field, matching the
>>> x86/Linux definition for e820 memory mapping types. But Jan Beulich
>>> successfully argued that we should keep the definition of this
>>> particular interface agnostic to architecture and OS and not limit the
>>> field to specific values. I believe the central idea behind Jan's
>>> argument was to keep the interface x86-agnostic as well as preserving
>>> the option to add additional memory mapping types in the future without
>>> them being sanctioned by whoever maintains E820 type assignments.
>>>
>>> That's why I changed the comment wording to what it is now. Basically
>>> spelling out the fact that this field simply needs to be agreed upon
>>> between the producer and the consumer since a hypervisor should
>>> generally know what type of guest it is starting. And I mentioned
>>> e820_type_xxx as the *example* of one such implementation, since that is
>>> the most obvious use case and the e820 types are part of the ACPI
>>> standard (and thus easy to find/reference).
>> But Roger makes a valid remark here. Statements like
>> "E820_TYPE_xxx, for example" are simply to vague for a stable public
>> interface.
> 
> How about "For example, E820 types like E820_RAM, E820_ACPI, etc as 
> defined in xen/include/asm-x86/e820.h of the Xen tree" ?

No, that's still "for example". You need to spell out (in the abstract
part) and provide C constants (in the C implementation part) for
the types currently permitted. Their 1:1 relationship with E820_*
constants could/should then be documented with a couple of
BUILD_BUG_ON()s.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel