Re: [Xen-devel] [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Jun 23, 2016 at 10:44:26AM -0400, Boris Ostrovsky wrote:
> On 06/22/2016 01:15 PM, Anthony PERARD wrote:
> > +/*
> > + * The memory layout of the start_info page and the modules, and where the
> > + * addresses are stored:
> > + *
> > + * /----------------------------------\
> > + * | struct hvm_start_info            |
> > + * +----------------------------------+ <- start_info->modlist_paddr
> > + * | struct hvm_modlist_entry[0]      |
> > + * +----------------------------------+
> > + * | struct hvm_modlist_entry[1]      |
> > + * +----------------------------------+ <- modlist[0].cmdline_paddr
> > + * | cmdline of module 0              |
> > + * | char[HVMLOADER_MODULE_NAME_SIZE] |
> > + * +----------------------------------+ <- modlist[1].cmdline_paddr
> > + * | cmdline of module 1              |
> > + * +----------------------------------+
> > + */
> 
> Should this go to public/xen.h?

No, it should not. This is to describe how the memory is allocated
and used by this function. The different calculation may be a bit
complicated to follow.

> > +static void add_module_to_list(struct xc_dom_image *dom,
> > +                               struct xc_hvm_firmware_module *module,
> > +                               const char *name,
> > +                               struct hvm_modlist_entry *modlist,
> > +                               struct hvm_start_info *start_info)
> > +{
> > +    uint32_t index = start_info->nr_modules;
> > +    void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;
> > +    uint64_t modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) +
> > +        ((uintptr_t)modlist - (uintptr_t)start_info);
> > +    uint64_t modules_cmdline_paddr = modlist_paddr +
> > +        sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> > +
> > +    if ( module->length == 0 )
> > +        return;
> > +
> > +    assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT);
> > +    assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE)
> > +           < HVMLOADER_MODULE_NAME_SIZE);
> > +
> > +    modlist[index].paddr = module->guest_addr_out;
> > +    modlist[index].size = module->length;
> > +
> > +    strncpy(modules_cmdline_start + HVMLOADER_MODULE_NAME_SIZE * index,
> > +            name, HVMLOADER_MODULE_NAME_SIZE);
> > +    modlist[index].cmdline_paddr =
> > +        modules_cmdline_paddr + HVMLOADER_MODULE_NAME_SIZE * index;
> > +
> > +    start_info->nr_modules++;
> > +}
> > +
> >  static int bootlate_hvm(struct xc_dom_image *dom)
> >  {
> >      uint32_t domid = dom->guest_domid;
> >      xc_interface *xch = dom->xch;
> > +    struct hvm_start_info *start_info;
> > +    size_t start_info_size;
> > +    struct hvm_modlist_entry *modlist;
> >  
> > -    if ( !dom->device_model )
> > -    {
> > -        struct hvm_start_info *start_info;
> > -        size_t start_info_size;
> > -        void *start_page;
> > -
> > -        start_info_size = sizeof(*start_info) + dom->cmdline_size;
> > -        if ( dom->ramdisk_blob )
> > -            start_info_size += sizeof(struct hvm_modlist_entry);
> > +    start_info_size = sizeof(*start_info) + dom->cmdline_size;
> > +    if ( dom->ramdisk_blob )
> > +        start_info_size += sizeof(struct hvm_modlist_entry);
> >  
> > -        if ( start_info_size >
> > -             dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> > -        {
> > -            DOMPRINTF("Trying to map beyond start_info_seg");
> > -            return -1;
> > -        }
> > +    if ( start_info_size >
> > +         dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> > +    {
> > +        DOMPRINTF("Trying to map beyond start_info_seg");
> > +        return -1;
> > +    }
> >  
> > -        start_page = xc_map_foreign_range(xch, domid, start_info_size,
> > -                                          PROT_READ | PROT_WRITE,
> > -                                          dom->start_info_seg.pfn);
> > -        if ( start_page == NULL )
> > -        {
> > -            DOMPRINTF("Unable to map HVM start info page");
> > -            return -1;
> > -        }
> > +    start_info = xc_map_foreign_range(xch, domid, start_info_size,
> > +                                      PROT_READ | PROT_WRITE,
> > +                                      dom->start_info_seg.pfn);
> > +    if ( start_info == NULL )
> > +    {
> > +        DOMPRINTF("Unable to map HVM start info page");
> > +        return -1;
> > +    }
> >  
> > -        start_info = start_page;
> > +    modlist = (void*)(start_info + 1) + dom->cmdline_size;
> >  
> > +    if ( !dom->device_model )
> > +    {
> >          if ( dom->cmdline )
> >          {
> > -            char *cmdline = start_page + sizeof(*start_info);
> > +            char *cmdline = (void*)(start_info + 1);
> >  
> >              strncpy(cmdline, dom->cmdline, dom->cmdline_size);
> >              start_info->cmdline_paddr = (dom->start_info_seg.pfn << 
> > PAGE_SHIFT) +
> > @@ -1733,22 +1791,30 @@ static int bootlate_hvm(struct xc_dom_image *dom)
> >  
> >          if ( dom->ramdisk_blob )
> >          {
> > -            struct hvm_modlist_entry *modlist =
> > -                start_page + sizeof(*start_info) + dom->cmdline_size;
> >  
> >              modlist[0].paddr = dom->ramdisk_seg.vstart - 
> > dom->parms.virt_base;
> >              modlist[0].size = dom->ramdisk_seg.vend - 
> > dom->ramdisk_seg.vstart;
> > -            start_info->modlist_paddr = (dom->start_info_seg.pfn << 
> > PAGE_SHIFT) +
> > -                                ((uintptr_t)modlist - 
> > (uintptr_t)start_info);
> >              start_info->nr_modules = 1;
> >          }
> > -
> > -        start_info->magic = XEN_HVM_START_MAGIC_VALUE;
> > -
> > -        munmap(start_page, start_info_size);
> >      }
> >      else
> >      {
> > +        add_module_to_list(dom, &dom->system_firmware_module, "firmware",
> > +                           modlist, start_info);
> > +    }
> 
> Is it possible to add PVH's ramdisk via this routine as well?

I guest that could be possible with some change to add_module_to_list,
or with maybe with two different function, one that takes
xc_hvm_firmware_module and another that takes xc_dom_seg. I think I'll
leave this refactoring for another day.

-- 
Anthony PERARD

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