Re: [Minios-devel] [PATCH v3 30/43] arm64: add a new helper ioremap

[Julien Grall <julien.grall@xxxxxxx>]

Hi Shijie,

On 16/04/18 07:32, Huang Shijie wrote:
> This patch adds a new helper : ioremap.
>
> This helper is used by the GIC mapping.
> The return address is got from the demand area.
I think you mean "the mapping will be allocated from the demand area".

Also, it would be nice you explain why you are moving from to_virt -> 
full mapping.
> Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
[...]

> @@ -234,6 +228,7 @@ void init_pagetable(unsigned long *start_pfn, unsigned long 
> base,
>   }
>   
>   static unsigned long virt_kernel_area_end;
> +static unsigned long virt_demand_area_end;
>   void arch_mm_preinit(void *dtb_pointer)
>   {
>       paddr_t **dtb_p = dtb_pointer;
> @@ -241,6 +236,7 @@ void arch_mm_preinit(void *dtb_pointer)
>       uintptr_t end = (uintptr_t) &_end;
>   
>       virt_kernel_area_end = VIRT_KERNEL_AREA;
> +    virt_demand_area_end = VIRT_DEMAND_AREA;
Should not this belong to arch_init_demand_mapping_area?

>   
>       dtb = to_virt(((paddr_t)dtb));
>       first_free_pfn = PFN_UP(to_phys(end));
> @@ -293,6 +289,36 @@ unsigned long map_frame_virt(unsigned long mfn)
>       return addr;
>   }
>   
> +unsigned long allocate_ondemand(unsigned long n, unsigned long alignment)
> +{
> +    unsigned long addr;
> +
> +    addr = virt_demand_area_end;
> +
> +    /* Just for simple, make it page aligned. */
What if the caller requires a different alignment? I was kind of 
expecting the parameter alignment to be used somehow in your algo.
> +    virt_demand_area_end += (n + PAGE_SIZE - 1) & PAGE_MASK; > +
> +    ASSERT(virt_demand_area_end <= VIRT_HEAP_AREA);
ASSERT is going to disappear in non-debug build. So is it the right 
thing to use? As the caller has no way to know whether there are free 
virtual space, it would be better to return 0 (as x86 does). So the 
caller is able to know something went wrong.
> +
> +    return addr;
> +}
> +
> +void *ioremap(paddr_t paddr, unsigned long size)
> +{
> +    unsigned long addr;
> +    int ret;
> +
> +    addr = allocate_ondemand(size, 1);
> +    if (!addr)
I know that the ARM port has been really messy in coding style, but 
please use the correct coding style in mini-OS. This should be:
if ( !addr )

> +        return NULL;
> +
> +    ret = build_pagetable(addr, PHYS_PFN(paddr), PFN_UP(size), MEM_DEV_ATTR,
> +                  init_pagetable_ok? alloc_new_page: early_alloc_page, 3);
> +    if (ret < 0)
Same here.

> +        return NULL;
> +    return (void*)addr;
> +}
> +
>   void arch_init_mm(unsigned long *start_pfn_p, unsigned long *max_pfn_p)
>   {
>       int memory;
> diff --git a/include/arm/arch_mm.h b/include/arm/arch_mm.h
> index 4f3fd8f..ad122e7 100644
> --- a/include/arm/arch_mm.h
> +++ b/include/arm/arch_mm.h
> @@ -7,6 +7,7 @@ typedef uint64_t paddr_t;
>   #define MAX_MEM_SIZE            (1UL << 39)
>   #define VIRT_KERNEL_AREA        ((unsigned long)to_virt(MAX_MEM_SIZE))
>   #define VIRT_DEMAND_AREA        (VIRT_KERNEL_AREA + MAX_MEM_SIZE)
> +#define VIRT_HEAP_AREA          (VIRT_DEMAND_AREA + MAX_MEM_SIZE)
I have asked that on a previous patch and going to repeat here. I am 
going to need more context about the meaning of MAX_MEM_SIZE here and 
why you need 2^39MB of on demand memory.
>   
>   typedef uint64_t lpae_t;
>   
> @@ -40,4 +41,5 @@ void arch_mm_preinit(void *dtb_pointer);
>   // FIXME
>   #define map_frames(f, n) (NULL)
>   
> +void *ioremap(paddr_t addr, unsigned long size);
>   #endif
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel