Re: [Xen-devel] [PATCH v4 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

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

Hello,

On Tue, 15 Mar 2016, Jan Beulich wrote:

> >>> On 14.03.16 at 18:55, <anthony.perard@xxxxxxxxxx> wrote:
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -841,6 +841,37 @@ typedef struct start_info start_info_t;
> >   */
> >  #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
> >  
> > +#if defined(__i386__) || defined(__x86_64__)
> > +/* C representation of the x86/HVM start info layout.
> > + *
> > + * The canonical definition of this layout is abrove, this is just a way to
> > + * represent the layout described there using C types.
> > + *
> > + * NB: the packed attribute is not really needed, but it helps us enforce
> > + * the fact this this is just a representation, and it might indeed
> > + * be required in the future if there are alignment changes.
> > + */

        ^ Rationale on why the packed attribute was added.

> > +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 cmdline_paddr;     /* Physical address of the command line.   
> >   */
> > +    uint32_t nr_modules;        /* Number of modules passed to the kernel. 
> >   */
> > +    uint32_t modlist_paddr;     /* Physical address of an array of         
> >   */
> > +                                /* hvm_modlist_entry.                      
> >   */
> > +    uint32_t rsdp_paddr;        /* Physical address of the RSDP ACPI data  
> >   */
> > +                                /* structure.                              
> >   */
> > +} __attribute__((packed));
> > +
> > +struct hvm_modlist_entry {
> > +    uint64_t paddr;             /* Physical address of the module.         
> >   */
> > +    uint64_t size;              /* Size of the module in bytes.            
> >   */
> > +    uint64_t cmdline_paddr;     /* Physical address of the command line.   
> >   */
> > +    uint64_t reserved;
> > +} __attribute__((packed));
> > +#endif /* x86 */
> 
> This needs extra care: Either the packed attributes need to be
> dropped (Roger - why did they get put there in the first place?),
> or their use needs to be shielded from compilers not understanding
> them.

Hello,

The rationale behind the packed attribute is in the highlighted comment 
above.

I would really like to avoid placing this in public headers, or else 
people will think this is the definition of the payload and will forget 
that this is just a C representation of it, but the definition is in a 
comment just above. I want to avoid the issues we have already seen by the 
usage of C structures as definitions of the placement of payloads in 
memory.

If this really has to be there, please guard it with:

#if defined(__XEN__) || defined(__XEN_TOOLS__)

So only the Xen kernel/tools can use it.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel