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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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.”

So there is no leave any open question so I'll provide new patch
series.

~ Oleksii