Re: [PATCH v3 21/34] xen/riscv: introduce p2m.h

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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'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