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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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?
> 
>> +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