Re: [PATCH v2 5/8] xen/riscv: introduce asm/pmap.h header

[Julien Grall <julien@xxxxxxx>]

On 22/07/2024 13:58, Jan Beulich wrote:
> On 21.07.2024 10:51, Julien Grall wrote:
> > On 12/07/2024 17:22, Oleksii Kurochko wrote:
> > > +inline pte_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr)
> > > +{
> > > +    /* there is no attr field in RISC-V's pte */
> > > +    (void) attr;
> > Surely you have a way to say indicate whether an entry is readable/writable?
> I'm puzzled by this question. The sole outlier in Arm code is pmap.h, in
> passing PAGE_HYPERVISOR_RW to this function when all others pass memory
> types (MT_*).  > Afaics only the low three bits are then used in the
> function, discarding access control bits altogether. R/W access appears
> to be implied.
Arm is not exempt of odd interfaces. However, from just this patch, it 
is not clear why RISC-V would continue to use the same appropach if the 
attributes doesn't exist.
Looking at the rest of the series, it seems to be because we wanted to 
have a generic page-table code. If there are a desire to continue 
towards this direction, then I would assume we would need a arch 
specific way to set the read/write bit.
At which point it makes a lot more sense to push setting the access bits 
in mfn_to_xen_entry().
Even if we don't do any code consolidation, I think it is odd for Arm 
that a caller will assume mfn_to_xen_entry() will always return a 
read-writable page and update as necessary. It would be nicer to push 
this decision to mfn_to_xen_entry().
Cheers,

--
Julien Grall