On Thu, 20 Dec 2018, Julien Grall wrote:
(Sorry for the formatting)
On Thu, 20 Dec 2018 at 23:08, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
On Thu, 20 Dec 2018, Julien Grall wrote:
> Current, foreign mappings can only be read-write. A follow-up patch
will
> extend foreign mapping for Xen backend memory (via XEN_DOMID), some of
> that memory should only be read accessible for the mapping domain.
>
> Introduce a new p2m_type to cater read-only foreign mappings. For now,
> the decision between the two foreign mapping type is based on the type
> of the guest page mapped.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> ---
>
> Changes in v2:
> - Add Andrii's reviewed-by
> ---
> xen/arch/arm/mm.c | 2 +-
> xen/arch/arm/p2m.c | 1 +
> xen/include/asm-arm/p2m.h | 42
+++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7193d83b44..58f7e54640 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1267,7 +1267,7 @@ int xenmem_add_to_physmap_one(
> }
>
> mfn = page_to_mfn(page);
> - t = p2m_map_foreign_rw;
> + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
p2m_map_foreign_ro;
I know there is a p2m_is_ram check close by, but I think it would still
be better to do:
if (p2mt == p2m_ram_rw)
t = p2m_map_foreign_rw;
else if (p2mt == p2m_ram_ro)
t = p2m_map_foreign_ro;
else
error
to avoid cases where p2mt is something completely different
(p2m_mmio_direct_dev for instance) and t gets set to p2m_map_foreign_ro
by mistake.
The case you suggest is impossible. You can only be here if you manage to get a
reference on the page (e.g p2m_foreign or
p2m_ram).
The check above remove the foreign types. But if you ever get here there are
not much harm done as it would be read-only.
The extras 5 lines of code are just not worth it.
I realize the case is impossible today, it was for clarity and for
future proof-ness. You can reduce line code count by combining it with
the p2m_is_ram check above:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 49d7a76..01ae2cc 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1259,7 +1259,9 @@ int xenmem_add_to_physmap_one(
return -EINVAL;
}
- if ( !p2m_is_ram(p2mt) )
+ if ( p2m_is_ram(p2mt) )
+ t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
+ else
{
put_page(page);
put_pg_owner(od);
@@ -1267,7 +1269,6 @@ int xenmem_add_to_physmap_one(
}
mfn = page_to_mfn(page);
- t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
put_pg_owner(od);
break;
