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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan