Re: [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Maran Wilson <maran.wilson@oracle.com>]

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, <maran.wilson@oracle.com> 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