Re: [PATCH ARM v6 08/14] mini-os: arm: memory management

[Julien Grall <julien.grall@linaro.org>]

Hi Thomas,

On 07/16/2014 12:07 PM, Thomas Leonard wrote:
> Based on an initial patch by Karim Raslan.
> 
> Signed-off-by: Karim Allah Ahmed <karim.allah.ahmed@gmail.com>
> Signed-off-by: Thomas Leonard <talex5@gmail.com>
> 
> ---
> 
> Changes since v5:
> 
> - Require only minimum property lengths (requested by Ian Campbell).
> 
> Addressed Julien Grall's comments:
> - Added comments explaining the lengths when checking FDT properties.
> - Use paddr_t type to represent physical addresses.
> ---
>  extras/mini-os/arch/arm/mm.c | 138 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>  create mode 100644 extras/mini-os/arch/arm/mm.c
> 
> diff --git a/extras/mini-os/arch/arm/mm.c b/extras/mini-os/arch/arm/mm.c
> new file mode 100644
> index 0000000..ec1f821
> --- /dev/null
> +++ b/extras/mini-os/arch/arm/mm.c
> @@ -0,0 +1,138 @@
> +#include <mini-os/console.h>
> +#include <xen/memory.h>
> +#include <arch_mm.h>
> +#include <mini-os/hypervisor.h>
> +#include <libfdt.h>
> +#include <lib.h>
> +
> +int physical_address_offset;

int64_t maybe?

> +
> +unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
> +{
> +    // FIXME
> +    BUG();
> +}
> +
> +void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
> +{

[..]

> +    unsigned int end = (unsigned int) &_end;
> +    paddr_t mem_base = fdt64_to_cpu(regs[0]);
> +    unsigned int mem_size = fdt64_to_cpu(regs[1]);

mem_size should be a paddr_t. Even though, the first bank will always
have the size fit in 32 bits.

> +    printk("Found memory at %p (len 0x%x)\n", mem_base, mem_size);
> +
> +    BUG_ON(to_virt(mem_base) > (void *) &_text);          /* Our image isn't in our RAM! */
> +    *start_pfn_p = PFN_UP(to_phys(end));
> +    int heap_len = mem_size - (PFN_PHYS(*start_pfn_p) - mem_base);

You need to use at least an unsigned int here. And it will be better if
you use paddr_t.

> +    *max_pfn_p = *start_pfn_p + PFN_DOWN(heap_len);
> +
> +    printk("Using pages %d to %d as free space for heap.\n", *start_pfn_p, *max_pfn_p);
> +
> +    /* The device tree is probably in memory that we're about to hand over to the page
> +     * allocator, so move it to the end and reserve that space.
> +     */
> +    int fdt_size = fdt_totalsize(device_tree);

uint32_t has the definition of the field totalsize in the fdt structure.

Regards,

-- 
Julien Grall