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 <[email protected]>
>> Reviewed-by: Michal Orzel <[email protected]>
>
> 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
