Hi Juergen,
On 27/11/2020 14:05, Jürgen Groß wrote:
On 27.11.20 14:58, Julien Grall wrote:
Hi Juergen,
On 25/11/2020 10:51, Juergen Gross wrote:
-static struct evtchn_fifo_queue *lock_old_queue(const struct domain *d,
- struct evtchn *evtchn,
- unsigned long *flags)
-{
- struct vcpu *v;
- struct evtchn_fifo_queue *q, *old_q;
- unsigned int try;
- union evtchn_fifo_lastq lastq;
-
- for ( try = 0; try < 3; try++ )
- {
- 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[lastq.last_vcpu_id];
- q = &v->evtchn_fifo->queue[lastq.last_priority];
-
- if ( old_q == q )
- return old_q;
-
- spin_unlock_irqrestore(&old_q->lock, *flags);
- }
-
- gprintk(XENLOG_WARNING,
- "dom%d port %d lost event (too many queue changes)\n",
- d->domain_id, evtchn->port);
- return NULL;
-}
-
static int try_set_link(event_word_t *word, event_word_t *w,
uint32_t link)
{
event_word_t new, old;
@@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu
*v, struct evtchn *evtchn)
event_word_t *word;
unsigned long flags;
bool_t was_pending;
+ struct evtchn_fifo_queue *q, *old_q;
+ unsigned int try;
+ bool linked = true;
port = evtchn->port;
word = evtchn_fifo_word_from_port(d, port);
@@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu
*v, struct evtchn *evtchn)
return;
}
+ /*
+ * Lock all queues related to the event channel (in case of a
queue change
+ * this might be two).
+ * It is mandatory to do that before setting and testing the
PENDING bit
+ * and to hold the current queue lock until the event has put
into the
+ * list of pending events in order to avoid waking up a guest
without the
+ * event being visibly pending in the guest.
+ */
+ for ( try = 0; try < 4; try++ )
May I ask why the number of try is 4 rather than the original 3?
Oh, I think this is just a typo. OTOH it doesn't really matter.
I agree that the number of try was likely random and therefore using a
different number should not matter.
However, this is making more difficult to review the patch because this
is an unexplained change.
I would prefer if this is dropped. But if you want to keep this change,
then it should be explained in the commit message.
Cheers,
--
Julien Grall
