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

[Juergen Gross <jgross@xxxxxxxx>]

On 21/03/18 17:46, Maran Wilson wrote:
> On 3/21/2018 2:40 AM, Juergen Gross wrote:
>> On 21/03/18 10:28, Roger Pau Monné wrote:
>>> On Tue, Mar 20, 2018 at 09:48:56AM -0700, Maran Wilson wrote:
>>>> +/*
>>>>    * 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 +134,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.               */
>>> I would write "Must be zero." only. If at some point we introduce
>>> version 2 we would likely have to fixup this comment to mention
>>> version 1 and version 2.
>> In case you are going this route I'd suggest to drop the version remarks
>> for the individual fields and just add a comment like:
>>
>> /* All following fields only present in version 1 and newer. */
>>
>> above memmap_paddr.
> 
> OK, so combining the above suggestions, I'd have the following. Is the
> formatting and alignment of comments what you had in mind and acceptable
> to all?

Looks good to me.


Juergen

> 
> struct hvm_start_info {
>     uint32_t magic;             /* Contains the magic value
> 0x336ec578       */
>                                 /* ("xEn3" with the 0x80 bit of the "E"
> set).*/
>     uint32_t version;           /* Version of this
> structure.                */
>     uint32_t flags;             /* SIF_xxx
> flags.                            */
>     uint32_t nr_modules;        /* Number of modules passed to the
> kernel.   */
>     uint64_t modlist_paddr;     /* Physical address of an array
> of           */
>                                 /*
> hvm_modlist_entry.                        */
>     uint64_t cmdline_paddr;     /* Physical address of the command
> line.     */
>     uint64_t rsdp_paddr;        /* Physical address of the RSDP ACPI
> data    */
>                                 /*
> structure.                                */
>     /* All following fields only present in version 1 and newer */
>     uint64_t memmap_paddr;      /* Physical address of an array
> of           */
>                                 /*
> hvm_memmap_table_entry.                   */
>     uint32_t memmap_entries;    /* Number of entries in the memmap
> table.    */
>                                 /* Value will be zero if there is no
> memory  */
>                                 /* map being
> provided.                       */
>     uint32_t reserved;          /* Must be
> zero.                             */
> };
> 
> Thanks,
> -Maran
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel