| Message ID | c3b1f24aea1ba01505697717b240c8d036abfee1.1703255175.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
Hi Oleksii, On 12/22/23 9:13 AM, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V3: > - add SPDX > - drop unneeded for now p2m types. > - return false in all functions implemented with BUG() inside. > - update the commit message > --- > Changes in V2: > - Nothing changed. Only rebase. > --- > xen/arch/ppc/include/asm/p2m.h | 3 +- > xen/arch/riscv/include/asm/p2m.h | 102 +++++++++++++++++++++++++++++++ > 2 files changed, 103 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/riscv/include/asm/p2m.h > > diff --git a/xen/arch/ppc/include/asm/p2m.h b/xen/arch/ppc/include/asm/p2m.h > index 25ba054668..3bc05b7c05 100644 > --- a/xen/arch/ppc/include/asm/p2m.h > +++ b/xen/arch/ppc/include/asm/p2m.h > @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct domain *d) > static inline int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn, > unsigned int order) > { > - BUG_ON("unimplemented"); > - return 1; > + return -EOPNOTSUPP; > } > Was this change included by mistake? I'm not sure why this patch should touch PPC's p2m.h. Thanks, Shawn
Hi Oleksii, On 22/12/2023 15:13, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V3: > - add SPDX > - drop unneeded for now p2m types. > - return false in all functions implemented with BUG() inside. > - update the commit message > --- > Changes in V2: > - Nothing changed. Only rebase. > --- > xen/arch/ppc/include/asm/p2m.h | 3 +- > xen/arch/riscv/include/asm/p2m.h | 102 +++++++++++++++++++++++++++++++ > 2 files changed, 103 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/riscv/include/asm/p2m.h > > diff --git a/xen/arch/ppc/include/asm/p2m.h b/xen/arch/ppc/include/asm/p2m.h > index 25ba054668..3bc05b7c05 100644 > --- a/xen/arch/ppc/include/asm/p2m.h > +++ b/xen/arch/ppc/include/asm/p2m.h > @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct domain *d) > static inline int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn, > unsigned int order) > { > - BUG_ON("unimplemented"); > - return 1; > + return -EOPNOTSUPP; > } > > static inline int guest_physmap_add_entry(struct domain *d, > diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h > new file mode 100644 > index 0000000000..d270ef6635 > --- /dev/null > +++ b/xen/arch/riscv/include/asm/p2m.h > @@ -0,0 +1,102 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef __ASM_RISCV_P2M_H__ > +#define __ASM_RISCV_P2M_H__ > + > +#include <asm/page-bits.h> > + > +#define paddr_bits PADDR_BITS > + > +/* > + * List of possible type for each page in the p2m entry. > + * The number of available bit per page in the pte for this purpose is 4 bits. > + * So it's possible to only have 16 fields. If we run out of value in the > + * future, it's possible to use higher value for pseudo-type and don't store > + * them in the p2m entry. > + */ This looks like a verbatim copy from Arm. Did you actually check RISC-V has 4 bits available in the PTE to store this value? > +typedef enum { > + p2m_invalid = 0, /* Nothing mapped here */ > + p2m_ram_rw, /* Normal read/write guest RAM */ s/guest/domain/ as this also applies for dom0. > +} p2m_type_t; > + > +#include <xen/p2m-common.h> > + > +static inline int get_page_and_type(struct page_info *page, > + struct domain *domain, > + unsigned long type) > +{ > + BUG(); I understand your goal with the BUG() but I find it risky. This is not a problem right now, it is more when we will decide to have RISC-V supported. You will have to go through all the BUG() to figure out which one are warrant or not. To reduce the load, I would recommend to switch to ASSERT_UNREACHABLE() (or maybe introduced a different macro) that would lead to a crash on debug build but propagate the error normally on production build. Of course, if you can't propagate an error, then the right course of action is a BUG(). But I expect this case to be limited. [...] > +static inline mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn) > +{ > + BUG(); > + return _mfn(0); This wants to be INVALID_MFN. [...]
On 12.01.2024 11:39, Julien Grall wrote: > On 22/12/2023 15:13, Oleksii Kurochko wrote: >> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >> --- >> Changes in V3: >> - add SPDX >> - drop unneeded for now p2m types. >> - return false in all functions implemented with BUG() inside. >> - update the commit message >> --- >> Changes in V2: >> - Nothing changed. Only rebase. >> --- >> xen/arch/ppc/include/asm/p2m.h | 3 +- >> xen/arch/riscv/include/asm/p2m.h | 102 +++++++++++++++++++++++++++++++ >> 2 files changed, 103 insertions(+), 2 deletions(-) >> create mode 100644 xen/arch/riscv/include/asm/p2m.h >> >> diff --git a/xen/arch/ppc/include/asm/p2m.h b/xen/arch/ppc/include/asm/p2m.h >> index 25ba054668..3bc05b7c05 100644 >> --- a/xen/arch/ppc/include/asm/p2m.h >> +++ b/xen/arch/ppc/include/asm/p2m.h >> @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct domain *d) >> static inline int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn, >> unsigned int order) >> { >> - BUG_ON("unimplemented"); >> - return 1; >> + return -EOPNOTSUPP; >> } >> >> static inline int guest_physmap_add_entry(struct domain *d, >> diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h >> new file mode 100644 >> index 0000000000..d270ef6635 >> --- /dev/null >> +++ b/xen/arch/riscv/include/asm/p2m.h >> @@ -0,0 +1,102 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +#ifndef __ASM_RISCV_P2M_H__ >> +#define __ASM_RISCV_P2M_H__ >> + >> +#include <asm/page-bits.h> >> + >> +#define paddr_bits PADDR_BITS >> + >> +/* >> + * List of possible type for each page in the p2m entry. >> + * The number of available bit per page in the pte for this purpose is 4 bits. >> + * So it's possible to only have 16 fields. If we run out of value in the >> + * future, it's possible to use higher value for pseudo-type and don't store >> + * them in the p2m entry. >> + */ > > This looks like a verbatim copy from Arm. Did you actually check RISC-V > has 4 bits available in the PTE to store this value? > >> +typedef enum { >> + p2m_invalid = 0, /* Nothing mapped here */ >> + p2m_ram_rw, /* Normal read/write guest RAM */ > > s/guest/domain/ as this also applies for dom0. > >> +} p2m_type_t; >> + >> +#include <xen/p2m-common.h> >> + >> +static inline int get_page_and_type(struct page_info *page, >> + struct domain *domain, >> + unsigned long type) >> +{ >> + BUG(); > > I understand your goal with the BUG() but I find it risky. This is not a > problem right now, it is more when we will decide to have RISC-V > supported. You will have to go through all the BUG() to figure out which > one are warrant or not. > > To reduce the load, I would recommend to switch to ASSERT_UNREACHABLE() > (or maybe introduced a different macro) that would lead to a crash on > debug build but propagate the error normally on production build. Elsewhere BUG_ON("unimplemented") is used to indicate such cases. Can't this be used here (and then uniformly elsewhere) as well? Jan
Hi Jan, On 12/01/2024 11:06, Jan Beulich wrote: > On 12.01.2024 11:39, Julien Grall wrote: >> On 22/12/2023 15:13, Oleksii Kurochko wrote: >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes in V3: >>> - add SPDX >>> - drop unneeded for now p2m types. >>> - return false in all functions implemented with BUG() inside. >>> - update the commit message >>> --- >>> Changes in V2: >>> - Nothing changed. Only rebase. >>> --- >>> xen/arch/ppc/include/asm/p2m.h | 3 +- >>> xen/arch/riscv/include/asm/p2m.h | 102 +++++++++++++++++++++++++++++++ >>> 2 files changed, 103 insertions(+), 2 deletions(-) >>> create mode 100644 xen/arch/riscv/include/asm/p2m.h >>> >>> diff --git a/xen/arch/ppc/include/asm/p2m.h b/xen/arch/ppc/include/asm/p2m.h >>> index 25ba054668..3bc05b7c05 100644 >>> --- a/xen/arch/ppc/include/asm/p2m.h >>> +++ b/xen/arch/ppc/include/asm/p2m.h >>> @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct domain *d) >>> static inline int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn, >>> unsigned int order) >>> { >>> - BUG_ON("unimplemented"); >>> - return 1; >>> + return -EOPNOTSUPP; >>> } >>> >>> static inline int guest_physmap_add_entry(struct domain *d, >>> diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h >>> new file mode 100644 >>> index 0000000000..d270ef6635 >>> --- /dev/null >>> +++ b/xen/arch/riscv/include/asm/p2m.h >>> @@ -0,0 +1,102 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +#ifndef __ASM_RISCV_P2M_H__ >>> +#define __ASM_RISCV_P2M_H__ >>> + >>> +#include <asm/page-bits.h> >>> + >>> +#define paddr_bits PADDR_BITS >>> + >>> +/* >>> + * List of possible type for each page in the p2m entry. >>> + * The number of available bit per page in the pte for this purpose is 4 bits. >>> + * So it's possible to only have 16 fields. If we run out of value in the >>> + * future, it's possible to use higher value for pseudo-type and don't store >>> + * them in the p2m entry. >>> + */ >> >> This looks like a verbatim copy from Arm. Did you actually check RISC-V >> has 4 bits available in the PTE to store this value? >> >>> +typedef enum { >>> + p2m_invalid = 0, /* Nothing mapped here */ >>> + p2m_ram_rw, /* Normal read/write guest RAM */ >> >> s/guest/domain/ as this also applies for dom0. >> >>> +} p2m_type_t; >>> + >>> +#include <xen/p2m-common.h> >>> + >>> +static inline int get_page_and_type(struct page_info *page, >>> + struct domain *domain, >>> + unsigned long type) >>> +{ >>> + BUG(); >> >> I understand your goal with the BUG() but I find it risky. This is not a >> problem right now, it is more when we will decide to have RISC-V >> supported. You will have to go through all the BUG() to figure out which >> one are warrant or not. >> >> To reduce the load, I would recommend to switch to ASSERT_UNREACHABLE() >> (or maybe introduced a different macro) that would lead to a crash on >> debug build but propagate the error normally on production build. > > Elsewhere BUG_ON("unimplemented") is used to indicate such cases. > Can't this be used here (and then uniformly elsewhere) as well? I would prefer something that can be compiled out in production build. But I would be Ok with BUG_ON("...") this is at least clearer than a plain BUG().
On Thu, 2024-01-11 at 17:11 -0600, Shawn Anastasio wrote: > Hi Oleksii, > > On 12/22/23 9:13 AM, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V3: > > - add SPDX > > - drop unneeded for now p2m types. > > - return false in all functions implemented with BUG() inside. > > - update the commit message > > --- > > Changes in V2: > > - Nothing changed. Only rebase. > > --- > > xen/arch/ppc/include/asm/p2m.h | 3 +- > > xen/arch/riscv/include/asm/p2m.h | 102 > > +++++++++++++++++++++++++++++++ > > 2 files changed, 103 insertions(+), 2 deletions(-) > > create mode 100644 xen/arch/riscv/include/asm/p2m.h > > > > diff --git a/xen/arch/ppc/include/asm/p2m.h > > b/xen/arch/ppc/include/asm/p2m.h > > index 25ba054668..3bc05b7c05 100644 > > --- a/xen/arch/ppc/include/asm/p2m.h > > +++ b/xen/arch/ppc/include/asm/p2m.h > > @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct > > domain *d) > > static inline int guest_physmap_mark_populate_on_demand(struct > > domain *d, unsigned long gfn, > > unsigned > > int order) > > { > > - BUG_ON("unimplemented"); > > - return 1; > > + return -EOPNOTSUPP; > > } > > > > Was this change included by mistake? I'm not sure why this patch > should > touch PPC's p2m.h. I think you are right. It's mistake. RISC-V has the similar p2m.h so I faulty changed PPC version too. Thanks. ~ Oleksii
Hi Julien, On Fri, 2024-01-12 at 10:39 +0000, Julien Grall wrote: > Hi Oleksii, > > On 22/12/2023 15:13, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V3: > > - add SPDX > > - drop unneeded for now p2m types. > > - return false in all functions implemented with BUG() inside. > > - update the commit message > > --- > > Changes in V2: > > - Nothing changed. Only rebase. > > --- > > xen/arch/ppc/include/asm/p2m.h | 3 +- > > xen/arch/riscv/include/asm/p2m.h | 102 > > +++++++++++++++++++++++++++++++ > > 2 files changed, 103 insertions(+), 2 deletions(-) > > create mode 100644 xen/arch/riscv/include/asm/p2m.h > > > > diff --git a/xen/arch/ppc/include/asm/p2m.h > > b/xen/arch/ppc/include/asm/p2m.h > > index 25ba054668..3bc05b7c05 100644 > > --- a/xen/arch/ppc/include/asm/p2m.h > > +++ b/xen/arch/ppc/include/asm/p2m.h > > @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct > > domain *d) > > static inline int guest_physmap_mark_populate_on_demand(struct > > domain *d, unsigned long gfn, > > unsigned > > int order) > > { > > - BUG_ON("unimplemented"); > > - return 1; > > + return -EOPNOTSUPP; > > } > > > > static inline int guest_physmap_add_entry(struct domain *d, > > diff --git a/xen/arch/riscv/include/asm/p2m.h > > b/xen/arch/riscv/include/asm/p2m.h > > new file mode 100644 > > index 0000000000..d270ef6635 > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/p2m.h > > @@ -0,0 +1,102 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +#ifndef __ASM_RISCV_P2M_H__ > > +#define __ASM_RISCV_P2M_H__ > > + > > +#include <asm/page-bits.h> > > + > > +#define paddr_bits PADDR_BITS > > + > > +/* > > + * List of possible type for each page in the p2m entry. > > + * The number of available bit per page in the pte for this > > purpose is 4 bits. > > + * So it's possible to only have 16 fields. If we run out of value > > in the > > + * future, it's possible to use higher value for pseudo-type and > > don't store > > + * them in the p2m entry. > > + */ > > This looks like a verbatim copy from Arm. Did you actually check > RISC-V > has 4 bits available in the PTE to store this value? Thanks for noticing that, in RISC-V it is available only 2 bits ( bits 8 and 9), so I'll update the comment: 53 10 9 8 7 6 5 4 3 2 1 0 Physical Page Number RSV D A G U X W R V It seems that I missed something in the Arm code/architecture.As far as I recall, in Arm, bits 5-8 are ignored by the MMU, and they are expected to be used by the hypervisor for its purpose. However, in the code, I notice that these bits are utilized for storing a reference counter. Could you confirm if my understanding is correct? Additionally, I am curious about where the PTE bits are used to store one of the values of the enum `p2m_type_t`. > > > +typedef enum { > > + p2m_invalid = 0, /* Nothing mapped here */ > > + p2m_ram_rw, /* Normal read/write guest RAM */ > > s/guest/domain/ as this also applies for dom0. Thanks. I'll update that. > > > +} p2m_type_t; > > + > > +#include <xen/p2m-common.h> > > + > > +static inline int get_page_and_type(struct page_info *page, > > + struct domain *domain, > > + unsigned long type) > > +{ > > + BUG(); > > I understand your goal with the BUG() but I find it risky. This is > not a > problem right now, it is more when we will decide to have RISC-V > supported. You will have to go through all the BUG() to figure out > which > one are warrant or not. > > To reduce the load, I would recommend to switch to > ASSERT_UNREACHABLE() > (or maybe introduced a different macro) that would lead to a crash on > debug build but propagate the error normally on production build. > > Of course, if you can't propagate an error, then the right course of > action is a BUG(). But I expect this case to be limited. Thanks. I'm currently transitioning to using ASSERT_UNREACHABLE() or BUG_ON() throughout the codebase, and this is one of the instances where I overlooked the update. > > [...] > > > +static inline mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn) > > +{ > > + BUG(); > > + return _mfn(0); > > This wants to be INVALID_MFN. > > [...] > ~ Oleksii
On 15.01.2024 11:35, Oleksii wrote: > Hi Julien, > > On Fri, 2024-01-12 at 10:39 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> On 22/12/2023 15:13, Oleksii Kurochko wrote: >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes in V3: >>> - add SPDX >>> - drop unneeded for now p2m types. >>> - return false in all functions implemented with BUG() inside. >>> - update the commit message >>> --- >>> Changes in V2: >>> - Nothing changed. Only rebase. >>> --- >>> xen/arch/ppc/include/asm/p2m.h | 3 +- >>> xen/arch/riscv/include/asm/p2m.h | 102 >>> +++++++++++++++++++++++++++++++ >>> 2 files changed, 103 insertions(+), 2 deletions(-) >>> create mode 100644 xen/arch/riscv/include/asm/p2m.h >>> >>> diff --git a/xen/arch/ppc/include/asm/p2m.h >>> b/xen/arch/ppc/include/asm/p2m.h >>> index 25ba054668..3bc05b7c05 100644 >>> --- a/xen/arch/ppc/include/asm/p2m.h >>> +++ b/xen/arch/ppc/include/asm/p2m.h >>> @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct >>> domain *d) >>> static inline int guest_physmap_mark_populate_on_demand(struct >>> domain *d, unsigned long gfn, >>> unsigned >>> int order) >>> { >>> - BUG_ON("unimplemented"); >>> - return 1; >>> + return -EOPNOTSUPP; >>> } >>> >>> static inline int guest_physmap_add_entry(struct domain *d, >>> diff --git a/xen/arch/riscv/include/asm/p2m.h >>> b/xen/arch/riscv/include/asm/p2m.h >>> new file mode 100644 >>> index 0000000000..d270ef6635 >>> --- /dev/null >>> +++ b/xen/arch/riscv/include/asm/p2m.h >>> @@ -0,0 +1,102 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +#ifndef __ASM_RISCV_P2M_H__ >>> +#define __ASM_RISCV_P2M_H__ >>> + >>> +#include <asm/page-bits.h> >>> + >>> +#define paddr_bits PADDR_BITS >>> + >>> +/* >>> + * List of possible type for each page in the p2m entry. >>> + * The number of available bit per page in the pte for this >>> purpose is 4 bits. >>> + * So it's possible to only have 16 fields. If we run out of value >>> in the >>> + * future, it's possible to use higher value for pseudo-type and >>> don't store >>> + * them in the p2m entry. >>> + */ >> >> This looks like a verbatim copy from Arm. Did you actually check >> RISC-V >> has 4 bits available in the PTE to store this value? > Thanks for noticing that, in RISC-V it is available only 2 bits ( bits > 8 and 9), so I'll update the comment: > 53 10 9 8 7 6 5 4 3 2 1 0 > Physical Page Number RSV D A G U X W R V It's RSW (Reserved for Supervisor softWare use), not RSV, which is pretty important in this context. > It seems that I missed something in the Arm code/architecture.As far as I recall, in Arm, bits 5-8 are ignored by the MMU, and they > are expected > to be used by the hypervisor for its purpose. > However, in the code, I notice that these bits are utilized for storing > a reference counter. Why "however"? Hardware still is going to ignore these bits. Jan
On Mon, 2024-01-15 at 12:01 +0100, Jan Beulich wrote: > On 15.01.2024 11:35, Oleksii wrote: > > Hi Julien, > > > > On Fri, 2024-01-12 at 10:39 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > On 22/12/2023 15:13, Oleksii Kurochko wrote: > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > > > --- > > > > Changes in V3: > > > > - add SPDX > > > > - drop unneeded for now p2m types. > > > > - return false in all functions implemented with BUG() > > > > inside. > > > > - update the commit message > > > > --- > > > > Changes in V2: > > > > - Nothing changed. Only rebase. > > > > --- > > > > xen/arch/ppc/include/asm/p2m.h | 3 +- > > > > xen/arch/riscv/include/asm/p2m.h | 102 > > > > +++++++++++++++++++++++++++++++ > > > > 2 files changed, 103 insertions(+), 2 deletions(-) > > > > create mode 100644 xen/arch/riscv/include/asm/p2m.h > > > > > > > > diff --git a/xen/arch/ppc/include/asm/p2m.h > > > > b/xen/arch/ppc/include/asm/p2m.h > > > > index 25ba054668..3bc05b7c05 100644 > > > > --- a/xen/arch/ppc/include/asm/p2m.h > > > > +++ b/xen/arch/ppc/include/asm/p2m.h > > > > @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct > > > > domain *d) > > > > static inline int > > > > guest_physmap_mark_populate_on_demand(struct > > > > domain *d, unsigned long gfn, > > > > > > > > unsigned > > > > int order) > > > > { > > > > - BUG_ON("unimplemented"); > > > > - return 1; > > > > + return -EOPNOTSUPP; > > > > } > > > > > > > > static inline int guest_physmap_add_entry(struct domain *d, > > > > diff --git a/xen/arch/riscv/include/asm/p2m.h > > > > b/xen/arch/riscv/include/asm/p2m.h > > > > new file mode 100644 > > > > index 0000000000..d270ef6635 > > > > --- /dev/null > > > > +++ b/xen/arch/riscv/include/asm/p2m.h > > > > @@ -0,0 +1,102 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > > +#ifndef __ASM_RISCV_P2M_H__ > > > > +#define __ASM_RISCV_P2M_H__ > > > > + > > > > +#include <asm/page-bits.h> > > > > + > > > > +#define paddr_bits PADDR_BITS > > > > + > > > > +/* > > > > + * List of possible type for each page in the p2m entry. > > > > + * The number of available bit per page in the pte for this > > > > purpose is 4 bits. > > > > + * So it's possible to only have 16 fields. If we run out of > > > > value > > > > in the > > > > + * future, it's possible to use higher value for pseudo-type > > > > and > > > > don't store > > > > + * them in the p2m entry. > > > > + */ > > > > > > This looks like a verbatim copy from Arm. Did you actually check > > > RISC-V > > > has 4 bits available in the PTE to store this value? > > Thanks for noticing that, in RISC-V it is available only 2 bits ( > > bits > > 8 and 9), so I'll update the comment: > > 53 10 9 8 7 6 5 4 3 2 1 0 > > Physical Page Number RSV D A G U X W R V > > It's RSW (Reserved for Supervisor softWare use), not RSV, which is > pretty > important in this context. Yes, you are right it is RSW. Thanks for the correction. > > > It seems that I missed something in the Arm code/architecture.As > > far as I recall, in Arm, bits 5-8 are ignored by the MMU, and they > > are expected > > to be used by the hypervisor for its purpose. > > However, in the code, I notice that these bits are utilized for > > storing > > a reference counter. > > Why "however"? Hardware still is going to ignore these bits. Sure, these bits are ignored by hardware. What I meant is that, according to the code, these bits are used for storing a reference counter, not p2m_type_t. I guess I am missing something... ~ Oleksii
Hi Oleksii, On 16/01/2024 09:44, Oleksii wrote: > On Mon, 2024-01-15 at 12:01 +0100, Jan Beulich wrote: >> On 15.01.2024 11:35, Oleksii wrote: >>> Hi Julien, >>> >>> On Fri, 2024-01-12 at 10:39 +0000, Julien Grall wrote: >>>> Hi Oleksii, >>>> >>>> On 22/12/2023 15:13, Oleksii Kurochko wrote: >>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>>>> --- >>>>> Changes in V3: >>>>> - add SPDX >>>>> - drop unneeded for now p2m types. >>>>> - return false in all functions implemented with BUG() >>>>> inside. >>>>> - update the commit message >>>>> --- >>>>> Changes in V2: >>>>> - Nothing changed. Only rebase. >>>>> --- >>>>> xen/arch/ppc/include/asm/p2m.h | 3 +- >>>>> xen/arch/riscv/include/asm/p2m.h | 102 >>>>> +++++++++++++++++++++++++++++++ >>>>> 2 files changed, 103 insertions(+), 2 deletions(-) >>>>> create mode 100644 xen/arch/riscv/include/asm/p2m.h >>>>> >>>>> diff --git a/xen/arch/ppc/include/asm/p2m.h >>>>> b/xen/arch/ppc/include/asm/p2m.h >>>>> index 25ba054668..3bc05b7c05 100644 >>>>> --- a/xen/arch/ppc/include/asm/p2m.h >>>>> +++ b/xen/arch/ppc/include/asm/p2m.h >>>>> @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct >>>>> domain *d) >>>>> static inline int >>>>> guest_physmap_mark_populate_on_demand(struct >>>>> domain *d, unsigned long gfn, >>>>> >>>>> unsigned >>>>> int order) >>>>> { >>>>> - BUG_ON("unimplemented"); >>>>> - return 1; >>>>> + return -EOPNOTSUPP; >>>>> } >>>>> >>>>> static inline int guest_physmap_add_entry(struct domain *d, >>>>> diff --git a/xen/arch/riscv/include/asm/p2m.h >>>>> b/xen/arch/riscv/include/asm/p2m.h >>>>> new file mode 100644 >>>>> index 0000000000..d270ef6635 >>>>> --- /dev/null >>>>> +++ b/xen/arch/riscv/include/asm/p2m.h >>>>> @@ -0,0 +1,102 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>>> +#ifndef __ASM_RISCV_P2M_H__ >>>>> +#define __ASM_RISCV_P2M_H__ >>>>> + >>>>> +#include <asm/page-bits.h> >>>>> + >>>>> +#define paddr_bits PADDR_BITS >>>>> + >>>>> +/* >>>>> + * List of possible type for each page in the p2m entry. >>>>> + * The number of available bit per page in the pte for this >>>>> purpose is 4 bits. >>>>> + * So it's possible to only have 16 fields. If we run out of >>>>> value >>>>> in the >>>>> + * future, it's possible to use higher value for pseudo-type >>>>> and >>>>> don't store >>>>> + * them in the p2m entry. >>>>> + */ >>>> >>>> This looks like a verbatim copy from Arm. Did you actually check >>>> RISC-V >>>> has 4 bits available in the PTE to store this value? >>> Thanks for noticing that, in RISC-V it is available only 2 bits ( >>> bits >>> 8 and 9), so I'll update the comment: >>> 53 10 9 8 7 6 5 4 3 2 1 0 >>> Physical Page Number RSV D A G U X W R V >> >> It's RSW (Reserved for Supervisor softWare use), not RSV, which is >> pretty >> important in this context. > Yes, you are right it is RSW. Thanks for the correction. > >> >>> It seems that I missed something in the Arm code/architecture.As >>> far as I recall, in Arm, bits 5-8 are ignored by the MMU, and they >>> are expected >>> to be used by the hypervisor for its purpose. >>> However, in the code, I notice that these bits are utilized for >>> storing >>> a reference counter. >> >> Why "however"? Hardware still is going to ignore these bits. > Sure, these bits are ignored by hardware. What I meant is that, > according to the code, these bits are used for storing a reference > counter, not p2m_type_t. I guess I am missing something... I can only guess where you saw the field used for reference counting. This was the domain map page infrastruture, right? If so, this is for stage-1 page-table (aka hypervisor table) and not the stage-2 (e.g. P2M). For the latter, we would use the p2m_type_t. Cheers,
Hi Julien, On Tue, 2024-01-16 at 17:12 +0000, Julien Grall wrote: > Hi Oleksii, > > On 16/01/2024 09:44, Oleksii wrote: > > On Mon, 2024-01-15 at 12:01 +0100, Jan Beulich wrote: > > > On 15.01.2024 11:35, Oleksii wrote: > > > > Hi Julien, > > > > > > > > On Fri, 2024-01-12 at 10:39 +0000, Julien Grall wrote: > > > > > Hi Oleksii, > > > > > > > > > > On 22/12/2023 15:13, Oleksii Kurochko wrote: > > > > > > Signed-off-by: Oleksii Kurochko > > > > > > <oleksii.kurochko@gmail.com> > > > > > > --- > > > > > > Changes in V3: > > > > > > - add SPDX > > > > > > - drop unneeded for now p2m types. > > > > > > - return false in all functions implemented with BUG() > > > > > > inside. > > > > > > - update the commit message > > > > > > --- > > > > > > Changes in V2: > > > > > > - Nothing changed. Only rebase. > > > > > > --- > > > > > > xen/arch/ppc/include/asm/p2m.h | 3 +- > > > > > > xen/arch/riscv/include/asm/p2m.h | 102 > > > > > > +++++++++++++++++++++++++++++++ > > > > > > 2 files changed, 103 insertions(+), 2 deletions(-) > > > > > > create mode 100644 xen/arch/riscv/include/asm/p2m.h > > > > > > > > > > > > diff --git a/xen/arch/ppc/include/asm/p2m.h > > > > > > b/xen/arch/ppc/include/asm/p2m.h > > > > > > index 25ba054668..3bc05b7c05 100644 > > > > > > --- a/xen/arch/ppc/include/asm/p2m.h > > > > > > +++ b/xen/arch/ppc/include/asm/p2m.h > > > > > > @@ -50,8 +50,7 @@ static inline void > > > > > > memory_type_changed(struct > > > > > > domain *d) > > > > > > static inline int > > > > > > guest_physmap_mark_populate_on_demand(struct > > > > > > domain *d, unsigned long gfn, > > > > > > > > > > > > unsigned > > > > > > int order) > > > > > > { > > > > > > - BUG_ON("unimplemented"); > > > > > > - return 1; > > > > > > + return -EOPNOTSUPP; > > > > > > } > > > > > > > > > > > > static inline int guest_physmap_add_entry(struct domain > > > > > > *d, > > > > > > diff --git a/xen/arch/riscv/include/asm/p2m.h > > > > > > b/xen/arch/riscv/include/asm/p2m.h > > > > > > new file mode 100644 > > > > > > index 0000000000..d270ef6635 > > > > > > --- /dev/null > > > > > > +++ b/xen/arch/riscv/include/asm/p2m.h > > > > > > @@ -0,0 +1,102 @@ > > > > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > > > > +#ifndef __ASM_RISCV_P2M_H__ > > > > > > +#define __ASM_RISCV_P2M_H__ > > > > > > + > > > > > > +#include <asm/page-bits.h> > > > > > > + > > > > > > +#define paddr_bits PADDR_BITS > > > > > > + > > > > > > +/* > > > > > > + * List of possible type for each page in the p2m entry. > > > > > > + * The number of available bit per page in the pte for > > > > > > this > > > > > > purpose is 4 bits. > > > > > > + * So it's possible to only have 16 fields. If we run out > > > > > > of > > > > > > value > > > > > > in the > > > > > > + * future, it's possible to use higher value for pseudo- > > > > > > type > > > > > > and > > > > > > don't store > > > > > > + * them in the p2m entry. > > > > > > + */ > > > > > > > > > > This looks like a verbatim copy from Arm. Did you actually > > > > > check > > > > > RISC-V > > > > > has 4 bits available in the PTE to store this value? > > > > Thanks for noticing that, in RISC-V it is available only 2 bits > > > > ( > > > > bits > > > > 8 and 9), so I'll update the comment: > > > > 53 10 9 8 7 6 5 4 3 2 1 0 > > > > Physical Page Number RSV D A G U X W R V > > > > > > It's RSW (Reserved for Supervisor softWare use), not RSV, which > > > is > > > pretty > > > important in this context. > > Yes, you are right it is RSW. Thanks for the correction. > > > > > > > > > It seems that I missed something in the Arm > > > > code/architecture.As > > > > far as I recall, in Arm, bits 5-8 are ignored by the MMU, and > > > > they > > > > are expected > > > > to be used by the hypervisor for its purpose. > > > > However, in the code, I notice that these bits are utilized for > > > > storing > > > > a reference counter. > > > > > > Why "however"? Hardware still is going to ignore these bits. > > Sure, these bits are ignored by hardware. What I meant is that, > > according to the code, these bits are used for storing a reference > > counter, not p2m_type_t. I guess I am missing something... > > I can only guess where you saw the field used for reference counting. > This was the domain map page infrastruture, right? Yes, you are right. > > If so, this is for stage-1 page-table (aka hypervisor table) and not > the > stage-2 (e.g. P2M). For the latter, we would use the p2m_type_t. I confused stage-1 & stage-2. Now everything fell into place. Thanks. ~ Oleksii
diff --git a/xen/arch/ppc/include/asm/p2m.h b/xen/arch/ppc/include/asm/p2m.h index 25ba054668..3bc05b7c05 100644 --- a/xen/arch/ppc/include/asm/p2m.h +++ b/xen/arch/ppc/include/asm/p2m.h @@ -50,8 +50,7 @@ static inline void memory_type_changed(struct domain *d) static inline int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn, unsigned int order) { - BUG_ON("unimplemented"); - return 1; + return -EOPNOTSUPP; } static inline int guest_physmap_add_entry(struct domain *d, diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h new file mode 100644 index 0000000000..d270ef6635 --- /dev/null +++ b/xen/arch/riscv/include/asm/p2m.h @@ -0,0 +1,102 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __ASM_RISCV_P2M_H__ +#define __ASM_RISCV_P2M_H__ + +#include <asm/page-bits.h> + +#define paddr_bits PADDR_BITS + +/* + * List of possible type for each page in the p2m entry. + * The number of available bit per page in the pte for this purpose is 4 bits. + * So it's possible to only have 16 fields. If we run out of value in the + * future, it's possible to use higher value for pseudo-type and don't store + * them in the p2m entry. + */ +typedef enum { + p2m_invalid = 0, /* Nothing mapped here */ + p2m_ram_rw, /* Normal read/write guest RAM */ +} p2m_type_t; + +#include <xen/p2m-common.h> + +static inline int get_page_and_type(struct page_info *page, + struct domain *domain, + unsigned long type) +{ + BUG(); + return -EINVAL; +} + +/* Look up a GFN and take a reference count on the backing page. */ +typedef unsigned int p2m_query_t; +#define P2M_ALLOC (1u<<0) /* Populate PoD and paged-out entries */ +#define P2M_UNSHARE (1u<<1) /* Break CoW sharing */ + +static inline struct page_info *get_page_from_gfn( + struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) +{ + BUG(); + return NULL; +} + +static inline void memory_type_changed(struct domain *d) +{ + BUG(); +} + + +static inline int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn, + unsigned int order) +{ + return -EOPNOTSUPP; +} + +static inline int guest_physmap_add_entry(struct domain *d, + gfn_t gfn, + mfn_t mfn, + unsigned long page_order, + p2m_type_t t) +{ + BUG(); + return -EINVAL; +} + +/* Untyped version for RAM only, for compatibility */ +static inline int __must_check +guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn, + unsigned int page_order) +{ + return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw); +} + +static inline mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn) +{ + BUG(); + return _mfn(0); +} + +static inline bool arch_acquire_resource_check(struct domain *d) +{ + /* + * The reference counting of foreign entries in set_foreign_p2m_entry() + * is supported on RISCV. + */ + return true; +} + +static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) +{ + /* Not supported on RISCV. */ +} + +#endif /* __ASM_RISCV_P2M_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: - add SPDX - drop unneeded for now p2m types. - return false in all functions implemented with BUG() inside. - update the commit message --- Changes in V2: - Nothing changed. Only rebase. --- xen/arch/ppc/include/asm/p2m.h | 3 +- xen/arch/riscv/include/asm/p2m.h | 102 +++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 xen/arch/riscv/include/asm/p2m.h