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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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@oracle.com>
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Signed-off-by: Maran Wilson <maran.wilson@oracle.com>
>>>> ---
>>>>  tools/libxc/xc_dom_x86.c | 29 ++++++++++++++++++++++++++++-
>>>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>>>> index 0b65dab..b2d8403 100644
>>>> --- a/tools/libxc/xc_dom_x86.c
>>>> +++ b/tools/libxc/xc_dom_x86.c
>>>> @@ -35,6 +35,8 @@
>>>>  #include <xen/arch-x86/hvm/start_info.h>
>>>>  #include <xen/io/protocols.h>
>>>>  
>>>> +#include <xen-tools/libs.h>
>>>> +
>>>>  #include "xg_private.h"
>>>>  #include "xc_dom.h"
>>>>  #include "xenctrl.h"
>>>> @@ -640,6 +642,8 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>>>              dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>>>>              start_info_size += dom->cmdline_size;
>>>>          }
>>>> +
>>>> +        start_info_size += dom->e820_entries * sizeof(*(dom->e820));
>>> This is not correct because sizeof(struct e820entry) != sizeof(struct
>>> hvm_modlist_entry) AFAICT. This should instead be sizeof(struct
>>> hvm_modlist_entry).
>>
>> The area for modlist is calculated above:
>>
>> start_info_size +=
>>         sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
>>
>> (What I should do though is move this from under 'if (
>> !dom->device_model )', now that we are providing this data to both HVM
>> and PVH guests.
> Sorry, I meant sizeof(struct hvm_memmap_table_entry) != sizeof(e820entry), so
> the above should be:
>
> start_info_size += dom->e820_entries * sizeof(struct hvm_memmap_table_entry);


Oh, duh!

>
>>>>      }
>>>>      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.)


>
> I think those are all previous issues in this code, TBH.
>
>>> Also the initial space calculation doesn't seem to take
>>> HVMLOADER_MODULE_CMDLINE_SIZE into account at all.
>>
>> modlist's offset is calculated with commandline's size in mind:
>>
>>     modlist = (void*)(start_info + 1) + dom->cmdline_size;
> Yes, that's fine AFAICT.
>
>>> And cmdline_paddr seems to be set to point to garbage if cmdline is not
>>> set.
>> Isn't the hvm_start_info set to zero when allocated?
> Yes, but the following code in add_module_to_list seems wrong to me:
>
> if ( cmdline )
> {
>     assert(strnlen(cmdline, HVMLOADER_MODULE_CMDLINE_SIZE)
>            < HVMLOADER_MODULE_CMDLINE_SIZE);
>     strncpy(modules_cmdline_start + HVMLOADER_MODULE_CMDLINE_SIZE * index,
>             cmdline, HVMLOADER_MODULE_CMDLINE_SIZE);
> }
>
> modlist[index].cmdline_paddr =
>     modules_cmdline_paddr + HVMLOADER_MODULE_CMDLINE_SIZE * index;
>
> AFAICT it will set cmdline_paddr to point to garbage if cmdline is not
> set.


Oh, I thought you were referring to start_info->cmdline_paddr. Yes, you
are right for the modules' pointer.

-boris

>
> Anyway, this is not introduced by your patch. I will send a couple of
> patches to try to fix the already existing issues in this area.
>
> Thanks, Roger.


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