Re: [PATCH v2 04/10] mini-os: respect memory map when ballooning up

2021-12-21 Thread Samuel Thibault 
Juergen Gross, le mar. 21 déc. 2021 07:16:49 +0100, a ecrit:
> On 21.12.21 00:22, Samuel Thibault wrote:
> > Juergen Gross, le lun. 20 déc. 2021 17:07:10 +0100, a ecrit:
> > > +unsigned long e820_get_max_contig_pages(unsigned long pfn, unsigned long 
> > > pages)
> > > +{
> > > +int i;
> > > +unsigned long end;
> > > +
> > > +for ( i = 0; i < e820_entries && e820_map[i].addr < (pfn << 
> > > PAGE_SHIFT);
> > 
> > Shouldn't that be addr+size? Otherwise if pfn is in the middle of an
> > e820 entry, we will miss picking up that.
> 
> No, we want to check all map entries starting below or at the given pfn.
> The test should be "e820_map[i].addr <= (pfn << PAGE_SHIFT)", of course.

Ah, yes, due to the "<" I mistook it for a loop that skips the entries
before the targetted pfn :)

With <=, 

Reviewed-by: Samuel Thibault 

Samuel

Re: [PATCH v2 04/10] mini-os: respect memory map when ballooning up

2021-12-20 Thread Juergen Gross 

On 21.12.21 00:22, Samuel Thibault wrote:

Juergen Gross, le lun. 20 déc. 2021 17:07:10 +0100, a ecrit:

+unsigned long e820_get_max_contig_pages(unsigned long pfn, unsigned long pages)
+{
+int i;
+unsigned long end;
+
+for ( i = 0; i < e820_entries && e820_map[i].addr < (pfn << PAGE_SHIFT);


Shouldn't that be addr+size? Otherwise if pfn is in the middle of an
e820 entry, we will miss picking up that.


No, we want to check all map entries starting below or at the given pfn.
The test should be "e820_map[i].addr <= (pfn << PAGE_SHIFT)", of course.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH v2 04/10] mini-os: respect memory map when ballooning up

2021-12-20 Thread Samuel Thibault 
Juergen Gross, le lun. 20 déc. 2021 17:07:10 +0100, a ecrit:
> +unsigned long e820_get_max_contig_pages(unsigned long pfn, unsigned long 
> pages)
> +{
> +int i;
> +unsigned long end;
> +
> +for ( i = 0; i < e820_entries && e820_map[i].addr < (pfn << PAGE_SHIFT);

Shouldn't that be addr+size? Otherwise if pfn is in the middle of an
e820 entry, we will miss picking up that.

> +  i++ )
> +{
> +end = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> +if ( e820_map[i].type != E820_RAM || end <= pfn )
> +continue;
> +
> +return ((end - pfn) > pages) ? pages : end - pfn;
> +}
> +
> +return 0;
> +}