Re: [PATCH v3 1/3] xen/riscv: introduce setup_initial_pages

[Julien Grall <julien@xxxxxxx>]

Hi,

On 28/03/2023 16:34, Jan Beulich wrote:
> On 27.03.2023 19:17, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -39,12 +39,25 @@
> >     name:
> >   #endif
> >   
> > -#define XEN_VIRT_START  _AT(UL, 0x80200000)
> > +#define ADDRESS_SPACE_END (_AC(-1, UL))
> > +#define SZ_1G             0x40000000
> No need for this - please use GB(1) (see xen/config.h) in its stead.
>
> > +#ifdef CONFIG_RISCV_64
> > +#define XEN_VIRT_START    (ADDRESS_SPACE_END - SZ_1G + 1)
> I first thought the " + 1" would be rendering the address misaligned.
> May I suggest (ADDRESS_SPACE_END + 1 - SZ_1G) to help avoiding this
> impression?
I will jump on this to say that I am finding quite difficult to review 
code using define/variable that contains "end" in their name because it 
is never clear whether this is inclusive or exclusive.
In this case, it is inclusive, but within the same patch I can see 
map_end which is exclusive.
For this case, my suggestion would be to remove ADDRESS_SPACE_END and 
just hardcode the value where you want to put Xen. For others, you could 
use (start, size) to describe a region.
Also, are you sure that all the CPU will be able to support more full 
64-bit VA space?
Cheers,

--
Julien Grall