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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 21, 2018 at 09:46:21AM -0700, 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?

It seems like your email client has skewed the formatting (or maybe
mine...)

Anyway, LGTM, I think this is better.

Thanks, Roger.

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