Hi! Attached patch lets libxc build with lzma support on NetBSD. NetBSD doesn''t have sysconf(_SC_PHYS_PAGES). Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger writes ("[Xen-devel] [PATCH] libxc: lzma build
fix"):> Attached patch lets libxc build with lzma support on NetBSD.
> NetBSD doesn''t have sysconf(_SC_PHYS_PAGES).
Can you please break this out into separate os-dependent code in an
appropriate different file ? These kind of #ifdefs break up the code
and make it hard to see what''s going on.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On Wednesday 26 January 2011 17:16:11 Ian Jackson wrote:> Christoph Egger writes ("[Xen-devel] [PATCH] libxc: lzma build fix"): > > Attached patch lets libxc build with lzma support on NetBSD. > > NetBSD doesn''t have sysconf(_SC_PHYS_PAGES). > > Can you please break this out into separate os-dependent code in an > appropriate different file ? These kind of #ifdefs break up the code > and make it hard to see what''s going on.Yes. Attached patch factors physmem() out into os-dependent files and renamed it to xc_get_physmem() to not pollute the namespace. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger writes ("Re: [Xen-devel] [PATCH] libxc: lzma build
fix"):> Yes. Attached patch factors physmem() out into os-dependent files
> and renamed it to xc_get_physmem() to not pollute the namespace.
Great, thanks.
I fixed up your commit message, so it looked like this:
libxc: break xc_get_physmem out into os-dependent files
NetBSD doesn''t have sysconf(_SC_PHYS_PAGES).
Factor physmem() out into os-dependent files and rename it to
xc_get_physmem() so as not to pollute the namespace.
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On Wed, 2011-01-26 at 17:27 +0000, Christoph Egger wrote:> On Wednesday 26 January 2011 17:16:11 Ian Jackson wrote: > > Christoph Egger writes ("[Xen-devel] [PATCH] libxc: lzma build fix"): > > > Attached patch lets libxc build with lzma support on NetBSD. > > > NetBSD doesn''t have sysconf(_SC_PHYS_PAGES). > > > > Can you please break this out into separate os-dependent code in an > > appropriate different file ? These kind of #ifdefs break up the code > > and make it hard to see what''s going on. > > Yes. Attached patch factors physmem() out into os-dependent files > and renamed it to xc_get_physmem() to not pollute the namespace.I see this has now been applied, which is my fault for putting this aside to reply to later and then forgetting... The physmem value calculated by this function is only used as an argument to lzma_alone_decoder, it is divided by 3 to get the memory limit for the decoder. It''s not clear to me why a userspace lzma decode would want to use that particular value, what bearing it has on anything or why it would assume it could use 1/3 of the total RAM in the system (potentially quite a large amount of RAM) as opposed to any other limit number. Why not just hardcode 32M or something? A quick scan through the rdepends on Debian shows at least a couple of users doing so. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxc: lzma build
fix"):> The physmem value calculated by this function is only used as an
> argument to lzma_alone_decoder, it is divided by 3 to get the memory
> limit for the decoder.
I hadn''t spotted that. I should have looked more closely.
> It''s not clear to me why a userspace lzma decode would want to use
that
> particular value, what bearing it has on anything or why it would assume
> it could use 1/3 of the total RAM in the system (potentially quite a
> large amount of RAM) as opposed to any other limit number.
It''s dom0''s "physmem", so not the whole system, but
yes.
> Why not just hardcode 32M or something? A quick scan through the
> rdepends on Debian shows at least a couple of users doing so.
Yes, we could revert this patch and hardcode a value. 32M seems
plausible.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson writes ("Re: [Xen-devel] [PATCH] libxc: lzma build
fix"):> Yes, we could revert this patch and hardcode a value. 32M seems
> plausible.
How about this.
Ian.
libxc: Do not use host physmem as parameter to lzma decoder
It''s not clear why a userspace lzma decode would want to use that
particular value, what bearing it has on anything or why it would
assume it could use 1/3 of the total RAM in the system (potentially
quite a large amount of RAM) as opposed to any other limit number.
Instead, hardcode 32Mby.
This reverts 22830:c80960244942, removes the xc_get_physmem/physmem
function entirely, and replaces the expression at the call site with a
fixed constant.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
Cc: Christoph Egger <Christoph.Egger@amd.com>
diff -r 88cf07fed7d2 tools/libxc/xc_dom_bzimageloader.c
--- a/tools/libxc/xc_dom_bzimageloader.c Fri Jan 28 18:39:09 2011 +0000
+++ b/tools/libxc/xc_dom_bzimageloader.c Fri Jan 28 18:49:08 2011 +0000
@@ -152,7 +152,7 @@ static int xc_try_lzma_decode(
int outsize;
const char *msg;
- ret = lzma_alone_decoder(&stream, xc_get_physmem() / 3);
+ ret = lzma_alone_decoder(&stream, 32*1024*1024);
if ( ret != LZMA_OK )
{
DOMPRINTF("LZMA: Failed to init stream decoder");
diff -r 88cf07fed7d2 tools/libxc/xc_linux.c
--- a/tools/libxc/xc_linux.c Fri Jan 28 18:39:09 2011 +0000
+++ b/tools/libxc/xc_linux.c Fri Jan 28 18:49:08 2011 +0000
@@ -55,27 +55,6 @@ void discard_file_cache(xc_interface *xc
errno = saved_errno;
}
-uint64_t xc_get_physmem(void)
-{
- uint64_t ret = 0;
- const long pagesize = sysconf(_SC_PAGESIZE);
- const long pages = sysconf(_SC_PHYS_PAGES);
-
- if ( (pagesize != -1) || (pages != -1) )
- {
- /*
- * According to docs, pagesize * pages can overflow.
- * Simple case is 32-bit box with 4 GiB or more RAM,
- * which may report exactly 4 GiB of RAM, and "long"
- * being 32-bit will overflow. Casting to uint64_t
- * hopefully avoids overflows in the near future.
- */
- ret = (uint64_t)(pagesize) * (uint64_t)(pages);
- }
-
- return ret;
-}
-
/*
* Local variables:
* mode: C
diff -r 88cf07fed7d2 tools/libxc/xc_netbsd.c
--- a/tools/libxc/xc_netbsd.c Fri Jan 28 18:39:09 2011 +0000
+++ b/tools/libxc/xc_netbsd.c Fri Jan 28 18:49:08 2011 +0000
@@ -23,9 +23,6 @@
#include <xen/sys/evtchn.h>
#include <unistd.h>
#include <fcntl.h>
-#include <stdio.h>
-#include <errno.h>
-#include <sys/sysctl.h>
static xc_osdep_handle netbsd_privcmd_open(xc_interface *xch)
{
@@ -354,24 +351,6 @@ void discard_file_cache(xc_interface *xc
errno = saved_errno;
}
-uint64_t xc_get_physmem(void)
-{
- int mib[2], rc;
- size_t len;
- uint64_t physmem;
-
- mib[0] = CTL_HW;
- mib[1] = HW_PHYSMEM64;
- rc = sysctl(mib, 2, &physmem, &len, NULL, 0);
-
- if (rc == -1) {
- /* PERROR("%s: Failed to get hw.physmem64: %s\n",
strerror(errno)); */
- return 0;
- }
-
- return physmem;
-}
-
static struct xc_osdep_ops *netbsd_osdep_init(xc_interface *xch, enum
xc_osdep_type type)
{
switch ( type )
diff -r 88cf07fed7d2 tools/libxc/xc_private.h
--- a/tools/libxc/xc_private.h Fri Jan 28 18:39:09 2011 +0000
+++ b/tools/libxc/xc_private.h Fri Jan 28 18:49:08 2011 +0000
@@ -275,9 +275,6 @@ void bitmap_byte_to_64(uint64_t *lp, con
/* Optionally flush file to disk and discard page cache */
void discard_file_cache(xc_interface *xch, int fd, int flush);
-/* How much physical RAM is available? */
-uint64_t xc_get_physmem(void);
-
#define MAX_MMU_UPDATES 1024
struct xc_mmu {
mmu_update_t updates[MAX_MMU_UPDATES];
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On Fri, 2011-01-28 at 18:51 +0000, Ian Jackson wrote:> Ian Jackson writes ("Re: [Xen-devel] [PATCH] libxc: lzma build fix"): > > Yes, we could revert this patch and hardcode a value. 32M seems > > plausible. > > How about this. > > Ian. > > > libxc: Do not use host physmem as parameter to lzma decoderStrictly speaking it''s the dom0 physmem.> It''s not clear why a userspace lzma decode would want to use that > particular value, what bearing it has on anything or why it would > assume it could use 1/3 of the total RAM in the system (potentially > quite a large amount of RAM) as opposed to any other limit number.Sounds good to me ;-)> Instead, hardcode 32Mby. > > This reverts 22830:c80960244942, removes the xc_get_physmem/physmem > function entirely, and replaces the expression at the call site with a > fixed constant. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>> Cc: Christoph Egger <Christoph.Egger@amd.com> > > diff -r 88cf07fed7d2 tools/libxc/xc_dom_bzimageloader.c > --- a/tools/libxc/xc_dom_bzimageloader.c Fri Jan 28 18:39:09 2011 +0000 > +++ b/tools/libxc/xc_dom_bzimageloader.c Fri Jan 28 18:49:08 2011 +0000 > @@ -152,7 +152,7 @@ static int xc_try_lzma_decode( > int outsize; > const char *msg; > > - ret = lzma_alone_decoder(&stream, xc_get_physmem() / 3); > + ret = lzma_alone_decoder(&stream, 32*1024*1024); > if ( ret != LZMA_OK ) > { > DOMPRINTF("LZMA: Failed to init stream decoder"); > diff -r 88cf07fed7d2 tools/libxc/xc_linux.c > --- a/tools/libxc/xc_linux.c Fri Jan 28 18:39:09 2011 +0000 > +++ b/tools/libxc/xc_linux.c Fri Jan 28 18:49:08 2011 +0000 > @@ -55,27 +55,6 @@ void discard_file_cache(xc_interface *xc > errno = saved_errno; > } > > -uint64_t xc_get_physmem(void) > -{ > - uint64_t ret = 0; > - const long pagesize = sysconf(_SC_PAGESIZE); > - const long pages = sysconf(_SC_PHYS_PAGES); > - > - if ( (pagesize != -1) || (pages != -1) ) > - { > - /* > - * According to docs, pagesize * pages can overflow. > - * Simple case is 32-bit box with 4 GiB or more RAM, > - * which may report exactly 4 GiB of RAM, and "long" > - * being 32-bit will overflow. Casting to uint64_t > - * hopefully avoids overflows in the near future. > - */ > - ret = (uint64_t)(pagesize) * (uint64_t)(pages); > - } > - > - return ret; > -} > - > /* > * Local variables: > * mode: C > diff -r 88cf07fed7d2 tools/libxc/xc_netbsd.c > --- a/tools/libxc/xc_netbsd.c Fri Jan 28 18:39:09 2011 +0000 > +++ b/tools/libxc/xc_netbsd.c Fri Jan 28 18:49:08 2011 +0000 > @@ -23,9 +23,6 @@ > #include <xen/sys/evtchn.h> > #include <unistd.h> > #include <fcntl.h> > -#include <stdio.h> > -#include <errno.h> > -#include <sys/sysctl.h> > > static xc_osdep_handle netbsd_privcmd_open(xc_interface *xch) > { > @@ -354,24 +351,6 @@ void discard_file_cache(xc_interface *xc > errno = saved_errno; > } > > -uint64_t xc_get_physmem(void) > -{ > - int mib[2], rc; > - size_t len; > - uint64_t physmem; > - > - mib[0] = CTL_HW; > - mib[1] = HW_PHYSMEM64; > - rc = sysctl(mib, 2, &physmem, &len, NULL, 0); > - > - if (rc == -1) { > - /* PERROR("%s: Failed to get hw.physmem64: %s\n", strerror(errno)); */ > - return 0; > - } > - > - return physmem; > -} > - > static struct xc_osdep_ops *netbsd_osdep_init(xc_interface *xch, enum xc_osdep_type type) > { > switch ( type ) > diff -r 88cf07fed7d2 tools/libxc/xc_private.h > --- a/tools/libxc/xc_private.h Fri Jan 28 18:39:09 2011 +0000 > +++ b/tools/libxc/xc_private.h Fri Jan 28 18:49:08 2011 +0000 > @@ -275,9 +275,6 @@ void bitmap_byte_to_64(uint64_t *lp, con > /* Optionally flush file to disk and discard page cache */ > void discard_file_cache(xc_interface *xch, int fd, int flush); > > -/* How much physical RAM is available? */ > -uint64_t xc_get_physmem(void); > - > #define MAX_MMU_UPDATES 1024 > struct xc_mmu { > mmu_update_t updates[MAX_MMU_UPDATES];_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxc: lzma build
fix"):> Strictly speaking it''s the dom0 physmem.
Ah, yes, I shouldn''t speak so loosely.
> Acked-by: Ian Campbell <Ian.Campbell@eu.citrix.com>
Applied, thanks.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
>>> On 28.01.11 at 19:51, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > libxc: Do not use host physmem as parameter to lzma decoder > > It''s not clear why a userspace lzma decode would want to use that > particular value, what bearing it has on anything or why it would > assume it could use 1/3 of the total RAM in the system (potentially > quite a large amount of RAM) as opposed to any other limit number. > > Instead, hardcode 32Mby.Has anyone of you actually tested this? I don''t get lzma to work with this set to less than 128Mb, and xz''s configure.ac also sort of states this to be a minimal requirement for arbitrary input.> This reverts 22830:c80960244942, removes the xc_get_physmem/physmem > function entirely, and replaces the expression at the call site with a > fixed constant. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> > Cc: Christoph Egger <Christoph.Egger@amd.com>Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, 2011-02-11 at 13:41 +0000, Jan Beulich wrote:> >>> On 28.01.11 at 19:51, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: > > libxc: Do not use host physmem as parameter to lzma decoder > > > > It''s not clear why a userspace lzma decode would want to use that > > particular value, what bearing it has on anything or why it would > > assume it could use 1/3 of the total RAM in the system (potentially > > quite a large amount of RAM) as opposed to any other limit number. > > > > Instead, hardcode 32Mby. > > Has anyone of you actually tested this? I don''t get lzma to work > with this set to less than 128Mb, and xz''s configure.ac also sort > of states this to be a minimal requirement for arbitrary input.I looked quite hard for any documentation on this and couldn''t find any, I could only find examples of people using host physmem like we were or hardcoding ~30MB. I should have looked in configure.ac, its obvious really ;-) Putting aside the fact that 128MB is a quite obscene amount of memory as a _minimum_ requirement for something like this I guess we should change the constant. Ian.> > > This reverts 22830:c80960244942, removes the xc_get_physmem/physmem > > function entirely, and replaces the expression at the call site with a > > fixed constant. > > > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Ian Campbell <Ian.Campbell@eu.citrix.com> > > Cc: Christoph Egger <Christoph.Egger@amd.com> > > Jan >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxc: lzma build
fix"):> Putting aside the fact that 128MB is a quite obscene amount of memory as
> a _minimum_ requirement for something like this I guess we should change
> the constant.
I have done so.
Ian.
# HG changeset patch
# User Ian Jackson <Ian.Jackson@eu.citrix.com>
# Date 1297446553 0
# Node ID 4376c4f0196f0dc129904a0f8c54c74ae4bcdff2
# Parent 6c22ae0f654084a4d2811e51e22b7f8b20b49cec
libxc: increase lzma max memory constant to 128Mby
According to lzma''s configure.ac (!) the minimum memory limit to cope
with arbitrary input is 128Mby (!)
This is obviously an unreasonable amount of memory for this kind of
task, but we need to increase the constant limit for it not to
randomly fail. So do so.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff -r 6c22ae0f6540 -r 4376c4f0196f tools/libxc/xc_dom_bzimageloader.c
--- a/tools/libxc/xc_dom_bzimageloader.c Fri Feb 11 16:51:44 2011 +0000
+++ b/tools/libxc/xc_dom_bzimageloader.c Fri Feb 11 17:49:13 2011 +0000
@@ -152,7 +152,7 @@ static int xc_try_lzma_decode(
int outsize;
const char *msg;
- ret = lzma_alone_decoder(&stream, 32*1024*1024);
+ ret = lzma_alone_decoder(&stream, 128*1024*1024);
if ( ret != LZMA_OK )
{
DOMPRINTF("LZMA: Failed to init stream decoder");
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel