[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 4/4] libxc: Pass e820 map to HVM/PVH guests via hvm_start_info
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> > >> > >> 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. 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); > > > >> } > >> 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? 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. 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |