Re: [PATCH v5 2/4] xen/riscv: introduce setup_initial_pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2023 22:05, Oleksii wrote:
> Hi Jan,
> 
> On Mon, 2023-04-24 at 17:35 +0200, Jan Beulich wrote:
>> On 24.04.2023 17:16, Oleksii wrote:
>>> On Mon, 2023-04-24 at 12:18 +0200, Jan Beulich wrote:
>>>> On 21.04.2023 18:01, Oleksii wrote:
>>>>> On Thu, 2023-04-20 at 16:36 +0200, Jan Beulich wrote:
>>>>>> On 19.04.2023 17:42, Oleksii Kurochko wrote:
>>>>>>> +    csr_write(CSR_SATP,
>>>>>>> +              ((unsigned long)stage1_pgtbl_root >>
>>>>>>> PAGE_SHIFT)
>>>>>>>>
>>>>>>> +              satp_mode << SATP_MODE_SHIFT);
>>>>>>> +
>>>>>>> +    if ( (csr_read(CSR_SATP) >> SATP_MODE_SHIFT) ==
>>>>>>> satp_mode
>>>>>>> )
>>>>>>> +        is_mode_supported = true;
>>>>>>> +
>>>>>>> +    /* Clean MMU root page table and disable MMU */
>>>>>>> +    stage1_pgtbl_root[index] = paddr_to_pte(0x0, 0x0);
>>>>>>> +
>>>>>>> +    csr_write(CSR_SATP, 0);
>>>>>>> +    asm volatile("sfence.vma");
>>>>>>
>>>>>> I guess what you do in this function could do with some more
>>>>>> comments.
>>>>>> Looks like you're briefly enabling the MMU to check that what
>>>>>> you
>>>>>> wrote
>>>>>> to SATP you can also read back. (Isn't there a register
>>>>>> reporting
>>>>>> whether the feature is available?)
>>>>> I supposed that it has to be but I couldn't find a register in
>>>>> docs.
>>>>
>>>> Well, yes, interestingly the register is marked WARL, so
>>>> apparently
>>>> intended to by used for probing like you do. (I find the
>>>> definition
>>>> of
>>>> WARL a little odd though, as such writes supposedly aren't
>>>> necessarily
>>>> value preserving. For SATP this might mean that translation is
>>>> enabled
>>>> by a write of an unsupported mode, with a different number of
>>>> levels.
>>>> This isn't going to work very well, I'm afraid.)
>>> Agree. It will be an issue in case of a different number of levels.
>>>
>>> Then it looks there is no way to check if SATP mode is supported.
>>>
>>> So we have to rely on the fact that the developer specified
>>> RV_STAGE1_MODE correctly in the config file.
>>
>> Well, maybe the spec could be clarified in this regard. That WARL
>> behavior may be okay for some registers, but as said I think it isn't
>> enough of a guarantee for SATP probing. Alistair, Bob - any thoughts?
> I've re-read the manual regarding CSR_SATP and the code of detecting
> SATP mode will work fine.
> From the manual ( 4.1.11
> Supervisor Address Translation and Protection (satp) Register ):
> “Implementations are not required to support all MODE settings, and if
> satp is written with an unsupported MODE, the entire write has no
> effect; no fields in satp are modified.”

Ah, I see. That's the sentence I had overlooked (and that's unhelpfully
not only not implied to be that way, but actively implied to be different
by figures 4.11 and 4.12 naming [all] the individual fields as WARL).

Jan