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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-29 at 14:20 +0200, Jan Beulich wrote:
> 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.
> 
You are right the top VA bits are ignored but my understanding is that
you can't map any address within 64-bit address space as the top bits
are ignored. Thereby address 0xFFFFFFXXXXXXXXXX are equal to
0x000000XXXXXXXXXX from HW point of view and the top VA bits are used
only by developer.

> > I am curious how that can be checked.
> 
> Try it out somewhere?
I re-read the initial question and also your answer and it looks like I
misunderstood a little bit the initial question.

So if the question was about if all CPUs support all MMU addressing
modes ( Sv32, Sv39, Sv48, Sv57 ) then an answer is no. There is a rule
that if ,for example, CPU supports Sv57 then it must support lower
modes.


If I understood the question correctly then we can add function which
will write/read SATP_MODE:
    csr_write(CSR_SATP,
              ((unsigned long)stage1_pgtbl_root >> PAGE_SHIFT) |
RV_STAGE1_MODE << SATP_MODE_SHIFT);     __sfence_vma_all();
        csr_write(CSR_SATP, satp);
        if ((csr_read(CSR_SATP) >> SATP_MODE_SHIFT) == SATP_MODE_SV57)
        {
                max_supported_mode = SATP_MODE_SV57;
                return
        }
        ... check other modes ...

~ Oleksii