[PATCH] xen/xsm: Improve alloc/free of evtchn buckets

[PATCH] xen/xsm: Improve alloc/free of evtchn buckets

[Andrew Cooper]

Currently, flask_alloc_security_evtchn() is called in loops of
64 (EVTCHNS_PER_BUCKET), which for non-dummy implementations is a function
pointer call even in the no-op case.  The non no-op case only sets a single
constant, and doesn't actually fail.

Spectre v2 protections has made function pointer calls far more expensive, and
64 back-to-back calls is a waste.  Rework the APIs to pass the size of the
bucket instead, and call them once.

No practical change, but {alloc,free}_evtchn_bucket() should be rather more
efficient now.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>

Spotted while fixing up evtchn to use fault-ttl semantics for testing the
error handling logic.
---
 xen/common/event_channel.c | 36 ++++++++++++++++--------------------
 xen/include/xsm/dummy.h    |  4 ++--
 xen/include/xsm/xsm.h      | 12 ++++++------
 xen/xsm/dummy.c            |  4 ++--
 xen/xsm/flask/hooks.c      | 20 +++++++++++++-------
 5 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index d590ddad99..2d84da2186 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -147,6 +147,14 @@ static bool virq_is_global(unsigned int virq)
     return true;
 }
 
+static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
+{
+    if ( !bucket )
+        return;
+
+    xsm_free_security_evtchns(bucket, EVTCHNS_PER_BUCKET);
+    xfree(bucket);
+}
 
 static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
 {
@@ -155,34 +163,22 @@ static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
 
     chn = xzalloc_array(struct evtchn, EVTCHNS_PER_BUCKET);
     if ( !chn )
-        return NULL;
+        goto err;
+
+    if ( xsm_alloc_security_evtchns(chn, EVTCHNS_PER_BUCKET) )
+        goto err;
 
     for ( i = 0; i < EVTCHNS_PER_BUCKET; i++ )
     {
-        if ( xsm_alloc_security_evtchn(&chn[i]) )
-        {
-            while ( i-- )
-                xsm_free_security_evtchn(&chn[i]);
-            xfree(chn);
-            return NULL;
-        }
         chn[i].port = port + i;
         rwlock_init(&chn[i].lock);
     }
-    return chn;
-}
-
-static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
-{
-    unsigned int i;
 
-    if ( !bucket )
-        return;
-
-    for ( i = 0; i < EVTCHNS_PER_BUCKET; i++ )
-        xsm_free_security_evtchn(bucket + i);
+    return chn;
 
-    xfree(bucket);
+ err:
+    free_evtchn_bucket(d, chn);
+    return NULL;
 }
 
 int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index fa40e880ba..b215429581 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -309,12 +309,12 @@ static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG struct domain *d1, struct
     return xsm_default_action(action, d1, d2);
 }
 
-static XSM_INLINE int xsm_alloc_security_evtchn(struct evtchn *chn)
+static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn *chn, unsigned int nr)
 {
     return 0;
 }
 
-static XSM_INLINE void xsm_free_security_evtchn(struct evtchn *chn)
+static XSM_INLINE void xsm_free_security_evtchns(struct evtchn *chn, unsigned int nr)
 {
     return;
 }
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 7bd03d8817..aaa3f60d9e 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -72,8 +72,8 @@ struct xsm_operations {
 
     int (*alloc_security_domain) (struct domain *d);
     void (*free_security_domain) (struct domain *d);
-    int (*alloc_security_evtchn) (struct evtchn *chn);
-    void (*free_security_evtchn) (struct evtchn *chn);
+    int (*alloc_security_evtchns) (struct evtchn *chn, unsigned int nr);
+    void (*free_security_evtchns) (struct evtchn *chn, unsigned int nr);
     char *(*show_security_evtchn) (struct domain *d, const struct evtchn *chn);
     int (*init_hardware_domain) (struct domain *d);
 
@@ -314,14 +314,14 @@ static inline void xsm_free_security_domain (struct domain *d)
     xsm_ops->free_security_domain(d);
 }
 
