RE: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Henry Wang <Henry.Wang@xxxxxxx>]

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 <julien@xxxxxxx>
> 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