diff mbox series

[v2,32/39] xen/riscv: add minimal stuff to asm/page.h to build full Xen

Message ID 34a4bc023eb50e1d1cf70fa149825c51f2f4555f.1700761381.git.oleksii.kurochko@gmail.com (mailing list archive)
State Superseded
Headers show
Series Enable build of full Xen for RISC-V | expand

Commit Message

Oleksii Kurochko Nov. 24, 2023, 10:30 a.m. UTC
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - Nothing changed. Only rebase.
---
 xen/arch/riscv/include/asm/page.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Jan Beulich Dec. 14, 2023, 3:57 p.m. UTC | #1
On 24.11.2023 11:30, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

I wonder though ...

> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -6,6 +6,7 @@
>  #ifndef __ASSEMBLY__
>  
>  #include <xen/const.h>
> +#include <xen/bug.h>
>  #include <xen/types.h>
>  
>  #include <asm/mm.h>
> @@ -32,6 +33,9 @@
>  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
>  #define PTE_TABLE                   (PTE_VALID)
>  
> +/* TODO */
> +#define PAGE_HYPERVISOR 0

... whether this couldn't be defined properly right away.

Jan
Oleksii Kurochko Dec. 18, 2023, 10:45 a.m. UTC | #2
On Thu, 2023-12-14 at 16:57 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> I wonder though ...
> 
> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -6,6 +6,7 @@
> >  #ifndef __ASSEMBLY__
> >  
> >  #include <xen/const.h>
> > +#include <xen/bug.h>
> >  #include <xen/types.h>
> >  
> >  #include <asm/mm.h>
> > @@ -32,6 +33,9 @@
> >  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE |
> > PTE_WRITABLE)
> >  #define PTE_TABLE                   (PTE_VALID)
> >  
> > +/* TODO */
> > +#define PAGE_HYPERVISOR 0
> 
> ... whether this couldn't be defined properly right away.
It can be introduced now but it requires some additional defines to be
introduced in the same time:

#define _PAGE_W_BIT     2
#define _PAGE_XN_BIT    3
#define _PAGE_RO_BIT    1
#define _PAGE_XN        (1U << _PAGE_XN_BIT)
#define _PAGE_RO        (1U << _PAGE_RO_BIT)
#define _PAGE_W         (1U << _PAGE_W_BIT)

...
/*
 * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
 * meant to be used outside of this header.
 */
// #define _PAGE_DEVICE    _PAGE_XN
#define _PAGE_NORMAL    _PAGE_PRESENT

#define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL | _PAGE_RO | _PAGE_XN |
_PAGE_W)

#define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW

And _PAGE_PRESENT in pgtbl-bits.h:

#define _PAGE_PRESENT   (1 << 0)

I prefer to introduce all this things when it will be really used.

~ Oleksii
Jan Beulich Dec. 18, 2023, 11:36 a.m. UTC | #3
On 18.12.2023 11:45, Oleksii wrote:
> On Thu, 2023-12-14 at 16:57 +0100, Jan Beulich wrote:
>> On 24.11.2023 11:30, Oleksii Kurochko wrote:
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> I wonder though ...
>>
>>> --- a/xen/arch/riscv/include/asm/page.h
>>> +++ b/xen/arch/riscv/include/asm/page.h
>>> @@ -6,6 +6,7 @@
>>>  #ifndef __ASSEMBLY__
>>>  
>>>  #include <xen/const.h>
>>> +#include <xen/bug.h>
>>>  #include <xen/types.h>
>>>  
>>>  #include <asm/mm.h>
>>> @@ -32,6 +33,9 @@
>>>  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE |
>>> PTE_WRITABLE)
>>>  #define PTE_TABLE                   (PTE_VALID)
>>>  
>>> +/* TODO */
>>> +#define PAGE_HYPERVISOR 0
>>
>> ... whether this couldn't be defined properly right away.
> It can be introduced now but it requires some additional defines to be
> introduced in the same time:
> 
> #define _PAGE_W_BIT     2
> #define _PAGE_XN_BIT    3
> #define _PAGE_RO_BIT    1
> #define _PAGE_XN        (1U << _PAGE_XN_BIT)
> #define _PAGE_RO        (1U << _PAGE_RO_BIT)
> #define _PAGE_W         (1U << _PAGE_W_BIT)

Why would you need these, when you already have
PTE_{READABLE,WRITABLE,EXECUTABLE} just out of context from above? (And
that's aside from me (a) not following the naming (vs their purpose) and
(b) not seeing what _PAGE_*_BIT are needed for, not even thinking about
the leading underscores in these identifiers again.)

> ...
> /*
>  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
>  * meant to be used outside of this header.
>  */
> // #define _PAGE_DEVICE    _PAGE_XN
> #define _PAGE_NORMAL    _PAGE_PRESENT
> 
> #define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL | _PAGE_RO | _PAGE_XN |
> _PAGE_W)
> 
> #define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> 
> And _PAGE_PRESENT in pgtbl-bits.h:
> 
> #define _PAGE_PRESENT   (1 << 0)
> 
> I prefer to introduce all this things when it will be really used.

I understand that, yet for easy things it may help doing them right
away, rather than leaving them to be touched (in a straightforward way)
again very soon.

Jan
Oleksii Kurochko Dec. 18, 2023, 11:57 a.m. UTC | #4
On Mon, 2023-12-18 at 12:36 +0100, Jan Beulich wrote:
> On 18.12.2023 11:45, Oleksii wrote:
> > On Thu, 2023-12-14 at 16:57 +0100, Jan Beulich wrote:
> > > On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > > 
> > > Acked-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > I wonder though ...
> > > 
> > > > --- a/xen/arch/riscv/include/asm/page.h
> > > > +++ b/xen/arch/riscv/include/asm/page.h
> > > > @@ -6,6 +6,7 @@
> > > >  #ifndef __ASSEMBLY__
> > > >  
> > > >  #include <xen/const.h>
> > > > +#include <xen/bug.h>
> > > >  #include <xen/types.h>
> > > >  
> > > >  #include <asm/mm.h>
> > > > @@ -32,6 +33,9 @@
> > > >  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE
> > > > |
> > > > PTE_WRITABLE)
> > > >  #define PTE_TABLE                   (PTE_VALID)
> > > >  
> > > > +/* TODO */
> > > > +#define PAGE_HYPERVISOR 0
> > > 
> > > ... whether this couldn't be defined properly right away.
> > It can be introduced now but it requires some additional defines to
> > be
> > introduced in the same time:
> > 
> > #define _PAGE_W_BIT     2
> > #define _PAGE_XN_BIT    3
> > #define _PAGE_RO_BIT    1
> > #define _PAGE_XN        (1U << _PAGE_XN_BIT)
> > #define _PAGE_RO        (1U << _PAGE_RO_BIT)
> > #define _PAGE_W         (1U << _PAGE_W_BIT)
> 
> Why would you need these, when you already have
> PTE_{READABLE,WRITABLE,EXECUTABLE} just out of context from above?
I thought that the hypervisor page table is fully software-related, and
that's why a separate PAGE*BIT was introduced. These bits can be
different from PTE* bits, which are hardware-related
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/include/asm/page.h?ref_type=heads#L66

It looks like I misunderstood that, and PTE* can be used everywhere
instead of _PAGE*. Alternatively, we could consider renaming everything
to PAGE* to maintain consistency across architectures.

Does it make sense?


