Re: [Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

2018-03-21 Thread Roger Pau Monné 
On Wed, Mar 21, 2018 at 09:46:21AM -0700, Maran Wilson wrote:
> On 3/21/2018 2:40 AM, Juergen Gross wrote:
> > On 21/03/18 10:28, Roger Pau Monné wrote:
> > > On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
> > > > +/*
> > > >* C representation of the x86/HVM start info layout.
> > > >*
> > > >* The canonical definition of this layout is above, this is just a 
> > > > way to
> > > > @@ -86,6 +134,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;  /* Must be zero for Version 1. 
> > > >   */
> > > I would write "Must be zero." only. If at some point we introduce
> > > version 2 we would likely have to fixup this comment to mention
> > > version 1 and version 2.
> > In case you are going this route I'd suggest to drop the version remarks
> > for the individual fields and just add a comment like:
> > 
> > /* All following fields only present in version 1 and newer. */
> > 
> > above memmap_paddr.
> 
> OK, so combining the above suggestions, I'd have the following. Is the
> formatting and alignment of comments what you had in mind and acceptable to
> all?

It seems like your email client has skewed the formatting (or maybe
mine...)

Anyway, LGTM, I think this is better.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

2018-03-21 Thread Juergen Gross 
On 21/03/18 17:46, Maran Wilson wrote:
> On 3/21/2018 2:40 AM, Juergen Gross wrote:
>> On 21/03/18 10:28, Roger Pau Monné wrote:
>>> On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
 +/*
    * C representation of the x86/HVM start info layout.
    *
    * The canonical definition of this layout is above, this is just
 a way to
 @@ -86,6 +134,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;  /* Must be zero for Version
 1.   */
>>> I would write "Must be zero." only. If at some point we introduce
>>> version 2 we would likely have to fixup this comment to mention
>>> version 1 and version 2.
>> In case you are going this route I'd suggest to drop the version remarks
>> for the individual fields and just add a comment like:
>>
>> /* All following fields only present in version 1 and newer. */
>>
>> above memmap_paddr.
> 
> OK, so combining the above suggestions, I'd have the following. Is the
> formatting and alignment of comments what you had in mind and acceptable
> to all?

Looks good to me.


Juergen

> 
> 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 nr_modules;    /* Number of modules passed to the
> kernel.   */
>     uint64_t modlist_paddr; /* Physical address of an array
> of   */
>     /*
> hvm_modlist_entry.    */
>     uint64_t cmdline_paddr; /* Physical address of the command
> line. */
>     uint64_t rsdp_paddr;    /* Physical address of the RSDP ACPI
> data    */
>     /*
> structure.    */
>     /* All following fields only present in version 1 and newer */
>     uint64_t memmap_paddr;  /* Physical address of an array
> of   */
>     /*
> hvm_memmap_table_entry.   */
>     uint32_t memmap_entries;    /* Number of entries in the memmap
> table.    */
>     /* Value will be zero if there is no
> memory  */
>     /* map being
> provided.   */
>     uint32_t reserved;  /* Must be
> zero. */
> };
> 
> Thanks,
> -Maran
> 
> 


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

