|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct
On 3/16/2018 4:11 AM, Roger Pau Monné wrote: On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote: 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. OnlyAgain (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 { _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |