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.
+ */
+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:
+ */
-- 
2.43.0

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.

[...]


-- 
Julien Grall

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,

-- 
Julien Grall

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

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().

-- 
Julien Grall

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

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