Re: [PATCH 3/3] mini-os: fix number of pages for PVH

[Juergen Gross <jgross@suse.com>]

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

On 18.06.22 17:56, Samuel Thibault wrote:
> Juergen Gross, le sam. 18 juin 2022 16:07:07 +0200, a ecrit:
>> On 18.06.22 14:13, Samuel Thibault wrote:
>>> Hello,
>>>
>>> Juergen Gross, le sam. 18 juin 2022 12:48:16 +0200, a ecrit:
>>>> @@ -124,7 +126,7 @@ void arch_mm_preinit(void *p)
>>>>            do_exit();
>>>>        }
>>>> -    last_free_pfn = e820_get_maxpfn(ret);
>>>> +    last_free_pfn = e820_get_maxpfn(ret - e820_initial_reserved_pfns);
>>>
>>> Mmm, but the reserved pfn could be in the middle of the e820 address
>>> space.
>>
>> That doesn't matter.
>>
>> e820_get_maxpfn(n) will just return the pfn of the n-th RAM pfn it is
>> finding in the E820 map.
> 
> Yes, but subtracting at this point looks a bit hacky to me.
> 
> It seems to me that it'd be better to make e820_get_maxpfn count by
> itself the reserved pages (but never return its pfn of course), rather
> than having to make e820_sanitize look at the reserved pages, store
> it somewhere, and hope that other code will remember to subtract that
> before calling e820_get_maxpfn.
> 
> I mean something like:
> 
> unsigned long e820_get_maxpfn(unsigned long pages)
> {
>      int i;
>      unsigned long pfns = 0, start = 0;
> 
>      if ( !e820_entries )
>          e820_get_memmap();
> 
>      for ( i = 0; i < e820_entries; i++ )
>      {
>          pfns = e820_map[i].size >> PAGE_SHIFT;
> 
> 	if ( e820_map[i].type == E820_RESERVED )
> 	{
> 	    /* This counts in the memory reservation, but is not usable */
>              pages -= pfns;
> 	    continue;
> 	}
>          if ( e820_map[i].type != E820_RAM )
>              continue;
> 
>          start = e820_map[i].addr >> PAGE_SHIFT;
>          if ( pages <= pfns )
>              return start + pages;
>          pages -= pfns;
>      }
> 
>      return start + pfns;
> }

This would lead to wrong values of nr_mem_pages. I think the best solution
would be to have functions returning the number of available and max RAM
pages to e820.c. This would address your valid concern, while not leading
to wrong values at the callers side.


Juergen

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

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