[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id together



On 12.10.20 11:48, Paul Durrant wrote:
-----Original Message-----
From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Juergen 
Gross
Sent: 12 October 2020 10:28
To: xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Juergen Gross <jgross@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; 
George Dunlap
<george.dunlap@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; Jan Beulich 
<jbeulich@xxxxxxxx>; Julien
Grall <julien@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
<wl@xxxxxxx>
Subject: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id 
together

The queue for a fifo event is depending on the vcpu_id and the
priority of the event. When sending an event it might happen the
event needs to change queues and the old queue needs to be kept for
keeping the links between queue elements intact. For this purpose
the event channel contains last_priority and last_vcpu_id values
elements for being able to identify the old queue.

In order to avoid races always access last_priority and last_vcpu_id
with a single atomic operation avoiding any inconsistencies.

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
  xen/common/event_fifo.c | 25 +++++++++++++++++++------
  xen/include/xen/sched.h |  3 +--
  2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index fc189152e1..fffbd409c8 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -42,6 +42,14 @@ struct evtchn_fifo_domain {
      unsigned int num_evtchns;
  };

+union evtchn_fifo_lastq {
+    u32 raw;
+    struct {
+        u8 last_priority;
+        u16 last_vcpu_id;
+    };
+};

I guess you want to s/u32/uint32_t, etc. above.

Hmm, yes, probably.


+
  static inline event_word_t *evtchn_fifo_word_from_port(const struct domain *d,
                                                         unsigned int port)
  {
@@ -86,16 +94,18 @@ static struct evtchn_fifo_queue *lock_old_queue(const 
struct domain *d,
      struct vcpu *v;
      struct evtchn_fifo_queue *q, *old_q;
      unsigned int try;
+    union evtchn_fifo_lastq lastq;

      for ( try = 0; try < 3; try++ )
      {
-        v = d->vcpu[evtchn->last_vcpu_id];
-        old_q = &v->evtchn_fifo->queue[evtchn->last_priority];
+        lastq.raw = read_atomic(&evtchn->fifo_lastq);
+        v = d->vcpu[lastq.last_vcpu_id];
+        old_q = &v->evtchn_fifo->queue[lastq.last_priority];

          spin_lock_irqsave(&old_q->lock, *flags);

-        v = d->vcpu[evtchn->last_vcpu_id];
-        q = &v->evtchn_fifo->queue[evtchn->last_priority];
+        v = d->vcpu[lastq.last_vcpu_id];
+        q = &v->evtchn_fifo->queue[lastq.last_priority];

          if ( old_q == q )
              return old_q;
@@ -246,8 +256,11 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct 
evtchn *evtchn)
          /* Moved to a different queue? */
          if ( old_q != q )
          {
-            evtchn->last_vcpu_id = v->vcpu_id;
-            evtchn->last_priority = q->priority;
+            union evtchn_fifo_lastq lastq;
+
+            lastq.last_vcpu_id = v->vcpu_id;
+            lastq.last_priority = q->priority;
+            write_atomic(&evtchn->fifo_lastq, lastq.raw);


You're going to leak some stack here I think. Perhaps add a 'pad' field between 
'last_priority' and 'last_vcpu_id' and zero it?

I can do that, but why? This is nothing a guest is supposed to see at
any time.


Juergen



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.