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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Fri, Nov 03, 2017 at 03:03:16PM +0000, Julien Grall wrote:
> 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?
okay.

> 
> > +#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?

I do not have an arm32 platform. So I can add #ifdef for the
to_phys/to_virt, such as:

#ifdef __arm__
#define to_phys(x)                 (((paddr_t)(x)+physical_address_offset) & 
0xffffffff)
#define to_virt(x)                 ((void *)(((x)-physical_address_offset) & 
0xffffffff))
#ifdef __aarch64__
#define to_phys(x)                 (((paddr_t)(x)-physical_address_offset) & 
ADDR_MASK)
#define to_virt(x)                 ((void *)(((x)+physical_address_offset) & 
ADDR_MASK))
#endif

What about this solution?

Thanks
Huang Shijie


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