| 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 |
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
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
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
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
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 --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 */
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(+)