Re: [Minios-devel] [PATCH 05/40] arm64: fix the wrong mask for to_virt/to_phys

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Shijie,

On 03/11/17 03:11, Huang Shijie wrote:
> In the arm64, the mask 0xffffffff will truncate the value, and
> to_virt/to_phys will get wrong results.
>
> By add a new macro, this patch fixes it.
I think it is "By adding a new macro".

> Change-Id: Icbd1d3ddd0d3f850e1a4944d67237f2c14e6a701
> Jira: ENTOS-247
> Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> ---
>   include/arm/arch_mm.h | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/arm/arch_mm.h b/include/arm/arch_mm.h
> index 7ce8dd8..6dfa61e 100644
> --- a/include/arm/arch_mm.h
> +++ b/include/arm/arch_mm.h
> @@ -16,8 +16,14 @@ extern paddr_t physical_address_offset;
>   
>   #define L1_PROT          0
>   
> -#define to_phys(x)                 (((paddr_t)(x)+physical_address_offset) & 0xffffffff)
> -#define to_virt(x)                 ((void *)(((x)-physical_address_offset) & 
> 0xffffffff))
I took me quite a while to understand how those 2 helpers can work on 
Arm32. They can only work with physical address < 4GB and I think 
because the offset (phys - virt) is always positive.
If it were negative, physical_address_offset been 32-bit would be 
promoted to 64-bit making the offset positive. Screwing everything.
Also, to_phys will work on 64-bit value whilst to_virt will work with 
what the user passes.
Overall, I think using static inline helper would be beneficial here.

> +#if defined(__aarch64__)
> +#define ADDR_MASK         0xffffffffffffffff
> +#else
> +#define ADDR_MASK         0xffffffff
This is hard to check the number of f is correct. How about using ~0UL 
or ~(vaddr_t)0/~(paddr_t)0 when it is relevant?
> +#endif
> +
> +#define to_phys(x)                 (((paddr_t)(x)-physical_address_offset) & 
> ADDR_MASK)
Why did you switch from an addition to a subtraction?

> +#define to_virt(x)                 ((void *)(((x)+physical_address_offset) & 
> ADDR_MASK))
Why did you switch from a subtraction to an addition?


>   #define PFN_UP(x)                  (unsigned long)(((x) + PAGE_SIZE-1) >> 
> PAGE_SHIFT)
>   #define PFN_DOWN(x)                (unsigned long)((x) >> PAGE_SHIFT)
Technically PFN_UP/PFN_DOWN should use (paddr_t) and not (unsigned long)

>
Cheers,

--
Julien Grall

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