* [PATCH for-4.11 0/2] libxc/x86: fix a couple of bugs @ 2018-03-21 14:42 Roger Pau Monne 2018-03-21 14:42 ` [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area Roger Pau Monne 2018-03-21 14:42 ` [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address Roger Pau Monne 0 siblings, 2 replies; 9+ messages in thread From: Roger Pau Monne @ 2018-03-21 14:42 UTC (permalink / raw) To: xen-devel; +Cc: Roger Pau Monne Hello, Following two patches fix two bugs related to the mapping and handling of the hvm_start_info. Thanks, Roger. Roger Pau Monne (2): libxc/x86: fix mapping of the start_info area libxc/x86: do not unconditionally set the module cmdline address tools/libxc/xc_dom_x86.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) -- 2.16.2 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area 2018-03-21 14:42 [PATCH for-4.11 0/2] libxc/x86: fix a couple of bugs Roger Pau Monne @ 2018-03-21 14:42 ` Roger Pau Monne 2018-03-21 18:09 ` Wei Liu 2018-03-21 14:42 ` [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address Roger Pau Monne 1 sibling, 1 reply; 9+ messages in thread From: Roger Pau Monne @ 2018-03-21 14:42 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Ian Jackson, Roger Pau Monne The start_info size calculated in bootlate_hvm is wrong. It should use HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't take into account the size of the modules command line. This is not a problem so far because the actually used amount of memory doesn't cross a page boundary, and so no page-fault is triggered. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libxc/xc_dom_x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index 0b65dab4bc..e29c666b89 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -1671,7 +1671,11 @@ static int bootlate_hvm(struct xc_dom_image *dom) unsigned int i; start_info_size = sizeof(*start_info) + dom->cmdline_size; - start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules; + start_info_size += sizeof(struct hvm_modlist_entry) * + HVMLOADER_MODULE_MAX_COUNT; + start_info_size += HVMLOADER_MODULE_CMDLINE_SIZE * + HVMLOADER_MODULE_MAX_COUNT; + if ( start_info_size > dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) -- 2.16.2 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area 2018-03-21 14:42 ` [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area Roger Pau Monne @ 2018-03-21 18:09 ` Wei Liu 2018-03-22 9:10 ` Roger Pau Monné 0 siblings, 1 reply; 9+ messages in thread From: Wei Liu @ 2018-03-21 18:09 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ian Jackson, Wei Liu On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote: > The start_info size calculated in bootlate_hvm is wrong. It should use > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't > take into account the size of the modules command line. > > This is not a problem so far because the actually used amount of > memory doesn't cross a page boundary, and so no page-fault is > triggered. I get the cmdline bit. What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just map what we need here? Wei. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxc/xc_dom_x86.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > index 0b65dab4bc..e29c666b89 100644 > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -1671,7 +1671,11 @@ static int bootlate_hvm(struct xc_dom_image *dom) > unsigned int i; > > start_info_size = sizeof(*start_info) + dom->cmdline_size; > - start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules; > + start_info_size += sizeof(struct hvm_modlist_entry) * > + HVMLOADER_MODULE_MAX_COUNT; > + start_info_size += HVMLOADER_MODULE_CMDLINE_SIZE * > + HVMLOADER_MODULE_MAX_COUNT; > + > > if ( start_info_size > > dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) > -- > 2.16.2 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area 2018-03-21 18:09 ` Wei Liu @ 2018-03-22 9:10 ` Roger Pau Monné 2018-03-28 7:48 ` Roger Pau Monné 2018-03-28 11:08 ` Wei Liu 0 siblings, 2 replies; 9+ messages in thread From: Roger Pau Monné @ 2018-03-22 9:10 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote: > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote: > > The start_info size calculated in bootlate_hvm is wrong. It should use > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't > > take into account the size of the modules command line. > > > > This is not a problem so far because the actually used amount of > > memory doesn't cross a page boundary, and so no page-fault is > > triggered. > > I get the cmdline bit. > > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just > map what we need here? Because the position of the modules command line is: modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; (This is from add_module_to_list). So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region is smaller that what we might end up using. I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating memory (in alloc_magic_pages_hvm) instead of the actual number of modules (dom->num_modules), but the proposed change seems to be the easier way to fix the mapping issue. I've CC'ed the original authors of this code in another thread, but got no response. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area 2018-03-22 9:10 ` Roger Pau Monné @ 2018-03-28 7:48 ` Roger Pau Monné 2018-03-28 11:08 ` Wei Liu 1 sibling, 0 replies; 9+ messages in thread From: Roger Pau Monné @ 2018-03-28 7:48 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Ian Jackson Ping? On Thu, Mar 22, 2018 at 09:10:03AM +0000, Roger Pau Monné wrote: > On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote: > > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote: > > > The start_info size calculated in bootlate_hvm is wrong. It should use > > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't > > > take into account the size of the modules command line. > > > > > > This is not a problem so far because the actually used amount of > > > memory doesn't cross a page boundary, and so no page-fault is > > > triggered. > > > > I get the cmdline bit. > > > > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just > > map what we need here? > > Because the position of the modules command line is: > > modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; > > (This is from add_module_to_list). > > So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region > is smaller that what we might end up using. > > I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating > memory (in alloc_magic_pages_hvm) instead of the actual number of > modules (dom->num_modules), but the proposed change seems to be the > easier way to fix the mapping issue. > > I've CC'ed the original authors of this code in another thread, but > got no response. > > Thanks, Roger. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area 2018-03-22 9:10 ` Roger Pau Monné 2018-03-28 7:48 ` Roger Pau Monné @ 2018-03-28 11:08 ` Wei Liu 2018-03-28 11:29 ` Roger Pau Monné 1 sibling, 1 reply; 9+ messages in thread From: Wei Liu @ 2018-03-28 11:08 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Ian Jackson On Thu, Mar 22, 2018 at 09:10:03AM +0000, Roger Pau Monné wrote: > On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote: > > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote: > > > The start_info size calculated in bootlate_hvm is wrong. It should use > > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't > > > take into account the size of the modules command line. > > > > > > This is not a problem so far because the actually used amount of > > > memory doesn't cross a page boundary, and so no page-fault is > > > triggered. > > > > I get the cmdline bit. > > > > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just > > map what we need here? > > Because the position of the modules command line is: > > modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; > > (This is from add_module_to_list). > > So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region > is smaller that what we might end up using. > > I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating > memory (in alloc_magic_pages_hvm) instead of the actual number of > modules (dom->num_modules), but the proposed change seems to be the > easier way to fix the mapping issue. > This patch is correct, in the sense that it replicates the logic from alloc_magic_pages_hvm to bootlate_hvm. However, I don't think bootlate_hvm is in the business of calculating the size once more. This is bound to fail in the future. Instead, you can stash the size to dom once the calculation in alloc_magic_pages_hvm is done, and then use it in bootlate_hvm. This is the least fragile way I can think of. Does this make sense? Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area 2018-03-28 11:08 ` Wei Liu @ 2018-03-28 11:29 ` Roger Pau Monné 0 siblings, 0 replies; 9+ messages in thread From: Roger Pau Monné @ 2018-03-28 11:29 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Ian Jackson On Wed, Mar 28, 2018 at 12:08:07PM +0100, Wei Liu wrote: > On Thu, Mar 22, 2018 at 09:10:03AM +0000, Roger Pau Monné wrote: > > On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote: > > > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote: > > > > The start_info size calculated in bootlate_hvm is wrong. It should use > > > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't > > > > take into account the size of the modules command line. > > > > > > > > This is not a problem so far because the actually used amount of > > > > memory doesn't cross a page boundary, and so no page-fault is > > > > triggered. > > > > > > I get the cmdline bit. > > > > > > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just > > > map what we need here? > > > > Because the position of the modules command line is: > > > > modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; > > > > (This is from add_module_to_list). > > > > So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region > > is smaller that what we might end up using. > > > > I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating > > memory (in alloc_magic_pages_hvm) instead of the actual number of > > modules (dom->num_modules), but the proposed change seems to be the > > easier way to fix the mapping issue. > > > > This patch is correct, in the sense that it replicates the logic from > alloc_magic_pages_hvm to bootlate_hvm. However, I don't think > bootlate_hvm is in the business of calculating the size once more. This > is bound to fail in the future. Agree, the calculation now is fairly simple, yet we have already failed to replicate it properly. > Instead, you can stash the size to dom once the calculation in > alloc_magic_pages_hvm is done, and then use it in bootlate_hvm. This is > the least fragile way I can think of. Ack, I think this is correct, and more robust future-wise. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address 2018-03-21 14:42 [PATCH for-4.11 0/2] libxc/x86: fix a couple of bugs Roger Pau Monne 2018-03-21 14:42 ` [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area Roger Pau Monne @ 2018-03-21 14:42 ` Roger Pau Monne 2018-03-21 18:11 ` Wei Liu 1 sibling, 1 reply; 9+ messages in thread From: Roger Pau Monne @ 2018-03-21 14:42 UTC (permalink / raw) To: xen-devel; +Cc: Wei Liu, Ian Jackson, Roger Pau Monne This will lead to writing a wrong module command line physical memory address if no command line is actually provided. This hasn't caused problems so far because hvmloader is the only consumer of the modules command line, and it's unconditionally set in that case. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libxc/xc_dom_x86.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index e29c666b89..27dad89906 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -1653,11 +1653,10 @@ static void add_module_to_list(struct xc_dom_image *dom, < 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; } - modlist[index].cmdline_paddr = - modules_cmdline_paddr + HVMLOADER_MODULE_CMDLINE_SIZE * index; - start_info->nr_modules++; } -- 2.16.2 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address 2018-03-21 14:42 ` [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address Roger Pau Monne @ 2018-03-21 18:11 ` Wei Liu 0 siblings, 0 replies; 9+ messages in thread From: Wei Liu @ 2018-03-21 18:11 UTC (permalink / raw) To: Roger Pau Monne; +Cc: xen-devel, Ian Jackson, Wei Liu On Wed, Mar 21, 2018 at 02:42:11PM +0000, Roger Pau Monne wrote: > This will lead to writing a wrong module command line physical memory > address if no command line is actually provided. > > This hasn't caused problems so far because hvmloader is the only > consumer of the modules command line, and it's unconditionally set > in that case. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-03-28 11:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-21 14:42 [PATCH for-4.11 0/2] libxc/x86: fix a couple of bugs Roger Pau Monne 2018-03-21 14:42 ` [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area Roger Pau Monne 2018-03-21 18:09 ` Wei Liu 2018-03-22 9:10 ` Roger Pau Monné 2018-03-28 7:48 ` Roger Pau Monné 2018-03-28 11:08 ` Wei Liu 2018-03-28 11:29 ` Roger Pau Monné 2018-03-21 14:42 ` [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address Roger Pau Monne 2018-03-21 18:11 ` Wei Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.