On 3/16/2018 4:11 AM, Roger Pau Monné wrote:
On Thu, Mar 15, 2018 at 02:33:09PM -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.wil...@oracle.com>
---
xen/include/public/arch-x86/hvm/start_info.h | 66 +++++++++++++++++++++++++++-
1 file changed, 65 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..f2e8ba6 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.
Why are you adding the above sentence? PV guest never used or will use
the hvm_start_info structure (note the hvm_ prefix).
Thanks for the feed back Roger,
As you noticed, my first version did not contain that comment. Konrad
suggested adding that particular line (in reply to the Linux tree
version of this patch) and no one seemed to object at the time so I went
ahead and added it.
Konrad, do you care to weigh in here?
* 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. Zero
+ * | | if there is no memory map being provided. Only
Again (as I've mentioned in previous reviews) the way to signal a
non-present memory map is to set memmap_paddr to 0, not memmap_entries
to 0. This is already covered by the comment at the top of the header,
which states:
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.
I'm not really following the argument for why checking for PA != 0 is a
better approach. The way I see it, we have to check #entries anyway so
the consumer side code is more efficient if we just check #entries and
assume the PA is valid when #entries != 0 (seems like a reasonable
demand for the producer side). If we define it to be "PA of zero means
no valid entries", then the consumer side code has to make the
additional check of PA != 0 in addition to making sure there are greater
than 0 entries. It's not a huge deal, but then again, I'm not seeing a
huge win by going the other way either.
The fact that a previous comment already exists regarding PA of 0 is
*slightly* awkward (redundant) depending on how you look at it. But it's
not blatantly contradictory (in most cases, I'm sure PA and #entries
will both be zero when no memory map is being provided) and it probably
serves a purpose for the rsdp_paddr field.
So after carefully reading everyone's input on this thread, my
preference as the person coding this up is to stick with the #entries
check and leave the other existing comments in place to cover the
existing fields and code that is already out there.
But if it's really a deal breaker for someone or if broad consensus is
that it is better to just make the change so that PA of 0 is the
definitive way to check for the presence of a memory map, then I will go
ahead and make the change.
+ * | | 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 +72,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.
Same again, the guest will have to check memmap_paddr != 0, not
memmap_entries, like it's done for other fields that contain a
physical address.
*/
#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 +135,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. */
As mentioned in previous reviews: no tabs please.
I'm embarrassed. I'll fix that and make sure the rest of the file is
using soft tabs everywhere I have touched.
My problem was switching between the Xen tree and other source trees
where tabs are required :-)
Thanks,
-Maran
};
struct hvm_modlist_entry {
@@ -95,4 +152,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; /* Must be zero for Version 1. */
+};
No tabs please.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