> (And
> that's aside from me (a) not following the naming (vs their purpose)
> and
> (b) not seeing what _PAGE_*_BIT are needed for, not even thinking
> about
> the leading underscores in these identifiers again.)
> 
> > ...
> > /*
> >  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are
> > not
> >  * meant to be used outside of this header.
> >  */
> > // #define _PAGE_DEVICE    _PAGE_XN
> > #define _PAGE_NORMAL    _PAGE_PRESENT
> > 
> > #define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL | _PAGE_RO | _PAGE_XN
> > |
> > _PAGE_W)
> > 
> > #define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> > 
> > And _PAGE_PRESENT in pgtbl-bits.h:
> > 
> > #define _PAGE_PRESENT   (1 << 0)
> > 
> > I prefer to introduce all this things when it will be really used.
> 
> I understand that, yet for easy things it may help doing them right
> away, rather than leaving them to be touched (in a straightforward
> way)
> again very soon.
> 
~ Oleksii
Jan Beulich Dec. 18, 2023, 12:05 p.m. UTC | #5
On 18.12.2023 12:57, Oleksii wrote:
> On Mon, 2023-12-18 at 12:36 +0100, Jan Beulich wrote:
>> On 18.12.2023 11:45, Oleksii wrote:
>>> On Thu, 2023-12-14 at 16:57 +0100, Jan Beulich wrote:
>>>> On 24.11.2023 11:30, Oleksii Kurochko wrote:
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I wonder though ...
>>>>
>>>>> --- a/xen/arch/riscv/include/asm/page.h
>>>>> +++ b/xen/arch/riscv/include/asm/page.h
>>>>> @@ -6,6 +6,7 @@
>>>>>  #ifndef __ASSEMBLY__
>>>>>  
>>>>>  #include <xen/const.h>
>>>>> +#include <xen/bug.h>
>>>>>  #include <xen/types.h>
>>>>>  
>>>>>  #include <asm/mm.h>
>>>>> @@ -32,6 +33,9 @@
>>>>>  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE
>>>>> |
>>>>> PTE_WRITABLE)
>>>>>  #define PTE_TABLE                   (PTE_VALID)
>>>>>  
>>>>> +/* TODO */
>>>>> +#define PAGE_HYPERVISOR 0
>>>>
>>>> ... whether this couldn't be defined properly right away.
>>> It can be introduced now but it requires some additional defines to
>>> be
>>> introduced in the same time:
>>>
>>> #define _PAGE_W_BIT     2
>>> #define _PAGE_XN_BIT    3
>>> #define _PAGE_RO_BIT    1
>>> #define _PAGE_XN        (1U << _PAGE_XN_BIT)
>>> #define _PAGE_RO        (1U << _PAGE_RO_BIT)
>>> #define _PAGE_W         (1U << _PAGE_W_BIT)
>>
>> Why would you need these, when you already have
>> PTE_{READABLE,WRITABLE,EXECUTABLE} just out of context from above?
> I thought that the hypervisor page table is fully software-related, and
> that's why a separate PAGE*BIT was introduced. These bits can be
> different from PTE* bits, which are hardware-related
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/include/asm/page.h?ref_type=heads#L66
> 
> It looks like I misunderstood that, and PTE* can be used everywhere
> instead of _PAGE*. Alternatively, we could consider renaming everything
> to PAGE* to maintain consistency across architectures.
> 
> Does it make sense?

Sure. Whether renaming makes sense is harder to tell though. It would
be only the name prefix that's uniform, as how exactly e.g. permissions
are controlled is arch-specific anyway. The one place I'm aware where
PAGE_* (or, sadly, still _PAGE_*) would matter for common code is
_PAGE_NONE (not sure though whether that's something that can / wants
to be expressed for RISC-V).

Jan
diff mbox series

Patch

diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 95074e29b3..abbae75aaf 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -6,6 +6,7 @@ 
 #ifndef __ASSEMBLY__
 
 #include <xen/const.h>
+#include <xen/bug.h>
 #include <xen/types.h>
 
 #include <asm/mm.h>
@@ -32,6 +33,9 @@ 
 #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
 #define PTE_TABLE                   (PTE_VALID)
 
+/* TODO */
+#define PAGE_HYPERVISOR 0
+
 /* Calculate the offsets into the pagetables for a given VA */
 #define pt_linear_offset(lvl, va)   ((va) >> XEN_PT_LEVEL_SHIFT(lvl))
 
@@ -46,6 +50,9 @@  typedef struct {
 #endif
 } pte_t;
 
+#define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
+#define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
+
 static inline pte_t paddr_to_pte(paddr_t paddr,
                                  unsigned int permissions)
 {
@@ -62,6 +69,20 @@  static inline bool pte_is_valid(pte_t p)
     return p.pte & PTE_VALID;
 }
 
+static inline void invalidate_icache(void)
+{
+    BUG();
+}
+
+#define clear_page(page) memset((void *)(page), 0, PAGE_SIZE)
+#define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
+
+/* TODO: Flush the dcache for an entire page. */
+static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
+{
+    BUG();
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PAGE_H */