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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.18 at 17:55, <maran.wilson@xxxxxxxxxx> wrote:
> On 3/13/2018 9:34 AM, Jan Beulich wrote:
>>>>> On 13.03.18 at 17:20, <maran.wilson@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel