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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.03.2023 13:47, Oleksii wrote:
> On Tue, 2023-03-28 at 16:44 +0100, Julien Grall wrote:
>> 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 ).

Hmm, the spec has this text in a rationalizing remark: "The mapping
between 64-bit virtual addresses and the 39-bit usable address space
of Sv39 is not based on zero-extension but instead follows an
entrenched convention that allows an OS to use one or a few of the
most-significant bits of a full-size (64-bit) virtual address to
quickly distinguish user and supervisor address regions."

Apart from that all descriptions look to assume that the top VA bits
are simply ignored for the purpose of translating to PA, which would
mean that any address within the 64-bit address space can be mapped,
and the same mapping would appear many times in VA space. I wonder
whether that's really how it's meant to be, or whether I'm overlooking
something, or whether the spec would want clarifying.

> I am curious how that can be checked.

Try it out somewhere?

Jan