[v2,08/10] xen/arm: Implement xen_rem_foreign_from_p2m

Message ID 1386560047-17500-9-git-send-email-julien.grall@linaro.org
State Superseded
Headers show

Commit Message

Julien Grall Dec. 9, 2013, 3:34 a.m.
Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v2:
        - Introduce the patch
---
 xen/arch/arm/mm.c         |   16 ++++++++++++++++
 xen/include/asm-arm/p2m.h |    6 +-----
 2 files changed, 17 insertions(+), 5 deletions(-)

Comments

Ian Campbell Dec. 9, 2013, 4:31 p.m. | #1
On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Introduce the patch
> ---
>  xen/arch/arm/mm.c         |   16 ++++++++++++++++
>  xen/include/asm-arm/p2m.h |    6 +-----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 960c872..ba51f6e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
>      share_xen_page_with_guest(page, dom_xen, readonly);
>  }
>  
> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
> +{
> +    unsigned long mfn = gmfn_to_mfn(d, gpfn);
> +    if ( !mfn_valid(mfn) )
> +    {
> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
> +                 gpfn, d->domain_id);

Is this guest triggerable? Or does it indicate a mistake when we made
the mapping?

> +        return -EINVAL;
> +    }
> +
> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
> +    put_page(mfn_to_page(mfn));

Some of this could become a common helper with create_p2m_entries and
the relinquish stuff in the previous patch.

> +
> +    return 0;
> +}
> +
>  static int xenmem_add_to_physmap_one(
>      struct domain *d,
>      uint16_t space,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index b0d3aea..cdd80d8 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -152,11 +152,7 @@ static inline int get_page_and_type(struct page_info *page,
>      return rc;
>  }
>  
> -static inline int xenmem_rem_foreign_from_p2m(struct domain *d, 
> -                                              unsigned long gpfn)
> -{
> -    return -ENOSYS;
> -}
> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn);
>  
>  #endif /* _XEN_P2M_H */
>
Julien Grall Dec. 9, 2013, 5:08 p.m. | #2
On 12/09/2013 04:31 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Introduce the patch
>> ---
>>  xen/arch/arm/mm.c         |   16 ++++++++++++++++
>>  xen/include/asm-arm/p2m.h |    6 +-----
>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 960c872..ba51f6e 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
>>      share_xen_page_with_guest(page, dom_xen, readonly);
>>  }
>>  
>> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
>> +{
>> +    unsigned long mfn = gmfn_to_mfn(d, gpfn);
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
>> +                 gpfn, d->domain_id);
> 
> Is this guest triggerable? Or does it indicate a mistake when we made
> the mapping?

It indicates a mistake when we made the mapping. Perhaps an ASSERT is
better here.

> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
>> +    put_page(mfn_to_page(mfn));
> 
> Some of this could become a common helper with create_p2m_entries and
> the relinquish stuff in the previous patch.

The best solution is to move put_page in guest_physmap_remove_page. But
as we didn't plan to handle refcount for each mapping in Xen 4.4,
perhaps we can delay it?
Ian Campbell Dec. 9, 2013, 5:18 p.m. | #3
On Mon, 2013-12-09 at 17:08 +0000, Julien Grall wrote:
> On 12/09/2013 04:31 PM, Ian Campbell wrote:
> > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>
> >> ---
> >>     Changes in v2:
> >>         - Introduce the patch
> >> ---
> >>  xen/arch/arm/mm.c         |   16 ++++++++++++++++
> >>  xen/include/asm-arm/p2m.h |    6 +-----
> >>  2 files changed, 17 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >> index 960c872..ba51f6e 100644
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -968,6 +968,22 @@ void share_xen_page_with_privileged_guests(
> >>      share_xen_page_with_guest(page, dom_xen, readonly);
> >>  }
> >>  
> >> +int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
> >> +{
> >> +    unsigned long mfn = gmfn_to_mfn(d, gpfn);
> >> +    if ( !mfn_valid(mfn) )
> >> +    {
> >> +        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
> >> +                 gpfn, d->domain_id);
> > 
> > Is this guest triggerable? Or does it indicate a mistake when we made
> > the mapping?
> 
> It indicates a mistake when we made the mapping. Perhaps an ASSERT is
> better here.

In that case either works for me.

> > 
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
> >> +    put_page(mfn_to_page(mfn));
> > 
> > Some of this could become a common helper with create_p2m_entries and
> > the relinquish stuff in the previous patch.
> 
> The best solution is to move put_page in guest_physmap_remove_page. But
> as we didn't plan to handle refcount for each mapping in Xen 4.4,
> perhaps we can delay it?

You can move it and still have it be conditional I think, since g_p_r_p
can see the type in the pte, can't it?

Ian.
Julien Grall Dec. 10, 2013, 1:33 a.m. | #4
On 12/09/2013 05:18 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 17:08 +0000, Julien Grall wrote:
>> On 12/09/2013 04:31 PM, Ian Campbell wrote:
>>> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:

[..]

>
>>>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    guest_physmap_remove_page(d, gpfn, mfn, 0);
>>>> +    put_page(mfn_to_page(mfn));
>>>
>>> Some of this could become a common helper with create_p2m_entries and
>>> the relinquish stuff in the previous patch.
>>
>> The best solution is to move put_page in guest_physmap_remove_page. But
>> as we didn't plan to handle refcount for each mapping in Xen 4.4,
>> perhaps we can delay it?
>
> You can move it and still have it be conditional I think, since g_p_r_p
> can see the type in the pte, can't it?

I plan to move the put_page in create_p2m_entries and check pte type.

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 960c872..ba51f6e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -968,6 +968,22 @@  void share_xen_page_with_privileged_guests(
     share_xen_page_with_guest(page, dom_xen, readonly);
 }
 
+int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn)
+{
+    unsigned long mfn = gmfn_to_mfn(d, gpfn);
+    if ( !mfn_valid(mfn) )
+    {
+        gdprintk(XENLOG_WARNING, "Invalid mfn for gpfn:%lx domid:%d\n",
+                 gpfn, d->domain_id);
+        return -EINVAL;
+    }
+
+    guest_physmap_remove_page(d, gpfn, mfn, 0);
+    put_page(mfn_to_page(mfn));
+
+    return 0;
+}
+
 static int xenmem_add_to_physmap_one(
     struct domain *d,
     uint16_t space,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b0d3aea..cdd80d8 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -152,11 +152,7 @@  static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
-static inline int xenmem_rem_foreign_from_p2m(struct domain *d, 
-                                              unsigned long gpfn)
-{
-    return -ENOSYS;
-}
+int xenmem_rem_foreign_from_p2m(struct domain *d, unsigned long gpfn);
 
 #endif /* _XEN_P2M_H */