Re: [PATCH v1 5/6] xen/riscv: add new p2m types and helper macros for type classification

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.05.2025 13:34, Oleksii Kurochko wrote:
> On 5/20/25 5:11 PM, Jan Beulich wrote:
>> On 09.05.2025 17:57, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/p2m.h
>>> +++ b/xen/arch/riscv/include/asm/p2m.h
>>> @@ -80,8 +80,36 @@ struct p2m_domain {
>>>   typedef enum {
>>>       p2m_invalid = 0,    /* Nothing mapped here */
>>>       p2m_ram_rw,         /* Normal read/write domain RAM */
>>> +    p2m_ram_ro,         /* Read-only; writes are silently dropped */
>> This is pretty special a type, which imo better wouldn't be introduced
>> without there being proper support for it. (I don't suppose RISC-V
>> hardware alone can effect this type?)
> 
> It is possible to make ro by using r, w, x bits of page table entry in the
> same way Arm does that:
>      case p2m_ram_ro:
>          e->p2m.xn = 0;
>          e->p2m.write = 0;
>          break;

That takes care of the r/o aspect, yes, but not of the "writes are silently
dropped" one.

>>> +    p2m_mmio_direct_dev,/* Read/write mapping of genuine Device MMIO area 
>>> */
>>> +    p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
>>> +    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */
>> Aiui you took these from Arm. Looking at its sole use, I'm not convinced
>> it's used correctly. If it is, the same comment as for p2m_ram_ro above
>> would apply here, too.
> 
> p2m_mmio_direct_dev - this one is defintely needed as it is used for device
> pass through to guest domain to map device's MMIO. It seems to me like it is
> correctly used.

My earlier comment was mainly about p2m_map_foreign_ro.

> Others we don't really use now in private branches but it seems like they 
> could be
> useful, so I added them now.
> 
> I can drop them now and return back if such functionality which uses them 
> will be
> introduced for RISC-V, and at that moment I think it will be
> more clear if it is used correctly or not.

Indeed. And maybe it'll just be p2m_map_foreign, as we have it on x86.

Jan