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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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>
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> Signed-off-by: Maran Wilson <maran.wilson@xxxxxxxxxx>
>> ---
>>  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.

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


>
> 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;



>
> 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?

-boris

>
> Or am I missing something?
>
> Adding Jonathan Ludlam and Anthony PERARD who are the ones that added
> this code.
>
> Thanks, Roger.


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