Re: [PATCH] Arm: tighten translate_get_page()

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 17/02/2026 16:54, Jan Beulich wrote:
> On 17.02.2026 16:28, Orzel, Michal wrote:
>>
>>
>> On 16/02/2026 16:20, Jan Beulich wrote:
>>> Permitting writes when the P2M type says "read-only" can't be correct.
>>>
>>> Fixes: 1661158723a ("xen/arm: Extend copy_to_guest to support copying 
>>> from/to guest physical address")
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> Thanks.
> 
>>> ---
>>> What exactly p2m_ram_ro means on Arm is unclear: The comment next to its
>>> definition says one thing, its use in get_page_from_gfn() says another.
>>> (I remember raising this point before, i.e. it feels a little odd that the
>>> ambiguity still exists.) The patch here assumes the comment is what is
>>> wrong.
>>>
>>> --- a/xen/arch/arm/guestcopy.c
>>> +++ b/xen/arch/arm/guestcopy.c
>>> @@ -44,7 +44,7 @@ static struct page_info *translate_get_p
>>>      if ( !page )
>>>          return NULL;
>>>  
>>> -    if ( !p2m_is_ram(p2mt) )
>>> +    if ( write ? p2mt != p2m_ram_rw : !p2m_is_ram(p2mt) )
>>>      {
>>>          put_page(page);
>>>          return NULL;
>>
>> The ambiguity you mention is indeed problematic. This mixes page type with 
>> p2m
>> type. The comment "The p2m_type is based on the type of the page" admits this
>> conflation for DOMID_XEN.
>>
>> AFAICT, p2m_ram_ro is not used on Arm for normal domains. The only use is in
>> get_page_from_gfn() for DOMID_XEN. Maybe we could change get_page_from_gfn() 
>> to
>> always return p2m_ram_rw since DOMID_XEN has direct 1:1 access anyway?
> 
> But that's not correct for cases where share_xen_page_with_privileged_guest()
> is passed SHARE_ro. XENMAPSPACE_gmfn_foreign requests have to result in r/o
> mappings in that case.
Yes. Therefore, on Arm:
- p2m_ram_ro is never stored in P2M tables for normal domains
- it's only used by get_page_from_gfn() for DOMID_XEN pages
- it's used as a signal to install p2m_map_foreign_ro mappings

The code should stay as is then and we could modify the comment to say:
/* Read-only RAM; only used for DOMID_XEN */

~Michal