| Message ID | 1384348525-3230-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 2013-11-13 at 13:15 +0000, Julien Grall wrote: > On ARM, when an initrd is given to xen by U-boot, it will reserve the memory in > the device tree. > In this case, when xen decides to free unused memory, dt_unreserved_regions > will call init_domheap_pages with the start and the end of range equals. But > the latter assumes that (start > end), if not Xen will hang because the > number of pages is equals to (unsigned)-1. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <jbeulich@suse.com> > > --- > Changes in v2: > - Change commit title > - Move the check in init_domheap_pages > --- > xen/common/page_alloc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 4c17fbd..10f67a1 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) > smfn = round_pgup(ps) >> PAGE_SHIFT; > emfn = round_pgdown(pe) >> PAGE_SHIFT; > > + if ( smfn <= emfn ) You've got this backwards I think. > + return; > + > init_heap_pages(mfn_to_page(smfn), emfn - smfn); > } >
>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote: > On ARM, when an initrd is given to xen by U-boot, it will reserve the memory > in the device tree. > In this case, when xen decides to free unused memory, dt_unreserved_regions > will call init_domheap_pages with the start and the end of range equals. But > the latter assumes that (start > end), if not Xen will hang because the > number of pages is equals to (unsigned)-1. The change is simple enough, so I don't really mind it going in, but I wonder ... > Signed-off-by: Julien Grall <julien.grall@linaro.org> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <jbeulich@suse.com> > > --- > Changes in v2: > - Change commit title > - Move the check in init_domheap_pages ... who and why suggested to move it here. After all, I'm considering it an error to call the function with non-page-aligned addresses and/ or end < start (I take it that page-aligned, but start == end is not a problem without your change). Jan > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) > smfn = round_pgup(ps) >> PAGE_SHIFT; > emfn = round_pgdown(pe) >> PAGE_SHIFT; > > + if ( smfn <= emfn ) > + return; > + > init_heap_pages(mfn_to_page(smfn), emfn - smfn); > } > > -- > 1.8.3.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 11/13/2013 01:26 PM, Jan Beulich wrote: >>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote: >> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory >> in the device tree. >> In this case, when xen decides to free unused memory, dt_unreserved_regions >> will call init_domheap_pages with the start and the end of range equals. But >> the latter assumes that (start > end), if not Xen will hang because the >> number of pages is equals to (unsigned)-1. > > The change is simple enough, so I don't really mind it going in, but > I wonder ... > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> CC: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <jbeulich@suse.com> >> >> --- >> Changes in v2: >> - Change commit title >> - Move the check in init_domheap_pages > > ... who and why suggested to move it here. After all, I'm considering > it an error to call the function with non-page-aligned addresses and/ > or end < start (I take it that page-aligned, but start == end is not a > problem without your change). if ps == pe, then emfn == (smfn - 1). This will result to the number of pages of -1. There is a similar check in init_xenheap_pages, it doesn't seem harmfull to let it here.
On 11/13/2013 01:23 PM, Ian Campbell wrote: > On Wed, 2013-11-13 at 13:15 +0000, Julien Grall wrote: >> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory in >> the device tree. >> In this case, when xen decides to free unused memory, dt_unreserved_regions >> will call init_domheap_pages with the start and the end of range equals. But >> the latter assumes that (start > end), if not Xen will hang because the >> number of pages is equals to (unsigned)-1. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> CC: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <jbeulich@suse.com> >> >> --- >> Changes in v2: >> - Change commit title >> - Move the check in init_domheap_pages >> --- >> xen/common/page_alloc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >> index 4c17fbd..10f67a1 100644 >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) >> smfn = round_pgup(ps) >> PAGE_SHIFT; >> emfn = round_pgdown(pe) >> PAGE_SHIFT; >> >> + if ( smfn <= emfn ) > > You've got this backwards I think. Oh right, I will fix it in the next version. > >> + return; >> + >> init_heap_pages(mfn_to_page(smfn), emfn - smfn); >> } >> > >
On Wed, 2013-11-13 at 13:26 +0000, Jan Beulich wrote: > >>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote: > > On ARM, when an initrd is given to xen by U-boot, it will reserve the memory > > in the device tree. > > In this case, when xen decides to free unused memory, dt_unreserved_regions > > will call init_domheap_pages with the start and the end of range equals. But > > the latter assumes that (start > end), if not Xen will hang because the > > number of pages is equals to (unsigned)-1. > > The change is simple enough, so I don't really mind it going in, but > I wonder ... > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > CC: Keir Fraser <keir@xen.org> > > CC: Jan Beulich <jbeulich@suse.com> > > > > --- > > Changes in v2: > > - Change commit title > > - Move the check in init_domheap_pages > > ... who and why suggested to move it here. Me in response to "xen/arm: Don't call init_domheap_page with an empty range". > After all, I'm considering > it an error to call the function with non-page-aligned addresses and/ > or end < start (I take it that page-aligned, but start == end is not a > problem without your change). Since init_xenheap_page does the right thing it seemed reasonable to me to make domheap do the same. Ian,
>>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote: > > On 11/13/2013 01:26 PM, Jan Beulich wrote: >>>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote: >>> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory >>> in the device tree. >>> In this case, when xen decides to free unused memory, dt_unreserved_regions >>> will call init_domheap_pages with the start and the end of range equals. But >>> the latter assumes that (start > end), if not Xen will hang because the >>> number of pages is equals to (unsigned)-1. >> >> The change is simple enough, so I don't really mind it going in, but >> I wonder ... >> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>> CC: Keir Fraser <keir@xen.org> >>> CC: Jan Beulich <jbeulich@suse.com> >>> >>> --- >>> Changes in v2: >>> - Change commit title >>> - Move the check in init_domheap_pages >> >> ... who and why suggested to move it here. After all, I'm considering >> it an error to call the function with non-page-aligned addresses and/ >> or end < start (I take it that page-aligned, but start == end is not a >> problem without your change). > > if ps == pe, then emfn == (smfn - 1). This will result to the number of > pages of -1. I'm not following: If ps == pe and they're page aligned, then smfn = round_pgup(ps) >> PAGE_SHIFT; emfn = round_pgdown(pe) >> PAGE_SHIFT; produces smfn == emfn. So as said earlier - emfn = smfn - 1 requires the input to either be not page aligned, or pe < ps, both of which look invalid to me. > There is a similar check in init_xenheap_pages, it doesn't seem harmfull > to let it here. In the CONFIG_SEPARATE_XENHEAP case, yes. And it's there for a good reason - that code doesn't tolerate ps == pe (and if I'm not mistaken would break even on specific ps < pe cases). Jan
On Wed, 2013-11-13 at 14:12 +0000, Jan Beulich wrote: > >>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote: > > > > > On 11/13/2013 01:26 PM, Jan Beulich wrote: > >>>>> On 13.11.13 at 14:15, Julien Grall <julien.grall@linaro.org> wrote: > >>> On ARM, when an initrd is given to xen by U-boot, it will reserve the memory > >>> in the device tree. > >>> In this case, when xen decides to free unused memory, dt_unreserved_regions > >>> will call init_domheap_pages with the start and the end of range equals. But > >>> the latter assumes that (start > end), if not Xen will hang because the > >>> number of pages is equals to (unsigned)-1. > >> > >> The change is simple enough, so I don't really mind it going in, but > >> I wonder ... > >> > >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >>> CC: Keir Fraser <keir@xen.org> > >>> CC: Jan Beulich <jbeulich@suse.com> > >>> > >>> --- > >>> Changes in v2: > >>> - Change commit title > >>> - Move the check in init_domheap_pages > >> > >> ... who and why suggested to move it here. After all, I'm considering > >> it an error to call the function with non-page-aligned addresses and/ > >> or end < start (I take it that page-aligned, but start == end is not a > >> problem without your change). > > > > if ps == pe, then emfn == (smfn - 1). This will result to the number of > > pages of -1. > > I'm not following: If ps == pe and they're page aligned, then > > smfn = round_pgup(ps) >> PAGE_SHIFT; > emfn = round_pgdown(pe) >> PAGE_SHIFT; > > produces smfn == emfn. So as said earlier - emfn = smfn - 1 requires > the input to either be not page aligned, or pe < ps, both of which look > invalid to me. > > > There is a similar check in init_xenheap_pages, it doesn't seem harmfull > > to let it here. > > In the CONFIG_SEPARATE_XENHEAP case, yes. And it's there for > a good reason - that code doesn't tolerate ps == pe (and if I'm not > mistaken would break even on specific ps < pe cases). init_xenheap_pages doesn't tolerate it, so it checks for it instead of pushing the burden onto the caller. Julien is adding the same check to init_domheap_pages which also doesn't tolerate it, but here you are arguing that it should be up to the caller to not pass invalid parameters. This could be fixed in the caller, but it seems cleaner to do it in the core to me, since there are plenty of case where you are munging around with addresses and catching all the corners where that munging ends up makes end<=start makes that code uglier. Ian.
>>> On 13.11.13 at 15:18, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2013-11-13 at 14:12 +0000, Jan Beulich wrote: >> >>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote: >> > There is a similar check in init_xenheap_pages, it doesn't seem harmfull >> > to let it here. >> >> In the CONFIG_SEPARATE_XENHEAP case, yes. And it's there for >> a good reason - that code doesn't tolerate ps == pe (and if I'm not >> mistaken would break even on specific ps < pe cases). > > init_xenheap_pages doesn't tolerate it, so it checks for it instead of > pushing the burden onto the caller. > > Julien is adding the same check to init_domheap_pages which also doesn't > tolerate it, but here you are arguing that it should be up to the caller > to not pass invalid parameters. Not exactly: init_xenheap_pages() wants to _shrink_ the area (under certain conditions), and hence needs to check even for the valid (from a calling perspective) case of ps == pe. > This could be fixed in the caller, but it seems cleaner to do it in the > core to me, since there are plenty of case where you are munging around > with addresses and catching all the corners where that munging ends up > makes end<=start makes that code uglier. And again, I'm not fundamentally opposed to the patch (and it's anyway Keir who'll need to judge on whether to take it), I'm just trying to point out that there must be something wrong with how the function is being called (and that if it was called with proper arguments, the issue would have shown up in the first place). Jan
On 11/13/2013 02:12 PM, Jan Beulich wrote: >>>> On 13.11.13 at 14:34, Julien Grall <julien.grall@linaro.org> wrote: > > I'm not following: If ps == pe and they're page aligned, then > > smfn = round_pgup(ps) >> PAGE_SHIFT; > emfn = round_pgdown(pe) >> PAGE_SHIFT; > > produces smfn == emfn. So as said earlier - emfn = smfn - 1 requires > the input to either be not page aligned, or pe < ps, both of which look > invalid to me. Right, my fault I didn't pay attention that ps and pe is non-aligned, with the same value. That will result to my previous assertion. In any case, init_domheap_pages seems to be able to handle non-aligned address. Otherwise round_pg{up,down} is not necessary.
On Wed, 2013-11-13 at 14:32 +0000, Jan Beulich wrote: > I'm just > trying to point out that there must be something wrong with > how the function is being called (and that if it was called with > proper arguments, the issue would have shown up in the first > place). I don't think any one has disputed that the inputs are invalid or that the caller could be changed instead. I think it is better to make this function more liberal in what it accepts (by rejecting certain invalid constructs) than to add complexity to all the callers. Ian.
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 4c17fbd..10f67a1 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1429,6 +1429,9 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) smfn = round_pgup(ps) >> PAGE_SHIFT; emfn = round_pgdown(pe) >> PAGE_SHIFT; + if ( smfn <= emfn ) + return; + init_heap_pages(mfn_to_page(smfn), emfn - smfn); }
On ARM, when an initrd is given to xen by U-boot, it will reserve the memory in the device tree. In this case, when xen decides to free unused memory, dt_unreserved_regions will call init_domheap_pages with the start and the end of range equals. But the latter assumes that (start > end), if not Xen will hang because the number of pages is equals to (unsigned)-1. Signed-off-by: Julien Grall <julien.grall@linaro.org> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <jbeulich@suse.com> --- Changes in v2: - Change commit title - Move the check in init_domheap_pages --- xen/common/page_alloc.c | 3 +++ 1 file changed, 3 insertions(+)