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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Tue, 2023-03-28 at 16:44 +0100, Julien Grall wrote:
> 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.
Thanks for the suggestion. I'll take it into account.
> 
> Also, are you sure that all the CPU will be able to support more full
> 64-bit VA space?
I am not sure but based on Linux it looks like it is true. ( at least,
they use the same definitions for RV64 ).

I am curious how that can be checked.

~ Oleksii