Re: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
>>> On 13.03.18 at 17:55, wrote: > On 3/13/2018 9:34 AM, Jan Beulich wrote: > On 13.03.18 at 17:20, 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
Re: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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, 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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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, 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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
On 3/13/2018 9:34 AM, Jan Beulich wrote: On 13.03.18 at 17:20, 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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
>>> On 13.03.18 at 17:20, 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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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 --- 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. T
Re: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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 > --- > 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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
>>> On 02.03.18 at 21:54, 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: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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 --- 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.*/
Re: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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 > --- > 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
[Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
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 | 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