Re: [Xen-devel] [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info

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

On Wed, Mar 21, 2018 at 01:53:37PM -0400, Boris Ostrovsky wrote:
> On 03/21/2018 10:18 AM, Roger Pau Monné wrote:
> > On Wed, Mar 21, 2018 at 09:37:09AM -0400, Boris Ostrovsky wrote:
> >> On 03/21/2018 06:07 AM, Roger Pau Monné wrote:
> >>> On Tue, Mar 20, 2018 at 09:50:52AM -0700, Maran Wilson wrote:
> >>>> From: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> >>>>      }
> >>>>      else
> >>>>      {
> >>>> @@ -1666,8 +1670,9 @@ 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;
> >>>> +    size_t start_info_size, modsize;
> >>>>      struct hvm_modlist_entry *modlist;
> >>>> +    struct hvm_memmap_table_entry *memmap;
> >>>>      unsigned int i;
> >>>>  
> >>>>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
> >>>> @@ -1731,7 +1736,29 @@ static int bootlate_hvm(struct xc_dom_image *dom)
> >>>>                              ((uintptr_t)modlist - 
> >>>> (uintptr_t)start_info);
> >>>>      }
> >>>>  
> >>>> +    /*
> >>>> +     * Check a couple of XEN_HVM_MEMMAP_TYPEs to verify consistency with
> >>>> +     * their corresponding e820 numerical values.
> >>>> +     */
> >>>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_RAM != E820_RAM);
> >>>> +    BUILD_BUG_ON(XEN_HVM_MEMMAP_TYPE_ACPI != E820_ACPI);
> >>>> +
> >>>> +    modsize = HVMLOADER_MODULE_MAX_COUNT *
> >>>> +        (sizeof(*modlist) + HVMLOADER_MODULE_CMDLINE_SIZE);
> >>> Hm, I'm not sure this is fully correct, but I think there are previous
> >>> issues in this area.
> >>>
> >>> The mapped area (start_info) is of size sizeof(*start_info) +
> >>> dom->cmdline_size + sizeof(struct hvm_modlist_entry) *
> >>> dom->num_modules. Yet here you seem to assume num_modules ==
> >>> HVMLOADER_MODULE_MAX_COUNT?
> >> Yes, see my response above. We've already allocated the segment to
> >> accommodate HVMLOADER_MODULE_MAX_COUNT entries. Which may indeed be an
> >> overkill.
> > I'm sorry, but I don't think I follow. There's only a single
> > xc_map_foreign_range call that maps start_info_size space:
> >
> > start_info_size = sizeof(*start_info) + dom->cmdline_size;
> > start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules;
> >
> > So for start_info_size bootlate_hvm takes into account the exact
> > number of modules used.
> >
> > Yet modsize seems to assume dom->num_modules ==
> > HVMLOADER_MODULE_MAX_COUNT?
> 
> 
> If you look at add_module_to_list() above you'll notice that it stores
> modules' commandlines after HVMLOADER_MODULE_MAX_COUNT modules:
> 
>     void *modules_cmdline_start = modlist + HVMLOADER_MODULE_MAX_COUNT;
> 
> 
> One thing I could do is
> 
>     modsize = HVMLOADER_MODULE_MAX_COUNT *(sizeof(*modlist)) + 
> dom->num_modules * HVMLOADER_MODULE_CMDLINE_SIZE;
> 
> but I think the resulting difference between expected/reserved number of
> modules vs number of commandlines makes this not worthwhile.
> 
> (As a side note, dom->num_modules is meaningless for HVM guests here ---
> we only add one module, the FW blob.)

Right. I think that after the fixes I've sent for the start_info_size
your calculation is correct:

https://lists.xenproject.org/archives/html/xen-devel/2018-03/msg02493.html

Roger.

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