Re: [Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

2018-03-21 Thread Juergen Gross 
On 21/03/18 10:28, Roger Pau Monné wrote:
> On Tue, Mar 20, 2018 at 09:48:56AM -0700, 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.
>>
>> Signed-off-by: Maran Wilson 
> 
> Reviewed-by: Roger Pau Monné 
> 
> Just a couple of nit suggestions...
> 
>> ---
>>  xen/include/public/arch-x86/hvm/start_info.h | 65 
>> +++-
>>  1 file changed, 64 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..d491f2d 100644
>> --- a/xen/include/public/arch-x86/hvm/start_info.h
>> +++ b/xen/include/public/arch-x86/hvm/start_info.h
>> @@ -33,7 +33,7 @@
>>   *| 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.
>>   *  8 ++
>>   *| flags  | SIF_xxx flags.
>> @@ -48,6 +48,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. Zero
>> + *|| if there is no memory map being provided. Only
>> + *|| present in version 1 and newer of the structure.
>> + * 52 ++
>> + *| reserved   | Version 1 and newer only.
>> + * 56 ++
>>   *
>>   * The layout of each entry in the module structure is the following:
>>   *
>> @@ -62,14 +71,53 @@
>>   *| 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. See XEN_HVM_MEMMAP_TYPE_*
> 
> I would remove "it's starting" here.
> 
>> + *|| values below.
>> + * 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:  Initial implementation.
>> + *
>> + * 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
>>  
>>  /*
>> + * The values used in the type field of the memory map table entries are
>> + * defined below and match the Address Range Types as defined in the "System
>> + * Address Map Interfaces" section of the ACPI Specification. Please refer 
>> to
>> + * section 15 in version 6.2 of the ACPI spec: 
>> http://uefi.org/specifications
>> + */
>> +#define XEN_HVM_MEMMAP_TYPE_RAM   1
>> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
>> +#define XEN_HVM_MEMMAP_TYPE_ACPI  3
>> +#define XEN_HVM_MEMMAP_TYPE_NVS   4
>> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
>> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
>> +#define XEN_HVM_MEMMAP_TYPE_PMEM  7
>> +
>> +/*
>>   * C representation of the x86/HVM start info layout.
>>   *
>>   * The canonical definition of this layout is above, this is just a way to
>> @@ -86,6 +134,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

Re: [Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

2018-03-21 Thread Roger Pau Monné 
On Tue, Mar 20, 2018 at 09:48:56AM -0700, 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.
> 
> Signed-off-by: Maran Wilson 

Reviewed-by: Roger Pau Monné 

Just a couple of nit suggestions...

> ---
>  xen/include/public/arch-x86/hvm/start_info.h | 65 
> +++-
>  1 file changed, 64 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..d491f2d 100644
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,7 +33,7 @@
>   *| 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.
>   *  8 ++
>   *| flags  | SIF_xxx flags.
> @@ -48,6 +48,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. Zero
> + *|| if there is no memory map being provided. Only
> + *|| present in version 1 and newer of the structure.
> + * 52 ++
> + *| reserved   | Version 1 and newer only.
> + * 56 ++
>   *
>   * The layout of each entry in the module structure is the following:
>   *
> @@ -62,14 +71,53 @@
>   *| 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. See XEN_HVM_MEMMAP_TYPE_*

I would remove "it's starting" here.

> + *|| values below.
> + * 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:  Initial implementation.
> + *
> + * 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
>  
>  /*
> + * The values used in the type field of the memory map table entries are
> + * defined below and match the Address Range Types as defined in the "System
> + * Address Map Interfaces" section of the ACPI Specification. Please refer to
> + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
> + */
> +#define XEN_HVM_MEMMAP_TYPE_RAM   1
> +#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
> +#define XEN_HVM_MEMMAP_TYPE_ACPI  3
> +#define XEN_HVM_MEMMAP_TYPE_NVS   4
> +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
> +#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
> +#define XEN_HVM_MEMMAP_TYPE_PMEM  7
> +
> +/*
>   * C representation of the x86/HVM start info layout.
>   *
>   * The canonical definition of this layout is above, this is just a way to
> @@ -86,6 +134,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  
>

Re: [Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

2018-03-20 Thread Jan Beulich 
>>> On 20.03.18 at 17:48,  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.
> 
> Signed-off-by: Maran Wilson 

Once again
Acked-by: Jan Beulich 

But I'd like Roger to confirm his concerns have all been dealt with.

Jan


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

[Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

2018-03-20 Thread 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 
---
 xen/include/public/arch-x86/hvm/start_info.h | 65 +++-
 1 file changed, 64 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..d491f2d 100644
--- a/xen/include/public/arch-x86/hvm/start_info.h
+++ b/xen/include/public/arch-x86/hvm/start_info.h
@@ -33,7 +33,7 @@
  *| 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.
  *  8 ++
  *| flags  | SIF_xxx flags.
@@ -48,6 +48,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. Zero
+ *|| if there is no memory map being provided. Only
+ *|| present in version 1 and newer of the structure.
+ * 52 ++
+ *| reserved   | Version 1 and newer only.
+ * 56 ++
  *
  * The layout of each entry in the module structure is the following:
  *
@@ -62,14 +71,53 @@
  *| 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. See XEN_HVM_MEMMAP_TYPE_*
+ *|| values below.
+ * 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:  Initial implementation.
+ *
+ * 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
 
 /*
+ * The values used in the type field of the memory map table entries are
+ * defined below and match the Address Range Types as defined in the "System
+ * Address Map Interfaces" section of the ACPI Specification. Please refer to
+ * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications
+ */
+#define XEN_HVM_MEMMAP_TYPE_RAM   1
+#define XEN_HVM_MEMMAP_TYPE_RESERVED  2
+#define XEN_HVM_MEMMAP_TYPE_ACPI  3
+#define XEN_HVM_MEMMAP_TYPE_NVS   4
+#define XEN_HVM_MEMMAP_TYPE_UNUSABLE  5
+#define XEN_HVM_MEMMAP_TYPE_DISABLED  6
+#define XEN_HVM_MEMMAP_TYPE_PMEM  7
+
+/*
  * C representation of the x86/HVM start info layout.
  *
  * The canonical definition of this layout is above, this is just a way to
@@ -86,6 +134,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;  /* Must be zero for Version 1.   */
 };