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

[Maran Wilson <maran.wilson@xxxxxxxxxx>]

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?
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