diff mbox

[v3,02/38] arm/p2m: Expose p2m_*lock helpers

Message ID 9ade638a-6a63-8f32-9d09-cf9207964f45@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall Sept. 2, 2016, 10:15 a.m. UTC
On 02/09/16 11:12, Sergej Proskurin wrote:
> Hi Julien,

Hello Sergej,

> On 09/01/2016 05:48 PM, Julien Grall wrote:
>> Hello Sergej,
>>
>> On 16/08/16 23:16, Sergej Proskurin wrote:
>>> This commit exposes the "p2m_*lock" helpers, as they will be used within
>>> the file ./xen/arch/arm/altp2m.c, as will be shown in the following
>>> commits.
>>>
>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>> ---
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> ---
>>>  xen/arch/arm/p2m.c        | 12 ++++++------
>>>  xen/include/asm-arm/p2m.h | 16 ++++++++++++++++
>>>  2 files changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index da6c7d4..08114d8 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -62,14 +62,14 @@ static inline bool_t p2m_is_superpage(lpae_t pte,
>>> unsigned int level)
>>>      return (level < 3) && p2m_mapping(pte);
>>>  }
>>>
>>> -static inline void p2m_write_lock(struct p2m_domain *p2m)
>>> +void p2m_write_lock(struct p2m_domain *p2m)
>>
>> This will introduce an overhead when locking the p2m. Those helpers
>> should be moved a inline in p2m.h and not transform to a functions.
>>
> 
> I did not move the definitions of the functions into the header file to
> avoid exposing the static function p2m_flush_tlb. However, what I can do
> is to declare the function prototypes in p2m.h and define them as inline
> in p2m.c, which should provide the same effect.

It will not have the same effect for call of p2m_*lock outside of p2m.c.
p2m_flush_tlb is only used in p2m_write_unlock, so what is the problem to
move the implement of all p2m_*lock but p2m_write_unlock in p2m.h?

commit 57b5214c6225c113b3cc50edbadce81b84e0a32c
Author: Julien Grall <julien.grall@arm.com>
Date:   Tue Aug 2 15:48:48 2016 +0100

    xen/arm: p2m: Export p2m_*_lock helpers
    
    Early patch exported the p2m interface (p2m_get_entry and p2m_set_entry)
    to allow splitting xen/arch/arm/p2m.c. Those functions require the
    callers to lock the p2m, so we need to export p2m_*_lock helpers.
    
    All helpers but p2m_write_unlock but p2m_write_unlock are moved in
    xen/include/asm-arm/p2m.h to allow inlining. The helpers
    p2m_write_unlock is kept in p2m.c because it depends on a static
    function.
    
    Signed-off-by: Julien Grall <julien.grall@arm.com>
    
    ---
        Changes in v2:
            - Patch added

Comments

Sergej Proskurin Sept. 2, 2016, 10:29 a.m. UTC | #1
Hi Julien,

On 09/02/2016 12:15 PM, Julien Grall wrote:
> On 02/09/16 11:12, Sergej Proskurin wrote:
>> Hi Julien,
> 
> Hello Sergej,
> 
>> On 09/01/2016 05:48 PM, Julien Grall wrote:
>>> Hello Sergej,
>>>
>>> On 16/08/16 23:16, Sergej Proskurin wrote:
>>>> This commit exposes the "p2m_*lock" helpers, as they will be used within
>>>> the file ./xen/arch/arm/altp2m.c, as will be shown in the following
>>>> commits.
>>>>
>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>  xen/arch/arm/p2m.c        | 12 ++++++------
>>>>  xen/include/asm-arm/p2m.h | 16 ++++++++++++++++
>>>>  2 files changed, 22 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index da6c7d4..08114d8 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -62,14 +62,14 @@ static inline bool_t p2m_is_superpage(lpae_t pte,
>>>> unsigned int level)
>>>>      return (level < 3) && p2m_mapping(pte);
>>>>  }
>>>>
>>>> -static inline void p2m_write_lock(struct p2m_domain *p2m)
>>>> +void p2m_write_lock(struct p2m_domain *p2m)
>>>
>>> This will introduce an overhead when locking the p2m. Those helpers
>>> should be moved a inline in p2m.h and not transform to a functions.
>>>
>>
>> I did not move the definitions of the functions into the header file to
>> avoid exposing the static function p2m_flush_tlb. However, what I can do
>> is to declare the function prototypes in p2m.h and define them as inline
>> in p2m.c, which should provide the same effect.
> 
> It will not have the same effect for call of p2m_*lock outside of p2m.c.
> p2m_flush_tlb is only used in p2m_write_unlock, so what is the problem to
> move the implement of all p2m_*lock but p2m_write_unlock in p2m.h?
> 

There is no problem at all. I will adapt this patch based on your
suggested patch, thank you.

> commit 57b5214c6225c113b3cc50edbadce81b84e0a32c
> Author: Julien Grall <julien.grall@arm.com>
> Date:   Tue Aug 2 15:48:48 2016 +0100
> 
>     xen/arm: p2m: Export p2m_*_lock helpers
>     
>     Early patch exported the p2m interface (p2m_get_entry and p2m_set_entry)
>     to allow splitting xen/arch/arm/p2m.c. Those functions require the
>     callers to lock the p2m, so we need to export p2m_*_lock helpers.
>     
>     All helpers but p2m_write_unlock but p2m_write_unlock are moved in
>     xen/include/asm-arm/p2m.h to allow inlining. The helpers
>     p2m_write_unlock is kept in p2m.c because it depends on a static
>     function.
>     
>     Signed-off-by: Julien Grall <julien.grall@arm.com>
>     
>     ---
>         Changes in v2:
>             - Patch added
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f4a808c..e41ed3c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -62,14 +62,10 @@ static inline bool_t p2m_is_superpage(lpae_t pte, unsigned int level)
>      return (level < 3) && p2m_mapping(pte);
>  }
>  
> -static inline void p2m_write_lock(struct p2m_domain *p2m)
> -{
> -    write_lock(&p2m->lock);
> -}
> -
>  static void p2m_flush_tlb(struct p2m_domain *p2m);
>  
> -static inline void p2m_write_unlock(struct p2m_domain *p2m)
> +/* Unlock the flush and do a P2M TLB flush if necessary */
> +void p2m_write_unlock(struct p2m_domain *p2m)
>  {
>      if ( p2m->need_flush )
>      {
> @@ -85,26 +81,6 @@ static inline void p2m_write_unlock(struct p2m_domain *p2m)
>      write_unlock(&p2m->lock);
>  }
>  
> -static inline void p2m_read_lock(struct p2m_domain *p2m)
> -{
> -    read_lock(&p2m->lock);
> -}
> -
> -static inline void p2m_read_unlock(struct p2m_domain *p2m)
> -{
> -    read_unlock(&p2m->lock);
> -}
> -
> -static inline int p2m_is_locked(struct p2m_domain *p2m)
> -{
> -    return rw_is_locked(&p2m->lock);
> -}
> -
> -static inline int p2m_is_write_locked(struct p2m_domain *p2m)
> -{
> -    return rw_is_write_locked(&p2m->lock);
> -}
> -
>  void p2m_dump_info(struct domain *d)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index c2f4f98..9b0cade 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -173,6 +173,33 @@ void p2m_restore_state(struct vcpu *n);
>  /* Print debugging/statistial info about a domain's p2m */
>  void p2m_dump_info(struct domain *d);
>  
> +static inline void p2m_write_lock(struct p2m_domain *p2m)
> +{
> +    write_lock(&p2m->lock);
> +}
> +
> +void p2m_write_unlock(struct p2m_domain *p2m);
> +
> +static inline void p2m_read_lock(struct p2m_domain *p2m)
> +{
> +    read_lock(&p2m->lock);
> +}
> +
> +static inline void p2m_read_unlock(struct p2m_domain *p2m)
> +{
> +    read_unlock(&p2m->lock);
> +}
> +
> +static inline int p2m_is_locked(struct p2m_domain *p2m)
> +{
> +    return rw_is_locked(&p2m->lock);
> +}
> +
> +static inline int p2m_is_write_locked(struct p2m_domain *p2m)
> +{
> +    return rw_is_write_locked(&p2m->lock);
> +}
> +
>  /* Look up the MFN corresponding to a domain's GFN. */
>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>  
> 

Cheers,
~Sergej
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f4a808c..e41ed3c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -62,14 +62,10 @@  static inline bool_t p2m_is_superpage(lpae_t pte, unsigned int level)
     return (level < 3) && p2m_mapping(pte);
 }
 
-static inline void p2m_write_lock(struct p2m_domain *p2m)
-{
-    write_lock(&p2m->lock);
-}
-
 static void p2m_flush_tlb(struct p2m_domain *p2m);
 
-static inline void p2m_write_unlock(struct p2m_domain *p2m)
+/* Unlock the flush and do a P2M TLB flush if necessary */
+void p2m_write_unlock(struct p2m_domain *p2m)
 {
     if ( p2m->need_flush )
     {
@@ -85,26 +81,6 @@  static inline void p2m_write_unlock(struct p2m_domain *p2m)
     write_unlock(&p2m->lock);
 }
 
-static inline void p2m_read_lock(struct p2m_domain *p2m)
-{
-    read_lock(&p2m->lock);
-}
-
-static inline void p2m_read_unlock(struct p2m_domain *p2m)
-{
-    read_unlock(&p2m->lock);
-}
-
-static inline int p2m_is_locked(struct p2m_domain *p2m)
-{
-    return rw_is_locked(&p2m->lock);
-}
-
-static inline int p2m_is_write_locked(struct p2m_domain *p2m)
-{
-    return rw_is_write_locked(&p2m->lock);
-}
-
 void p2m_dump_info(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c2f4f98..9b0cade 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -173,6 +173,33 @@  void p2m_restore_state(struct vcpu *n);
 /* Print debugging/statistial info about a domain's p2m */
 void p2m_dump_info(struct domain *d);
 
+static inline void p2m_write_lock(struct p2m_domain *p2m)
+{
+    write_lock(&p2m->lock);
+}
+
+void p2m_write_unlock(struct p2m_domain *p2m);
+
+static inline void p2m_read_lock(struct p2m_domain *p2m)
+{
+    read_lock(&p2m->lock);
+}
+
+static inline void p2m_read_unlock(struct p2m_domain *p2m)
+{
+    read_unlock(&p2m->lock);
+}
+
+static inline int p2m_is_locked(struct p2m_domain *p2m)
+{
+    return rw_is_locked(&p2m->lock);
+}
+
+static inline int p2m_is_write_locked(struct p2m_domain *p2m)
+{
+    return rw_is_write_locked(&p2m->lock);
+}
+
 /* Look up the MFN corresponding to a domain's GFN. */
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);