Re: [PATCH v4 16/30] xen/riscv: introduce p2m.h

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 05/02/2024 15:32, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V4:
>   - update the comment above p2m_type_t. RISC-V has only 2 free for use bits in 
> PTE, not 4 as Arm.
>   - update the comment after p2m_ram_rw: s/guest/domain/ as this also applies 
> for dom0.
>   - return INVALID_MFN in gfn_to_mfn() instead of mfn(0).
>   - drop PPC changes.
> ---
> 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/riscv/include/asm/p2m.h | 102 +++++++++++++++++++++++++++++++
>   1 file changed, 102 insertions(+)
>   create mode 100644 xen/arch/riscv/include/asm/p2m.h
>
> diff --git a/xen/arch/riscv/include/asm/p2m.h b/xen/arch/riscv/include/asm/p2m.h
> new file mode 100644
> index 0000000000..8ad020974f
> --- /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 2 bits.
That's not a lot and I expect you will ran out fairly quickly if you 
decide to store whether...
> + * So it's possible to only have 4 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 domain RAM */
... the RAM is Read-Write. Depend on your P2M implementation, you could 
rely on the HW page-attributes to augment you p2m_type. So effectively, 
your two bits would contain information you can't already store.
Anyway, your approach is ok as your aim is to only build Xen for now. 
BUt this likely want to be re-think once you add the P2M support.
> +} 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_ON("unimplemented");
> +    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 */
Coding style: I understansd this is what Arm did, but the style is not 
correct. Please add a space before and after <<.
> +
> +static inline struct page_info *get_page_from_gfn(
> +    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> +{
> +    BUG_ON("unimplemented");
> +    return NULL;
> +}
> +
> +static inline void memory_type_changed(struct domain *d)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +
> +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_ON("unimplemented");
> +    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_ON("unimplemented");
> +    return INVALID_MFN;
> +}
> +
> +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;
AFAICT, the current implementation of set_foreign_p2m_entry() is a 
BUG_ON(). So I think it would make sense to return 'false' as this 
reflects better the current state.
> +}
> +
> +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:
> + */
Cheers,

--
Julien Grall