[PATCH] tools: ROUNDUP() related adjustments

[PATCH] tools: ROUNDUP() related adjustments

[Jan Beulich]

For one xc_private.h needlessly repeats xen-tools/libs.h's definition.

And then there are two suspicious uses (resulting from the inconsistency
with the respective 2nd parameter of DIV_ROUNDUP()): While the one in
tools/console/daemon/io.c - as per the code comment - intentionally uses
8 as the second argument (meaning to align to a multiple of 256), the
one in alloc_magic_pages_hvm() pretty certainly does not: There the goal
is to align to a uint64_t boundary, for the following module struct to
end up aligned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xg_dom_x86.c's HVM guest command line handling has further oddities: The
command line gets copied twice, yet in only one case enforcing the
MAX_GUEST_CMDLINE upper bound on the length. The length calculation also
doesn't take this bound into account, despite the assumption that all of
start info fits into a single page. A terminating nul character gets
forced in place in only one of the two cases, too.

--- a/tools/libs/ctrl/xc_private.h
+++ b/tools/libs/ctrl/xc_private.h
@@ -63,8 +63,6 @@ struct iovec {
 #include <sys/uio.h>
 #endif
 
-#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
-
 #define DECLARE_DOMCTL struct xen_domctl domctl
 #define DECLARE_SYSCTL struct xen_sysctl sysctl
 #define DECLARE_PHYSDEV_OP struct physdev_op physdev_op
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -678,7 +678,7 @@ static int alloc_magic_pages_hvm(struct
     {
         if ( dom->cmdline )
         {
-            dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
+            dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 3);
             start_info_size += dom->cmdline_size;
         }
     }

Re: [PATCH] tools: ROUNDUP() related adjustments

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 699 bytes --]

On 10.08.21 12:03, Jan Beulich wrote:
> For one xc_private.h needlessly repeats xen-tools/libs.h's definition.
> 
> And then there are two suspicious uses (resulting from the inconsistency
> with the respective 2nd parameter of DIV_ROUNDUP()): While the one in
> tools/console/daemon/io.c - as per the code comment - intentionally uses
> 8 as the second argument (meaning to align to a multiple of 256), the
> one in alloc_magic_pages_hvm() pretty certainly does not: There the goal
> is to align to a uint64_t boundary, for the following module struct to
> end up aligned.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] tools: ROUNDUP() related adjustments

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH] tools: ROUNDUP() related adjustments"):
> On 10.08.21 12:03, Jan Beulich wrote:
> > For one xc_private.h needlessly repeats xen-tools/libs.h's definition.
> > 
> > And then there are two suspicious uses (resulting from the inconsistency
> > with the respective 2nd parameter of DIV_ROUNDUP()): While the one in
> > tools/console/daemon/io.c - as per the code comment - intentionally uses
> > 8 as the second argument (meaning to align to a multiple of 256), the
> > one in alloc_magic_pages_hvm() pretty certainly does not: There the goal
> > is to align to a uint64_t boundary, for the following module struct to
> > end up aligned.

This is really quite unpleasnt, to my mind.  ROUNDUP taking a bit
length is bad enough, but the magic knowledge about alignment is
really poor too.  It may not be right on all future architectures,
although I think your changae is correct on all the ones we support
(or which people are working on).

So IOW I think your change is correct and warranted, but I really
dislike the code here.

Therefore:

Acked-by: Ian Jackson <iwj@xenproject.org>

> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks for the review!

Ian.