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

[Thomas Leonard <talex5@xxxxxxxxx>]

On 21 July 2014 18:36, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> Signed-off-by: Thomas Leonard <talex5@xxxxxxxxx>
>>
>> ---
>>
>> 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?

Currently we're using the short descriptor format, so we do assume
physical addresses are 32-bit. But it probably does make sense to
treat them as 64-bit and BUG_ON any we can't handle for now, so I will
change this as you suggest.

>> +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.

OK.

> Regards,
>
> --
> Julien Grall



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel