Re: [PATCH 06/10] mini-os: add memory map service functions

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le lun. 06 déc. 2021 08:23:33 +0100, a ecrit:
> +void e820_put_reserved_pfns(unsigned long start_pfn, int pages)
> +{
> +    int i;
> +    unsigned long addr = start_pfn << PAGE_SHIFT;
> +    unsigned long size = (long)pages << PAGE_SHIFT;
> +
> +    for ( i = 0; i < e820_entries && addr < e820_map[i].addr; i++ );

Shouldn't that be addr > e820_map[i].addr + e820_map[i].size?

> +    BUG_ON(i == e820_entries || e820_map[i].type != E820_RESERVED);

We should also BUG_ON e820_map[i].addr > addr (i.e. we didn't find an
entry that contained our address).

> +    if ( addr == e820_map[i].addr )
> +    {
> +        e820_map[i].addr += size;

I'd say BUG_ON here if e820_map[i].size < size.

> +        e820_map[i].size -= size;
> +        if ( e820_map[i].size == 0 )
> +            e820_remove_entry(i);
> +        return;
> +    }
> +
> +    if ( addr + size == e820_map[i].addr + e820_map[i].size )
> +    {
> +        e820_map[i].addr = addr;
> +        e820_map[i].size = size;

? Shouldn't that rather be just

> +        e820_map[i].size -= size;

? (since what we remove is at the end of the area, the start of the area
doesn't change)

> +        return;
> +    }