|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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 <maran.wilson@xxxxxxxxxx>
>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> 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
>> */
>> + 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.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |