Hi Julien, Stefano, Bertrand,
I am trying to work on the follow-up improvements about the Arm P2M code,
and while trying to address the comment below, I noticed there was an unfinished
discussion between me and Julien which I would like to continue and here
opinions from all of you (if possible).
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Subject: Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in
> arch_domain_create()
> >> I also noticed that relinquish_p2m_mapping() is not called. This should
> >> be fine for us because arch_domain_create() should never create a
> >> mapping that requires p2m_put_l3_page() to be called.
For the background of the discussion, this was about the failure path of
arch_domain_create(), where we only call p2m_final_teardown() which does
not call relinquish_p2m_mapping()...
> >>
> >> I think it would be good to check it in __p2m_set_entry(). So we don't
> >> end up to add such mappings by mistake.
>
> For checking the mapping, we can do:
>
> if ( !removing_mapping && (p2m_is_foreign(t) || (p2m_is_ram(t) &&
> is_xenheap_mfn(mfn)) )
> return -EINVAL;
>
> We also need a way to check whether we are called from
> arch_domain_create(). I think we would need a field in the domain
> structure to indicate whether it is still initializating.
>
> This is a bit ugly though. Any other suggestions?
...and I agree with Julien that this check makes great sense and I will add
this check in the follow up patch as discussed.
However I am not really convinced this looks pretty, and I would like to
hear opinions / suggestions about below code, does below code snippet
seem plausible to you?
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
@@ -1043,6 +1043,10 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
*/
ASSERT(target > 0 && target <= 3);
+ if ( !removing_mapping && d->arch.p2m.from_arch_domain_create &&
+ (p2m_is_foreign(t) || (p2m_is_ram(t) && is_xenheap_mfn(mfn)) )
+ return -EINVAL;
+
table = p2m_get_root_pointer(p2m, sgfn);
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
@@ -687,6 +687,8 @@ int arch_domain_create(struct domain *d,
if ( (rc = p2m_init(d)) != 0 )
goto fail;
+ d->arch.p2m.from_arch_domain_create = true;
+
rc = -ENOMEM;
if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
goto fail;
@@ -751,6 +753,8 @@ int arch_domain_create(struct domain *d,
if ( (rc = domain_vpci_init(d)) != 0 )
goto fail;
+ d->arch.p2m.from_arch_domain_create = false;
+
return 0;
Kind regards,
Henry
