| Message ID | 677805f1-c44c-ef65-8190-c4de3bdb327d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: build adjustments | expand |
On 07/08/2020 12:33, Jan Beulich wrote: > --- a/xen/common/bitmap.c > +++ b/xen/common/bitmap.c > @@ -384,3 +386,87 @@ void bitmap_byte_to_long(unsigned long * > } > > #endif > + > +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, > + const unsigned long *bitmap, unsigned int nbits) > +{ > + unsigned int guest_bytes, copy_bytes, i; > + uint8_t zero = 0; > + int err = 0; > + uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8); > + > + if ( !bytemap ) > + return -ENOMEM; > + > + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; > + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); > + > + bitmap_long_to_byte(bytemap, bitmap, nbits); > + > + if ( copy_bytes != 0 ) > + if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) ) > + err = -EFAULT; > + > + for ( i = copy_bytes; !err && i < guest_bytes; i++ ) > + if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) ) > + err = -EFAULT; > + > + xfree(bytemap); > + > + return err; > +} > + > +int xenctl_bitmap_to_bitmap(unsigned long *bitmap, > + const struct xenctl_bitmap *xenctl_bitmap, > + unsigned int nbits) > +{ > + unsigned int guest_bytes, copy_bytes; > + int err = 0; > + uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8); > + > + if ( !bytemap ) > + return -ENOMEM; > + > + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; > + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); > + > + if ( copy_bytes != 0 ) > + { > + if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) ) > + err = -EFAULT; > + if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) ) > + bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 7)); > + } > + > + if ( !err ) > + bitmap_byte_to_long(bitmap, bytemap, nbits); > + > + xfree(bytemap); > + > + return err; > +} > + > +int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap, > + const cpumask_t *cpumask) > +{ > + return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask), > + nr_cpu_ids); > +} > + > +int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask, > + const struct xenctl_bitmap *xenctl_cpumap) > +{ > + int err = 0; > + > + if ( alloc_cpumask_var(cpumask) ) { At a minimum, please fix this style during the move. However, the more I look at this code, the more concerned I get. There is absolutely no need to allocate(/doubly allocate) memory or double/triple bounce buffer the data. All that is necessary is some careful handling of the copy length, and trailing zeroing. The cpumask variants should be trivial static inline wrapper. The fact that they're not suggests an API error. In reality, these are just data-shuffling helpers, and would probably live better in a guest-access.c if we had a suitable one to hand, but I guess bitmap.c will have to do for now. ~Andrew
Hi, On 07/08/2020 12:33, Jan Beulich wrote: > A subsequent change will exclude domctl.c from getting built for a > particular configuration, yet the two functions get used from elsewhere. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers, > --- > v2: Move function decls to xen/bitmap.h. > > --- a/xen/common/bitmap.c > +++ b/xen/common/bitmap.c > @@ -9,6 +9,8 @@ > #include <xen/errno.h> > #include <xen/bitmap.h> > #include <xen/bitops.h> > +#include <xen/cpumask.h> > +#include <xen/guest_access.h> > #include <asm/byteorder.h> > > /* > @@ -384,3 +386,87 @@ void bitmap_byte_to_long(unsigned long * > } > > #endif > + > +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, > + const unsigned long *bitmap, unsigned int nbits) > +{ > + unsigned int guest_bytes, copy_bytes, i; > + uint8_t zero = 0; > + int err = 0; > + uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8); > + > + if ( !bytemap ) > + return -ENOMEM; > + > + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; > + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); > + > + bitmap_long_to_byte(bytemap, bitmap, nbits); > + > + if ( copy_bytes != 0 ) > + if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) ) > + err = -EFAULT; > + > + for ( i = copy_bytes; !err && i < guest_bytes; i++ ) > + if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) ) > + err = -EFAULT; > + > + xfree(bytemap); > + > + return err; > +} > + > +int xenctl_bitmap_to_bitmap(unsigned long *bitmap, > + const struct xenctl_bitmap *xenctl_bitmap, > + unsigned int nbits) > +{ > + unsigned int guest_bytes, copy_bytes; > + int err = 0; > + uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8); > + > + if ( !bytemap ) > + return -ENOMEM; > + > + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; > + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); > + > + if ( copy_bytes != 0 ) > + { > + if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) ) > + err = -EFAULT; > + if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) ) > + bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 7)); > + } > + > + if ( !err ) > + bitmap_byte_to_long(bitmap, bytemap, nbits); > + > + xfree(bytemap); > + > + return err; > +} > + > +int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap, > + const cpumask_t *cpumask) > +{ > + return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask), > + nr_cpu_ids); > +} > + > +int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask, > + const struct xenctl_bitmap *xenctl_cpumap) > +{ > + int err = 0; > + > + if ( alloc_cpumask_var(cpumask) ) { > + err = xenctl_bitmap_to_bitmap(cpumask_bits(*cpumask), xenctl_cpumap, > + nr_cpu_ids); > + /* In case of error, cleanup is up to us, as the caller won't care! */ > + if ( err ) > + free_cpumask_var(*cpumask); > + } > + else > + err = -ENOMEM; > + > + return err; > +} > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -34,91 +34,6 @@ > > static DEFINE_SPINLOCK(domctl_lock); > > -static int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, > - const unsigned long *bitmap, > - unsigned int nbits) > -{ > - unsigned int guest_bytes, copy_bytes, i; > - uint8_t zero = 0; > - int err = 0; > - uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8); > - > - if ( !bytemap ) > - return -ENOMEM; > - > - guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; > - copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); > - > - bitmap_long_to_byte(bytemap, bitmap, nbits); > - > - if ( copy_bytes != 0 ) > - if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) ) > - err = -EFAULT; > - > - for ( i = copy_bytes; !err && i < guest_bytes; i++ ) > - if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) ) > - err = -EFAULT; > - > - xfree(bytemap); > - > - return err; > -} > - > -int xenctl_bitmap_to_bitmap(unsigned long *bitmap, > - const struct xenctl_bitmap *xenctl_bitmap, > - unsigned int nbits) > -{ > - unsigned int guest_bytes, copy_bytes; > - int err = 0; > - uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8); > - > - if ( !bytemap ) > - return -ENOMEM; > - > - guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; > - copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); > - > - if ( copy_bytes != 0 ) > - { > - if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) ) > - err = -EFAULT; > - if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) ) > - bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 7)); > - } > - > - if ( !err ) > - bitmap_byte_to_long(bitmap, bytemap, nbits); > - > - xfree(bytemap); > - > - return err; > -} > - > -int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap, > - const cpumask_t *cpumask) > -{ > - return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask), > - nr_cpu_ids); > -} > - > -int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask, > - const struct xenctl_bitmap *xenctl_cpumap) > -{ > - int err = 0; > - > - if ( alloc_cpumask_var(cpumask) ) { > - err = xenctl_bitmap_to_bitmap(cpumask_bits(*cpumask), xenctl_cpumap, > - nr_cpu_ids); > - /* In case of error, cleanup is up to us, as the caller won't care! */ > - if ( err ) > - free_cpumask_var(*cpumask); > - } > - else > - err = -ENOMEM; > - > - return err; > -} > - > static int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap, > const nodemask_t *nodemask) > { > --- a/xen/include/xen/bitmap.h > +++ b/xen/include/xen/bitmap.h > @@ -273,6 +273,13 @@ static inline void bitmap_clear(unsigned > void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp, int nbits); > void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits); > > +struct xenctl_bitmap; > +int xenctl_bitmap_to_bitmap(unsigned long *bitmap, > + const struct xenctl_bitmap *xenctl_bitmap, > + unsigned int nbits); > +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, > + const unsigned long *bitmap, unsigned int nbits); > + > #endif /* __ASSEMBLY__ */ > > #endif /* __XEN_BITMAP_H */ > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -27,9 +27,6 @@ struct xen_domctl_getdomaininfo; > void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info); > void arch_get_domain_info(const struct domain *d, > struct xen_domctl_getdomaininfo *info); > -int xenctl_bitmap_to_bitmap(unsigned long *bitmap, > - const struct xenctl_bitmap *xenctl_bitmap, > - unsigned int nbits); > > /* > * Arch-specifics. >
On 07.08.2020 19:50, Andrew Cooper wrote: > On 07/08/2020 12:33, Jan Beulich wrote: >> --- a/xen/common/bitmap.c >> +++ b/xen/common/bitmap.c >> @@ -384,3 +386,87 @@ void bitmap_byte_to_long(unsigned long * >> } >> >> #endif >> + >> +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, >> + const unsigned long *bitmap, unsigned int nbits) >> +{ >> + unsigned int guest_bytes, copy_bytes, i; >> + uint8_t zero = 0; >> + int err = 0; >> + uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8); >> + >> + if ( !bytemap ) >> + return -ENOMEM; >> + >> + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; >> + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); >> + >> + bitmap_long_to_byte(bytemap, bitmap, nbits); >> + >> + if ( copy_bytes != 0 ) >> + if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) ) >> + err = -EFAULT; >> + >> + for ( i = copy_bytes; !err && i < guest_bytes; i++ ) >> + if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) ) >> + err = -EFAULT; >> + >> + xfree(bytemap); >> + >> + return err; >> +} >> + >> +int xenctl_bitmap_to_bitmap(unsigned long *bitmap, >> + const struct xenctl_bitmap *xenctl_bitmap, >> + unsigned int nbits) >> +{ >> + unsigned int guest_bytes, copy_bytes; >> + int err = 0; >> + uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8); >> + >> + if ( !bytemap ) >> + return -ENOMEM; >> + >> + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; >> + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); >> + >> + if ( copy_bytes != 0 ) >> + { >> + if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) ) >> + err = -EFAULT; >> + if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) ) >> + bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 7)); >> + } >> + >> + if ( !err ) >> + bitmap_byte_to_long(bitmap, bytemap, nbits); >> + >> + xfree(bytemap); >> + >> + return err; >> +} >> + >> +int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap, >> + const cpumask_t *cpumask) >> +{ >> + return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask), >> + nr_cpu_ids); >> +} >> + >> +int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask, >> + const struct xenctl_bitmap *xenctl_cpumap) >> +{ >> + int err = 0; >> + >> + if ( alloc_cpumask_var(cpumask) ) { > > At a minimum, please fix this style during the move. Oh, I should have noticed this. I've also spotted a 2nd style issue. > However, the more I look at this code, the more concerned I get. > > There is absolutely no need to allocate(/doubly allocate) memory or > double/triple bounce buffer the data. All that is necessary is some > careful handling of the copy length, and trailing zeroing. > > The cpumask variants should be trivial static inline wrapper. The fact > that they're not suggests an API error. > > In reality, these are just data-shuffling helpers, and would probably > live better in a guest-access.c if we had a suitable one to hand, but I > guess bitmap.c will have to do for now. But changing the implementation is certainly way beyond the purpose here. (I also can't spot any pointless double allocation - the one in xenctl_bitmap_to_cpumask() allocates an output of the function.) Jan
--- a/xen/common/bitmap.c +++ b/xen/common/bitmap.c @@ -9,6 +9,8 @@ #include <xen/errno.h> #include <xen/bitmap.h> #include <xen/bitops.h> +#include <xen/cpumask.h> +#include <xen/guest_access.h> #include <asm/byteorder.h> /* @@ -384,3 +386,87 @@ void bitmap_byte_to_long(unsigned long * } #endif + +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, + const unsigned long *bitmap, unsigned int nbits) +{ + unsigned int guest_bytes, copy_bytes, i; + uint8_t zero = 0; + int err = 0; + uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8); + + if ( !bytemap ) + return -ENOMEM; + + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); + + bitmap_long_to_byte(bytemap, bitmap, nbits); + + if ( copy_bytes != 0 ) + if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) ) + err = -EFAULT; + + for ( i = copy_bytes; !err && i < guest_bytes; i++ ) + if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) ) + err = -EFAULT; + + xfree(bytemap); + + return err; +} + +int xenctl_bitmap_to_bitmap(unsigned long *bitmap, + const struct xenctl_bitmap *xenctl_bitmap, + unsigned int nbits) +{ + unsigned int guest_bytes, copy_bytes; + int err = 0; + uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8); + + if ( !bytemap ) + return -ENOMEM; + + guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; + copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); + + if ( copy_bytes != 0 ) + { + if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) ) + err = -EFAULT; + if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) ) + bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 7)); + } + + if ( !err ) + bitmap_byte_to_long(bitmap, bytemap, nbits); + + xfree(bytemap); + + return err; +} + +int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap, + const cpumask_t *cpumask) +{ + return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask), + nr_cpu_ids); +} + +int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask, + const struct xenctl_bitmap *xenctl_cpumap) +{ + int err = 0; + + if ( alloc_cpumask_var(cpumask) ) { + err = xenctl_bitmap_to_bitmap(cpumask_bits(*cpumask), xenctl_cpumap, + nr_cpu_ids); + /* In case of error, cleanup is up to us, as the caller won't care! */ + if ( err ) + free_cpumask_var(*cpumask); + } + else + err = -ENOMEM; + + return err; +} --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -34,91 +34,6 @@ static DEFINE_SPINLOCK(domctl_lock); -static int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, - const unsigned long *bitmap, - unsigned int nbits) -{ - unsigned int guest_bytes, copy_bytes, i; - uint8_t zero = 0; - int err = 0; - uint8_t *bytemap = xmalloc_array(uint8_t, (nbits + 7) / 8); - - if ( !bytemap ) - return -ENOMEM; - - guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; - copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); - - bitmap_long_to_byte(bytemap, bitmap, nbits); - - if ( copy_bytes != 0 ) - if ( copy_to_guest(xenctl_bitmap->bitmap, bytemap, copy_bytes) ) - err = -EFAULT; - - for ( i = copy_bytes; !err && i < guest_bytes; i++ ) - if ( copy_to_guest_offset(xenctl_bitmap->bitmap, i, &zero, 1) ) - err = -EFAULT; - - xfree(bytemap); - - return err; -} - -int xenctl_bitmap_to_bitmap(unsigned long *bitmap, - const struct xenctl_bitmap *xenctl_bitmap, - unsigned int nbits) -{ - unsigned int guest_bytes, copy_bytes; - int err = 0; - uint8_t *bytemap = xzalloc_array(uint8_t, (nbits + 7) / 8); - - if ( !bytemap ) - return -ENOMEM; - - guest_bytes = (xenctl_bitmap->nr_bits + 7) / 8; - copy_bytes = min_t(unsigned int, guest_bytes, (nbits + 7) / 8); - - if ( copy_bytes != 0 ) - { - if ( copy_from_guest(bytemap, xenctl_bitmap->bitmap, copy_bytes) ) - err = -EFAULT; - if ( (xenctl_bitmap->nr_bits & 7) && (guest_bytes == copy_bytes) ) - bytemap[guest_bytes-1] &= ~(0xff << (xenctl_bitmap->nr_bits & 7)); - } - - if ( !err ) - bitmap_byte_to_long(bitmap, bytemap, nbits); - - xfree(bytemap); - - return err; -} - -int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_cpumap, - const cpumask_t *cpumask) -{ - return bitmap_to_xenctl_bitmap(xenctl_cpumap, cpumask_bits(cpumask), - nr_cpu_ids); -} - -int xenctl_bitmap_to_cpumask(cpumask_var_t *cpumask, - const struct xenctl_bitmap *xenctl_cpumap) -{ - int err = 0; - - if ( alloc_cpumask_var(cpumask) ) { - err = xenctl_bitmap_to_bitmap(cpumask_bits(*cpumask), xenctl_cpumap, - nr_cpu_ids); - /* In case of error, cleanup is up to us, as the caller won't care! */ - if ( err ) - free_cpumask_var(*cpumask); - } - else - err = -ENOMEM; - - return err; -} - static int nodemask_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_nodemap, const nodemask_t *nodemask) { --- a/xen/include/xen/bitmap.h +++ b/xen/include/xen/bitmap.h @@ -273,6 +273,13 @@ static inline void bitmap_clear(unsigned void bitmap_long_to_byte(uint8_t *bp, const unsigned long *lp, int nbits); void bitmap_byte_to_long(unsigned long *lp, const uint8_t *bp, int nbits); +struct xenctl_bitmap; +int xenctl_bitmap_to_bitmap(unsigned long *bitmap, + const struct xenctl_bitmap *xenctl_bitmap, + unsigned int nbits); +int bitmap_to_xenctl_bitmap(struct xenctl_bitmap *xenctl_bitmap, + const unsigned long *bitmap, unsigned int nbits); + #endif /* __ASSEMBLY__ */ #endif /* __XEN_BITMAP_H */ --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -27,9 +27,6 @@ struct xen_domctl_getdomaininfo; void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info); void arch_get_domain_info(const struct domain *d, struct xen_domctl_getdomaininfo *info); -int xenctl_bitmap_to_bitmap(unsigned long *bitmap, - const struct xenctl_bitmap *xenctl_bitmap, - unsigned int nbits); /* * Arch-specifics.
A subsequent change will exclude domctl.c from getting built for a particular configuration, yet the two functions get used from elsewhere. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Move function decls to xen/bitmap.h.