Re: [Xen-devel] [PATCH v3 07/16] hvmloader: Grab the hvm_start_info pointer

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.02.16 at 15:56, <anthony.perard@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/tools/firmware/hvmloader/hvm_start_info.h
> @@ -0,0 +1,32 @@
> +#ifndef __HVM_START_INFO_H__
> +#define __HVM_START_INFO_H__
> +
> +/* C representation of the x86/HVM start info layout.
> + *
> + * The canonical definition of this layout resides in public/xen.h, 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.
> + */
> +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 /* __HVM_START_INFO_H__ */

With there already being such a C representation in libxc, can't we
make sure we have only one instance that will need adjustment upon
future enhancements? (Perhaps it would still have been a good idea
to expose such a C representation in public/xen.h.)

> @@ -258,6 +263,7 @@ int main(void)
>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
>  
>      printf("HVM Loader\n");
> +    BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE);

Wait - public/xen.h clearly says this is being handed for PVH guests
only. How can you even de-reference the pointer unconditionally?

Jan


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