-static inline int xsm_alloc_security_evtchn (struct evtchn *chn)
+static inline int xsm_alloc_security_evtchns(struct evtchn *chn, unsigned int nr)
 {
-    return xsm_ops->alloc_security_evtchn(chn);
+    return xsm_ops->alloc_security_evtchns(chn, nr);
 }
 
-static inline void xsm_free_security_evtchn (struct evtchn *chn)
+static inline void xsm_free_security_evtchns(struct evtchn *chn, unsigned int nr)
 {
-    (void)xsm_ops->free_security_evtchn(chn);
+    xsm_ops->free_security_evtchns(chn, nr);
 }
 
 static inline char *xsm_show_security_evtchn (struct domain *d, const struct evtchn *chn)
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 9e09512144..715aa1bcb5 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -49,8 +49,8 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
 
     set_to_dummy_if_null(ops, alloc_security_domain);
     set_to_dummy_if_null(ops, free_security_domain);
-    set_to_dummy_if_null(ops, alloc_security_evtchn);
-    set_to_dummy_if_null(ops, free_security_evtchn);
+    set_to_dummy_if_null(ops, alloc_security_evtchns);
+    set_to_dummy_if_null(ops, free_security_evtchns);
     set_to_dummy_if_null(ops, show_security_evtchn);
     set_to_dummy_if_null(ops, init_hardware_domain);
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 19b0d9e3eb..562754f3b4 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -307,19 +307,25 @@ static int flask_evtchn_reset(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_EVENT, EVENT__RESET);
 }
 
-static int flask_alloc_security_evtchn(struct evtchn *chn)
+static int flask_alloc_security_evtchns(struct evtchn *chn, unsigned int nr)
 {
-    chn->ssid.flask_sid = SECINITSID_UNLABELED;
+    unsigned int i;
+
+    for ( i = 0; i < nr; ++i )
+        chn[i].ssid.flask_sid = SECINITSID_UNLABELED;
 
-    return 0;    
+    return 0;
 }
 
-static void flask_free_security_evtchn(struct evtchn *chn)
+static void flask_free_security_evtchns(struct evtchn *chn, unsigned int nr)
 {
+    unsigned int i;
+
     if ( !chn )
         return;
 
-    chn->ssid.flask_sid = SECINITSID_UNLABELED;
+    for ( i = 0; i < nr; ++i )
+        chn[i].ssid.flask_sid = SECINITSID_UNLABELED;
 }
 
 static char *flask_show_security_evtchn(struct domain *d, const struct evtchn *chn)
@@ -1766,8 +1772,8 @@ static struct xsm_operations flask_ops = {
 
     .alloc_security_domain = flask_domain_alloc_security,
     .free_security_domain = flask_domain_free_security,
-    .alloc_security_evtchn = flask_alloc_security_evtchn,
-    .free_security_evtchn = flask_free_security_evtchn,
+    .alloc_security_evtchns = flask_alloc_security_evtchns,
+    .free_security_evtchns = flask_free_security_evtchns,
     .show_security_evtchn = flask_show_security_evtchn,
     .init_hardware_domain = flask_init_hardware_domain,
 
-- 
2.11.0

Re: [PATCH] xen/xsm: Improve alloc/free of evtchn buckets

[Jan Beulich]

On 18.01.2021 16:06, Andrew Cooper wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -147,6 +147,14 @@ static bool virq_is_global(unsigned int virq)
>      return true;
>  }
>  
> +static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
> +{
> +    if ( !bucket )
> +        return;

You could avoid this since flask_free_security_evtchns() has
a similar check. Alternatively it could be dropped from there.
But even if you want to keep the duplication
Reviewed-by: Jan Beulich <jbeulich@suse.com>

One further aspect to consider though:

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -309,12 +309,12 @@ static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG struct domain *d1, struct
>      return xsm_default_action(action, d1, d2);
>  }
>  
> -static XSM_INLINE int xsm_alloc_security_evtchn(struct evtchn *chn)
> +static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn *chn, unsigned int nr)

I wonder whether we wouldn't better identify the difference
between pointer (to individual element) and array by writing
this (and others below) as

static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn chn[], unsigned int nr)

