[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 21/34] xen/riscv: introduce p2m.h
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@xxxxxxxxx> > > --- > > 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |