Re: [PATCH 03/10] mini-os: don't assume contiguous RAM when initializing in PVH mode

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le mar. 14 déc. 2021 07:35:54 +0100, a ecrit:
> On 13.12.21 22:22, Samuel Thibault wrote:
> > Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit:
> > > On 12.12.21 01:15, Samuel Thibault wrote:
> > > > Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit:
> > > > > -    unsigned long pfn, max = 0;
> > > > > +    unsigned long pfns, max = 0;
> > > > 
> > > > I'd say rather rename max to start.
> > > > 
> > > > >        e820_get_memmap();
> > > > > @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void)
> > > > >        {
> > > > >            if ( e820_map[i].type != E820_RAM )
> > > > >                continue;
> > > > > -        pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT;
> > > > > -        if ( pfn > max )
> > > > > -            max = pfn;
> > > > > +        pfns = e820_map[i].size >> PAGE_SHIFT;
> > > > > +        max = e820_map[i].addr >> PAGE_SHIFT;
> > > > 
> > > > since it's it's always the start of the e820 entry.
> > > > 
> > > > > +        if ( pages <= pfns )
> > > > > +            return max + pages;
> > > > > +        pages -= pfns;
> > > > > +        max += pfns;
> > > > 
> > > > Here we don't need do change max, only pages.
> > > 
> > > It is needed in case the loop is finished.
> > > 
> > > And this was the reason for naming it max.
> > 
> > Ah, ok.
> > 
> > At first read the name was confusing me. Perhaps better use two
> > variables then: start and max, so that we have
> > 
> > start = e820_map[i].addr >> PAGE_SHIFT;
> > if ( pages <= pfns )
> >      return start + pages;
> > pages -= pfns;
> > max = start + pfns;
> 
> Hmm, or I can rename max to start, drop the "max += pfns;" and do a
> "return start + pfns;" at the end of the function.

That could do as well, yes.

Samuel