I think we've done so in a few places already, but of course it
would be a long way to get the entire code base consistent in
this regard. Plus of course while this works fine in function
declarations / definitions, it won't be possible to use for
struct / union fields.

Also it looks like this and further lines have become overly long.

Jan

Re: [PATCH] xen/xsm: Improve alloc/free of evtchn buckets

[Andrew Cooper]

On 18/01/2021 15:31, Jan Beulich wrote:
> On 18.01.2021 16:06, Andrew Cooper wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -147,6 +147,14 @@ static bool virq_is_global(unsigned int virq)
>>      return true;
>>  }
>>  
>> +static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
>> +{
>> +    if ( !bucket )
>> +        return;
> You could avoid this since flask_free_security_evtchns() has
> a similar check. Alternatively it could be dropped from there.

I considered altering both.  However, all functions like this really
should be idempotent.

For this case, the compiler can drop the check from both callsites, and
its safer if the structure of the callers change in the future.

> But even if you want to keep the duplication
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> One further aspect to consider though:
>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -309,12 +309,12 @@ static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG struct domain *d1, struct
>>      return xsm_default_action(action, d1, d2);
>>  }
>>  
>> -static XSM_INLINE int xsm_alloc_security_evtchn(struct evtchn *chn)
>> +static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn *chn, unsigned int nr)
> I wonder whether we wouldn't better identify the difference
> between pointer (to individual element) and array by writing
> this (and others below) as
>
> static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn chn[], unsigned int nr)

In which case want we want is (unsigned int nr, struct evtchn chn[nr])
which I think is the C99 way of writing this to help static analysis.

>
> I think we've done so in a few places already, but of course it
> would be a long way to get the entire code base consistent in
> this regard. Plus of course while this works fine in function
> declarations / definitions, it won't be possible to use for
> struct / union fields.
>
> Also it looks like this and further lines have become overly long.

Everything is long lines in this area of code.  Its all due an overhaul.

~Andrew

Re: [PATCH] xen/xsm: Improve alloc/free of evtchn buckets

[Daniel P. Smith]

On 1/18/21 11:21 AM, Andrew Cooper wrote:
> On 18/01/2021 15:31, Jan Beulich wrote:
>> On 18.01.2021 16:06, Andrew Cooper wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -147,6 +147,14 @@ static bool virq_is_global(unsigned int virq)
>>>       return true;
>>>   }
>>>   
>>> +static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
>>> +{
>>> +    if ( !bucket )
>>> +        return;
>> You could avoid this since flask_free_security_evtchns() has
>> a similar check. Alternatively it could be dropped from there.
> 
> I considered altering both.  However, all functions like this really
> should be idempotent.
> 
> For this case, the compiler can drop the check from both callsites, and
> its safer if the structure of the callers change in the future.
> 
>> But even if you want to keep the duplication
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.

You can add,
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

>>
>> One further aspect to consider though:
>>
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -309,12 +309,12 @@ static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG struct domain *d1, struct
>>>       return xsm_default_action(action, d1, d2);
>>>   }
>>>   
>>> -static XSM_INLINE int xsm_alloc_security_evtchn(struct evtchn *chn)
>>> +static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn *chn, unsigned int nr)
>> I wonder whether we wouldn't better identify the difference
>> between pointer (to individual element) and array by writing
>> this (and others below) as
>>
>> static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn chn[], unsigned int nr)
> 
> In which case want we want is (unsigned int nr, struct evtchn chn[nr])
> which I think is the C99 way of writing this to help static analysis.

Would it be better to switch this to the (unsigned int nr, struct evtchn 
chn[nr]) you suggested now or wait to make the change uniform? IMHO it 
is probably better for the sake of existing consistency to keep what you 
have here, with the understanding that this might get redone as part of 
a larger function semantic change.

>>
>> I think we've done so in a few places already, but of course it
>> would be a long way to get the entire code base consistent in
>> this regard. Plus of course while this works fine in function
>> declarations / definitions, it won't be possible to use for
>> struct / union fields.
>>
>> Also it looks like this and further lines have become overly long.
> 
> Everything is long lines in this area of code.  Its all due an overhaul.
> 
> ~Andrew
>