Re: [Minios-devel] [PATCH v3 08/43] arm64: fix the wrong mask for to_virt/to_phys

[Julien Grall <julien.grall@xxxxxxx>]

On 19/04/18 11:01, Huang Shijie wrote:
> On Wed, Apr 18, 2018 at 05:42:37PM +0100, Julien Grall wrote:
> > Hi,
> >
> > On 16/04/18 07:31, Huang Shijie wrote:
> > > In the arm64, the mask 0xffffffff will truncate the value, and
> > s/In the/On/
> >
> > > to_virt/to_phys will get wrong results.
> > >
> > > This patch fixes it.
> > >
> > > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > > ---
> > >   include/arm/arch_mm.h | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/arm/arch_mm.h b/include/arm/arch_mm.h
> > > index c66856c..f77a210 100644
> > > --- a/include/arm/arch_mm.h
> > > +++ b/include/arm/arch_mm.h
> > > @@ -15,8 +15,8 @@ extern paddr_t physical_address_offset;
> > >   #define DEF_PAGE_PROT     0
> > > -#define to_phys(x)                 (((paddr_t)(x)+physical_address_offset) & 
> > > 0xffffffff)
> > > -#define to_virt(x)                 ((void *)(((x)-physical_address_offset) & 
> > > 0xffffffff))
> > > +#define to_phys(x)                 (((paddr_t)(x)+physical_address_offset) & 
> > > (~0UL))
> > While this looks good on arm64, for arm32 unsigned long is not going to be
> > enough. So I think you want to use ~(paddr_t)0 to cover the 32-bit case.
> Sorry, I do not understand your meaning... :(
> Do we need to cover the arm32 here?
to_phys() and to_virt() are defined in generic arm headers. While 32-bit 
is not supported, it would be better to cover it as cast problem are 
really a pain to fix it afterwards.
But I just realized that mini-os only support 4G of RAM because it 
relies on mapping all the memory. Am I right?
If so, the UL might be fine. Any opinions?

Cheers,

--
Julien Grall

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